Skip to content

feat: add password reset for default admin/user accounts#1457

Open
jescalada wants to merge 7 commits intofinos:mainfrom
jescalada:1022-force-admin-password-reset
Open

feat: add password reset for default admin/user accounts#1457
jescalada wants to merge 7 commits intofinos:mainfrom
jescalada:1022-force-admin-password-reset

Conversation

@jescalada
Copy link
Copy Markdown
Contributor

@jescalada jescalada commented Mar 14, 2026

Fixes #1022.

I opted for the password reset solution instead of relying on the config, since I didn't want to cause any extra confusion for new users (GitProxy administrators) or friction for testing/development which relies on these dummy accounts.

The bulk of the changes are test files and frontend additions/refactoring.

I also added tests to improve coverage for auth routes.

Disclaimer: This PR was created with agentic AI and refactored manually to improve code quality. I validated the password reset flow manually and with Cypress tests. Please keep this in mind during review - there may be some obvious mistakes I missed!

Screenshot

Shown automatically when logging in with admin/admin for the first time, and when trying to navigate to other pages. Note that this is only visible when NODE_ENV is set to production.

image

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 14, 2026

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit bd57b22
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/69ca85417d0d1f0008de21bc
😎 Deploy Preview https://deploy-preview-1457.git-proxy.preview.finos.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 14, 2026

Codecov Report

❌ Patch coverage is 89.87342% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.88%. Comparing base (4e2eea8) to head (bd57b22).

Files with missing lines Patch % Lines
src/service/routes/auth.ts 91.75% 7 Missing and 1 partial ⚠️
src/service/passport/passwordChangeHandler.ts 44.44% 5 Missing ⚠️
src/service/routes/utils.ts 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1457      +/-   ##
==========================================
+ Coverage   89.66%   89.88%   +0.21%     
==========================================
  Files          68       69       +1     
  Lines        4869     5011     +142     
  Branches      888      922      +34     
==========================================
+ Hits         4366     4504     +138     
- Misses        485      488       +3     
- Partials       18       19       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@dcoric dcoric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall approach is solid. I spotted a few small things worth addressing

Removing this as testing is more trouble than it's worth (app is internal, /change-password only works with local auth and only used on first login)
@jescalada
Copy link
Copy Markdown
Contributor Author

@dcoric I've experimented with session regeneration/logging out after resetting password, but both of these seem to cause trouble due to differences between unit test vs E2E vs production. I made another PR for debugging the failing tests #1463 but couldn't find a setup that works as intended.

I believe there's no real risk of session-based attacks (orgs implementing GitProxy use OIDC/AD instead of local auth, plus password resetting is only used in the initial setup), so I've decided to skip the session refreshing.

@jescalada jescalada marked this pull request as ready for review March 20, 2026 07:13
@jescalada jescalada requested a review from a team as a code owner March 20, 2026 07:13
@dcoric
Copy link
Copy Markdown
Contributor

dcoric commented Mar 20, 2026

@dcoric I've experimented with session regeneration/logging out after resetting password, but both of these seem to cause trouble due to differences between unit test vs E2E vs production. I made another PR for debugging the failing tests #1463 but couldn't find a setup that works as intended.

I believe there's no real risk of session-based attacks (orgs implementing GitProxy use OIDC/AD instead of local auth, plus password resetting is only used in the initial setup), so I've decided to skip the session refreshing.

It is an extreme edge case. I think we are safe to proceed. If required we can always return to this in the future in a separated issue.

Copy link
Copy Markdown
Contributor

@dcoric dcoric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jescalada jescalada added this to the 2.1.0 milestone Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve default admin user creation

2 participants