fix: reject password changes when local auth is disabled - #1041
Conversation
| {config.get('localAuth') && ( | ||
| <ResponsiveButton | ||
| icon={<AccountCircle />} | ||
| label={name} |
There was a problem hiding this comment.
This is the only place where the currently logged in user is displayed. The button shouldn't be removed. Instead, the button should link to a new "settings" page which includes the Theme setting as select box (light,dark,system). and the change password form which should be disabled when localAuth is disabled.
There was a problem hiding this comment.
Fixed, the header entry stays and always shows the username. It now links to a new /settings page instead of opening the dialog. The settings page has a theme select (light/dark/system) and the change password form. The form renders disabled when local auth is off; with local auth on it still asks for elevation first, same as the dialog did. SettingsDialog is removed since nothing opens it anymore, and the e2e change-password test goes through the new page now and I left the theme toggle in the header alone; it and the select share the same state.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1041 +/- ##
=======================================
Coverage 75.77% 75.77%
=======================================
Files 66 66
Lines 3620 3620
=======================================
Hits 2743 2743
Misses 666 666
Partials 211 211 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
76705df to
ccbbc85
Compare
|
Should we also do that for UpdateUserByID? If this is a security concern (I personally don't think so based on the issue wording) we probably should. If this is merely a UI confusion problem, I think a frontend only change is fine? If it's neither and we want to enforce some kind of new logic in the backend I'm not super sure where we are right now - can an OIDC user have a password? Should they be allowed to login if localauth is enabled? What happens if an existing localauth user is later bound by OIDC by name? |
|
It's not about a security concern. I think it's more to streamline what a non-admin user sees. If only oidc login is enabled, it seems misleading to show a change password form. I think it's okay that admins can change the passwords, even if local auth is disabled. This can be a frontend change only, but given the the change password endpoint is the only non-admin accessible endpoint, I also think it's okay to change it. Do you have a preference here? |
|
I’d prefer to keep the current backend check as well. It keeps self-service password changes consistent with the UI when local auth is disabled, while admins can still manage passwords through UpdateUserByID. |
|
I think if it's not about security why is there a difference between a user asking to change a password or an admin asking to change a password? It seems like the concern here is simply user confusion (why can I change my password even though I can't login with password), doesn't admin also have the same issue? If the reasoning here is simply "well admins know better", this sounds like a good candidate for a frontend only change , non admin users who know better can still use the API to achieve a password change for whatever reason. If some escape valves are necessary for edge cases where one might want to set a password first before enabling local auth again, I don't see why the user themself should be blocked from doing it when admins are not? |
|
I think to summarize my point here: if we believe there is some valid edge case use case where one might need to set a password before enabling local auth - this should be a simple front end change (remove the button or add a notice saying the password will not work until admin re-enabled local auth). If we believe there is not, then every form of password change should be blocked. |
The WebUI offered a "Change Password" prompt even with GOTIFY_LOCALAUTH_ENABLED=false, and the endpoint behind it accepted the change: ChangePassword never consulted the setting, so a user on an OIDC-only server could still set a local password that the login form no longer accepts. Guard the handler the same way SessionAPI.Login already does, and hide the header entry that opens the dialog when local auth is off. Closes gotify#1040
The header account entry stays as the username display and now links to a new /settings page instead of opening the change password dialog. The page holds a theme select (light, dark, system) and the change password form, rendered disabled when local auth is off. SettingsDialog is removed since nothing opens it anymore.
It's now handled in the settings page
ccbbc85 to
9032508
Compare
|
I've removed the backend changes for now. If this should be changed, then it should be changed consistently. |
3 participants
Closes #1040.
Context
The issue reports that the WebUI still offers a "Change Password" prompt when
GOTIFY_LOCALAUTH_ENABLED=false. The reporter had not tried going through with it. It turns out the prompt works:UserAPI.ChangePasswordnever consults the setting, so on an OIDC-only server a user can still set a local password that the login form no longer accepts.SessionAPI.Loginalready guards onLocalAuthEnabled; this handler was missed.Changes
Backend:
api/user.go:UserAPIgains aLocalAuthEnabledfield andChangePasswordaborts with 403 when it is off, mirroringSessionAPI.Login(same status and message shape). The swagger block for this endpoint already documents 403, sodocs/spec.jsonis unchanged.router/router.go: passconf.LocalAuthEnabledthrough, as the session handler does.UI (reworked per review):
ui/src/user/Settings.tsx(new): settings page with a theme select (light, dark, system) and the change password form. The form renders disabled when local auth is off; when it is on, it still requires elevation first, same as the dialog did.ui/src/layout/Header.tsx: the account entry stays and always shows the username; it now links to/settingsinstead of opening the dialog.ui/src/layout/Layout.tsx: adds the/settingsroute; the header theme toggle and the new select share the same state.ui/src/common/SettingsDialog.tsx: removed, nothing opens it anymore.ui/src/tests/user.test.ts: the change-password test now goes through the new page.Verification
Test_UpdatePassword_LocalAuthDisabled_Expect403asserts the 403 and that the stored password is untouched. The suite default is nowLocalAuthEnabled: true, so the existing password tests are unaffected.go-sqlite3is a stub and everyUserSuitetest fails attestdb.NewDBonmasteras well; I could not run the suite here. I verified the guard directly with a throwaway test that callsChangePasswordwith no database: it returns 403 with the change, and without it the request runs on past that point into the user lookup. CI should exercise the suite test properly.go build ./...,go vet ./api/ ./router/,gofmt -lclean.tsc --noEmit,eslint "src/**/*.{ts,tsx}",prettier --list-differentandvite buildall clean.