-
Notifications
You must be signed in to change notification settings - Fork 380
Add current password validation to self-service credential updates #5290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1347,8 +1347,9 @@ paths: | |
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/UpdateSelfUserRequest' | ||
| $ref: '#/components/schemas/UpdateSelfCredentialsRequest' | ||
| example: | ||
| currentPassword: "0ldP@ssword!" | ||
| attributes: | ||
| password: "n3wP@ssword!" | ||
|
janithjay marked this conversation as resolved.
|
||
| responses: | ||
|
|
@@ -1371,6 +1372,16 @@ paths: | |
| description: | ||
| key: "error.userservice.missing_credentials_description" | ||
| defaultValue: "At least one credential field must be provided" | ||
| unsupported-credential-type: | ||
| summary: A credential type other than password was supplied | ||
| value: | ||
| code: "USR-1024" | ||
| message: | ||
| key: "error.userservice.invalid_credential" | ||
| defaultValue: "Invalid request format" | ||
| description: | ||
| key: "error.userservice.invalid_credential_description" | ||
| defaultValue: "Invalid credential fields in request" | ||
| "401": | ||
| description: Unauthorized - missing or invalid authentication token | ||
| content: | ||
|
|
@@ -1385,6 +1396,20 @@ paths: | |
| description: | ||
| key: "error.unauthorized_description" | ||
| defaultValue: "Authentication is required to access this resource" | ||
| "403": | ||
| description: Forbidden - the supplied current password is incorrect | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Error' | ||
| example: | ||
| code: "USR-1029" | ||
| message: | ||
| key: "error.userservice.invalid_current_password" | ||
| defaultValue: "Invalid current password" | ||
| description: | ||
| key: "error.userservice.invalid_current_password_description" | ||
| defaultValue: "The provided current password is incorrect" | ||
| "404": | ||
| description: Authenticated user not found | ||
| content: | ||
|
|
@@ -2191,6 +2216,27 @@ components: | |
| description: "User attributes" | ||
| additionalProperties: true | ||
|
|
||
| UpdateSelfCredentialsRequest: | ||
| type: object | ||
| required: [attributes] | ||
| properties: | ||
| currentPassword: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we keep this generic rather than hard coding it to the password? In thunderid, credentials can be defined as you wish and there's no mandatory field called password. IMO our API design should also allow that flexibility. Existing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also raised in: #5227 (comment) |
||
| type: string | ||
| format: password | ||
| description: "The user's existing password, verified before the write. Required once the account has a password. Omit it for a first-time set, on an account with no password yet." | ||
| example: "0ldP@ssword!" | ||
| attributes: | ||
| type: object | ||
| description: "Credential attributes to write. Only `password` is accepted here; other types are rejected with 400 USR-1024." | ||
| required: [password] | ||
| properties: | ||
| password: | ||
| type: string | ||
| format: password | ||
| description: "The new password to set." | ||
| example: "n3wP@ssword!" | ||
| additionalProperties: false | ||
|
|
||
| UpdateUserCredentialsRequest: | ||
| type: object | ||
| required: [credentials] | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,11 @@ const ( | |
| CredentialTypePasskey CredentialType = "passkey" | ||
| ) | ||
|
|
||
| // CredentialTypePassword is the schema-defined password credential. It is not system-managed, but | ||
| // it is named here because it acts as the account-level proof of ownership when a user changes | ||
| // their own credentials. | ||
| const CredentialTypePassword CredentialType = "password" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we expect credential type to be password even if the credential attribute name is defined as something else in the schema? Or the attribute name itself has to be password for this to work? |
||
|
|
||
| // systemManagedCredentialTypes defines credential types that are managed by the system, | ||
| // not through user types. These may support multiple values per user. | ||
| var systemManagedCredentialTypes = []CredentialType{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,6 +141,20 @@ var ( | |
| DefaultValue: "At least one identifying attribute must be provided", | ||
| }, | ||
| } | ||
| // ErrorInvalidCurrentPassword is returned when the authenticated user's supplied current | ||
| // password does not match the stored credential. | ||
| ErrorInvalidCurrentPassword = tidcommon.ServiceError{ | ||
| Type: tidcommon.ClientErrorType, | ||
| Code: "USR-1029", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we move this below |
||
| Error: tidcommon.I18nMessage{ | ||
| Key: "error.userservice.invalid_current_password", | ||
| DefaultValue: "Invalid current password", | ||
| }, | ||
| ErrorDescription: tidcommon.I18nMessage{ | ||
| Key: "error.userservice.invalid_current_password_description", | ||
| DefaultValue: "The provided current password is incorrect", | ||
| }, | ||
| } | ||
| // ErrorMissingCredentials is the error returned when credentials are missing. | ||
| ErrorMissingCredentials = tidcommon.ServiceError{ | ||
| Type: tidcommon.ClientErrorType, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,7 +145,7 @@ func TestHandleSelfUserCredentialUpdateRequest_Success(t *testing.T) { | |
|
|
||
| mockSvc := NewUserServiceInterfaceMock(t) | ||
| credentialsJSON := json.RawMessage(`{"password":[{"value":"Secret123!"}]}`) | ||
| mockSvc.On("UpdateUserCredentials", mock.Anything, userID, credentialsJSON).Return(nil) | ||
| mockSvc.On("UpdateSelfUserPassword", mock.Anything, userID, "", credentialsJSON).Return(nil) | ||
|
|
||
| handler := newUserHandler(mockSvc) | ||
| req := httptest.NewRequest(http.MethodPost, "/users/me/update-credentials", | ||
|
|
@@ -165,7 +165,7 @@ func TestHandleSelfUserCredentialUpdateRequest_StringValue(t *testing.T) { | |
|
|
||
| mockSvc := NewUserServiceInterfaceMock(t) | ||
| credentialsJSON := json.RawMessage(`{"password":"plaintext-password"}`) | ||
| mockSvc.On("UpdateUserCredentials", mock.Anything, userID, credentialsJSON).Return(nil) | ||
| mockSvc.On("UpdateSelfUserPassword", mock.Anything, userID, "", credentialsJSON).Return(nil) | ||
|
|
||
| handler := newUserHandler(mockSvc) | ||
| req := httptest.NewRequest(http.MethodPost, "/users/me/update-credentials", | ||
|
|
@@ -241,7 +241,7 @@ func TestHandleSelfUserCredentialUpdateRequest_ErrorCases(t *testing.T) { | |
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| mockSvc := NewUserServiceInterfaceMock(t) | ||
| mockSvc.On("UpdateUserCredentials", mock.Anything, userID, tc.mockJSON).Return(tc.mockError) | ||
| mockSvc.On("UpdateSelfUserPassword", mock.Anything, userID, "", tc.mockJSON).Return(tc.mockError) | ||
|
|
||
| handler := newUserHandler(mockSvc) | ||
| req := httptest.NewRequest(http.MethodPost, "/users/me/update-credentials", | ||
|
|
@@ -267,7 +267,7 @@ func TestHandleSelfUserCredentialUpdateRequest_MultipleCredentialTypes(t *testin | |
| mockSvc := NewUserServiceInterfaceMock(t) | ||
| // Test that multiple credential types are updated in a single atomic call | ||
| credentialsJSON := json.RawMessage(`{"password":"new-password","pin":"1234"}`) | ||
| mockSvc.On("UpdateUserCredentials", mock.Anything, userID, credentialsJSON).Return(nil) | ||
| mockSvc.On("UpdateSelfUserPassword", mock.Anything, userID, "", credentialsJSON).Return(nil) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The service now rejects |
||
|
|
||
| handler := newUserHandler(mockSvc) | ||
| req := httptest.NewRequest(http.MethodPost, "/users/me/update-credentials", | ||
|
|
@@ -279,8 +279,8 @@ func TestHandleSelfUserCredentialUpdateRequest_MultipleCredentialTypes(t *testin | |
|
|
||
| require.Equal(t, http.StatusNoContent, rr.Code) | ||
| require.Equal(t, 0, rr.Body.Len()) | ||
| // Verify that UpdateUserCredentials was called exactly once with all credentials | ||
| mockSvc.AssertNumberOfCalls(t, "UpdateUserCredentials", 1) | ||
| // Verify that UpdateSelfUserPassword was called exactly once with all credentials | ||
| mockSvc.AssertNumberOfCalls(t, "UpdateSelfUserPassword", 1) | ||
| } | ||
|
|
||
| func TestHandleUserCredentialUpdateRequest_Success(t *testing.T) { | ||
|
|
@@ -1342,3 +1342,78 @@ func TestHandleSelfUserMetadataGetRequest_ServiceError(t *testing.T) { | |
|
|
||
| require.Equal(t, http.StatusNotFound, rr.Code) | ||
| } | ||
|
|
||
| // ───────────────────────────────────────────────────────────────────────────── | ||
| // Self credential update – current-password plumbing | ||
| // ───────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| func TestHandleSelfUserCredentialUpdateRequest_ForwardsCurrentPassword(t *testing.T) { | ||
| userID := testUserID789 | ||
| authCtx := security.NewSecurityContextForTest(userID, "", "", nil, nil) | ||
|
|
||
| mockSvc := NewUserServiceInterfaceMock(t) | ||
| credentialsJSON := json.RawMessage(`{"password":"n3wP@ssword!"}`) | ||
| mockSvc. | ||
| On("UpdateSelfUserPassword", mock.Anything, userID, "0ldP@ssword!", credentialsJSON). | ||
| Return(nil) | ||
|
|
||
| handler := newUserHandler(mockSvc) | ||
| req := httptest.NewRequest(http.MethodPost, "/users/me/update-credentials", | ||
| bytes.NewBufferString(`{"currentPassword":"0ldP@ssword!","attributes":{"password":"n3wP@ssword!"}}`)) | ||
| req = req.WithContext(security.WithSecurityContextTest(req.Context(), authCtx)) | ||
| rr := httptest.NewRecorder() | ||
|
|
||
| handler.HandleSelfUserCredentialUpdateRequest(rr, req) | ||
|
|
||
| require.Equal(t, http.StatusNoContent, rr.Code) | ||
| mockSvc.AssertNumberOfCalls(t, "UpdateSelfUserPassword", 1) | ||
| } | ||
|
|
||
| func TestHandleSelfUserCredentialUpdateRequest_InvalidCurrentPasswordReturns403(t *testing.T) { | ||
| userID := testUserID789 | ||
| authCtx := security.NewSecurityContextForTest(userID, "", "", nil, nil) | ||
|
|
||
| mockSvc := NewUserServiceInterfaceMock(t) | ||
| mockSvc. | ||
| On("UpdateSelfUserPassword", mock.Anything, userID, "wrong", mock.Anything). | ||
| Return(&ErrorInvalidCurrentPassword) | ||
|
|
||
| handler := newUserHandler(mockSvc) | ||
| req := httptest.NewRequest(http.MethodPost, "/users/me/update-credentials", | ||
| bytes.NewBufferString(`{"currentPassword":"wrong","attributes":{"password":"n3wP@ssword!"}}`)) | ||
| req = req.WithContext(security.WithSecurityContextTest(req.Context(), authCtx)) | ||
| rr := httptest.NewRecorder() | ||
|
|
||
| handler.HandleSelfUserCredentialUpdateRequest(rr, req) | ||
|
|
||
| // 403 is what the SDK maps onto the current-password field, distinct from 401's | ||
| // "re-authenticate" meaning, so the status matters. | ||
| require.Equal(t, http.StatusForbidden, rr.Code) | ||
|
|
||
| var errResp apierror.ErrorResponse | ||
| require.NoError(t, json.NewDecoder(rr.Body).Decode(&errResp)) | ||
| require.Equal(t, ErrorInvalidCurrentPassword.Code, errResp.Code) | ||
| } | ||
|
|
||
| // Guards the console's admin reset flow: it must keep calling UpdateUserCredentials, never the | ||
| // verifying self variant, since an admin can't know the target user's current password. | ||
| func TestHandleUserCredentialUpdateRequest_DoesNotRequireCurrentPassword(t *testing.T) { | ||
| userID := testUserID789 | ||
|
|
||
| mockSvc := NewUserServiceInterfaceMock(t) | ||
| credentialsJSON := json.RawMessage(`{"password":"admin-reset-password"}`) | ||
| mockSvc.On("UpdateUserCredentials", mock.Anything, userID, credentialsJSON).Return(nil) | ||
|
|
||
| handler := newUserHandler(mockSvc) | ||
| req := httptest.NewRequest(http.MethodPost, "/users/"+userID+"/update-credentials", | ||
| bytes.NewBufferString(`{"credentials":{"password":"admin-reset-password"}}`)) | ||
| req.SetPathValue("id", userID) | ||
| rr := httptest.NewRecorder() | ||
|
|
||
| handler.HandleUserCredentialUpdateRequest(rr, req) | ||
|
|
||
| require.Equal(t, http.StatusNoContent, rr.Code) | ||
| mockSvc.AssertNumberOfCalls(t, "UpdateUserCredentials", 1) | ||
| mockSvc.AssertNotCalled(t, "UpdateSelfUserPassword", | ||
| mock.Anything, mock.Anything, mock.Anything, mock.Anything) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.