From ba094c5706c0a87bcedf84cd9b50e313f19173d0 Mon Sep 17 00:00:00 2001 From: George K Date: Tue, 7 Jul 2026 09:05:14 -0700 Subject: [PATCH] fix(coderd): allow user-admin password resets to succeed (#26537) User Admin password resets could update the target user's hashed password but fail while revoking that user's API keys. The transaction then rolled back and returned HTTP 500, so the password was never changed. Add a user-scoped API key revoker actor and use it in both password reset flows so key revocation succeeds without broader system auth. Refs: https://linear.app/codercom/issue/PLAT-316 --- coderd/database/dbauthz/dbauthz.go | 26 ++++++++++ coderd/database/dbauthz/dbauthz_test.go | 33 +++++++++++++ coderd/rbac/authz.go | 1 + coderd/userauth.go | 5 +- coderd/userauth_test.go | 16 +++++++ coderd/users.go | 4 +- coderd/users_test.go | 64 +++++++++++++++++++++++++ 7 files changed, 146 insertions(+), 3 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 15c97794e8..8d71522333 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -453,6 +453,26 @@ var ( }.WithCachedASTValue() } + subjectAPIKeyRevoker = func(userID uuid.UUID) rbac.Subject { + return rbac.Subject{ + Type: rbac.SubjectTypeAPIKeyRevoker, + FriendlyName: "API Key Revoker", + ID: userID.String(), + Roles: rbac.Roles([]rbac.Role{ + { + Identifier: rbac.RoleIdentifier{Name: "apikeyrevoker"}, + DisplayName: "API Key Revoker", + Site: []rbac.Permission{}, + User: rbac.Permissions(map[string][]policy.Action{ + rbac.ResourceApiKey.Type: {policy.ActionDelete}, + }), + ByOrgID: map[string]rbac.OrgPermissions{}, + }, + }), + Scope: rbac.ScopeAll, + }.WithCachedASTValue() + } + subjectSystemRestricted = rbac.Subject{ Type: rbac.SubjectTypeSystemRestricted, FriendlyName: "System", @@ -845,6 +865,12 @@ func AsSubAgentAPI(ctx context.Context, orgID uuid.UUID, userID uuid.UUID) conte return As(ctx, subjectSubAgentAPI(userID, orgID)) } +// AsAPIKeyRevoker returns a context with an actor that can revoke API +// keys owned by the specified user, and nothing else. +func AsAPIKeyRevoker(ctx context.Context, userID uuid.UUID) context.Context { + return As(ctx, subjectAPIKeyRevoker(userID)) +} + // AsSystemRestricted returns a context with an actor that has permissions // required for various system operations (login, logout, metrics cache). // DO NOT USE THIS UNLESS YOU HAVE ABSOLUTELY NO OTHER CHOICE. Prefer using a diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 1696b3f3c4..2128059686 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -7259,6 +7259,39 @@ func TestAuthorizeProvisionerJob_SystemFastPath(t *testing.T) { }) } +func TestAsAPIKeyRevoker(t *testing.T) { + t.Parallel() + + userID := uuid.New() + otherUserID := uuid.New() + ctx := dbauthz.AsAPIKeyRevoker(context.Background(), userID) + actor, ok := dbauthz.ActorFromContext(ctx) + require.True(t, ok, "actor must be present") + + auth := rbac.NewStrictCachingAuthorizer(prometheus.NewRegistry()) + + t.Run("OwnedAPIKeys", func(t *testing.T) { + t.Parallel() + + resource := rbac.ResourceApiKey.WithOwner(userID.String()) + for _, action := range rbac.ResourceApiKey.AvailableActions() { + err := auth.Authorize(ctx, actor, action, resource) + if action == policy.ActionDelete { + require.NoError(t, err, "owned api keys should allow %s", action) + continue + } + require.Error(t, err, "owned api keys should deny %s", action) + } + }) + + t.Run("OtherUsersAPIKeys", func(t *testing.T) { + t.Parallel() + + err := auth.Authorize(ctx, actor, policy.ActionDelete, rbac.ResourceApiKey.WithOwner(otherUserID.String())) + require.Error(t, err, "other users' api keys should not be deletable") + }) +} + func TestAsChatd(t *testing.T) { t.Parallel() diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 4b253bc10d..0be0138494 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -75,6 +75,7 @@ const ( SubjectTypeSystemReadProvisionerDaemons SubjectType = "system_read_provisioner_daemons" SubjectTypeSystemRestricted SubjectType = "system_restricted" SubjectTypeSystemOAuth SubjectType = "system_oauth" + SubjectTypeAPIKeyRevoker SubjectType = "api_key_revoker" // #nosec G101, not a credential. SubjectTypeNotifier SubjectType = "notifier" SubjectTypeSubAgentAPI SubjectType = "sub_agent_api" SubjectTypeFileReader SubjectType = "file_reader" diff --git a/coderd/userauth.go b/coderd/userauth.go index 3e85536d21..4babef1be8 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -430,8 +430,9 @@ func (api *API) postChangePasswordWithOneTimePasscode(rw http.ResponseWriter, r return xerrors.Errorf("update user hashed password: %w", err) } - //nolint:gocritic // We need the system auth context to be able to delete all API keys for the user. - err = tx.DeleteAPIKeysByUserID(dbauthz.AsSystemRestricted(ctx), user.ID) + //nolint:gocritic // Password resets must revoke all keys owned by the + // target user, not just keys addressable by the caller's actor. + err = tx.DeleteAPIKeysByUserID(dbauthz.AsAPIKeyRevoker(ctx, user.ID), user.ID) if err != nil { logger.Error(ctx, "unable to delete user's api keys", slog.Error(err)) return xerrors.Errorf("delete api keys for user: %w", err) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 8e4c230c4f..463ce83651 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -2850,11 +2850,27 @@ func TestUserForgotPassword(t *testing.T) { // as we haven't change the password yet. requireCannotLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) + // Create an API token to confirm the password reset revokes the + // user's existing keys. + token, tokenErr := anotherClient.CreateToken(ctx, codersdk.Me, codersdk.CreateTokenRequest{}) + require.NoError(t, tokenErr) + + tokenClient := codersdk.New(client.URL, codersdk.WithSessionToken(token.Key)) + + _, tokenErr = tokenClient.User(ctx, codersdk.Me) + require.NoError(t, tokenErr, "token should authenticate before the password reset") + oneTimePasscode := requireRequestOneTimePasscode(t, ctx, anotherClient, notifyEnq, anotherUser.Email, anotherUser.ID) requireChangePasswordWithOneTimePasscode(t, ctx, anotherClient, anotherUser.Email, oneTimePasscode, newPassword) requireCanLogin(t, ctx, anotherClient, anotherUser.Email, newPassword) + // The password reset must revoke every API key owned by the user. + _, tokenErr = tokenClient.User(ctx, codersdk.Me) + var tokenAPIErr *codersdk.Error + require.ErrorAs(t, tokenErr, &tokenAPIErr) + require.Equal(t, http.StatusUnauthorized, tokenAPIErr.StatusCode()) + // We now need to check that the one-time passcode isn't valid. err := anotherClient.ChangePasswordWithOneTimePasscode(ctx, codersdk.ChangePasswordWithOneTimePasscodeRequest{ Email: anotherUser.Email, diff --git a/coderd/users.go b/coderd/users.go index d067a63734..77c6954c6c 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1713,7 +1713,9 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { return xerrors.Errorf("update user hashed password: %w", err) } - err = tx.DeleteAPIKeysByUserID(ctx, user.ID) + //nolint:gocritic // Password resets must revoke all keys owned by the + // target user, not just keys addressable by the caller's actor. + err = tx.DeleteAPIKeysByUserID(dbauthz.AsAPIKeyRevoker(ctx, user.ID), user.ID) if err != nil { return xerrors.Errorf("delete api keys by user ID: %w", err) } diff --git a/coderd/users_test.go b/coderd/users_test.go index 2893c7126b..bf35cf1d7f 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1442,6 +1442,70 @@ func TestUpdateUserPassword(t *testing.T) { require.NoError(t, err, "member should login successfully with the new password") }) + t.Run("UserAdminCanUpdateMemberPassword", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + owner := coderdtest.CreateFirstUser(t, client) + + userAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleUserAdmin()) + memberClient, member := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + apikey1, err := memberClient.CreateToken(ctx, codersdk.Me, codersdk.CreateTokenRequest{}) + require.NoError(t, err) + apikey1ID, _, ok := strings.Cut(apikey1.Key, "-") + require.True(t, ok) + + apikey2, err := memberClient.CreateToken(ctx, codersdk.Me, codersdk.CreateTokenRequest{}) + require.NoError(t, err) + apikey2ID, _, ok := strings.Cut(apikey2.Key, "-") + require.True(t, ok) + + // Confirm the token IDs resolve before the reset, so later + // 404s prove password reset removed them. + _, err = memberClient.APIKeyByID(ctx, member.ID.String(), apikey1ID) + require.NoError(t, err) + _, err = memberClient.APIKeyByID(ctx, member.ID.String(), apikey2ID) + require.NoError(t, err) + + err = userAdmin.UpdateUserPassword(ctx, member.ID.String(), codersdk.UpdateUserPasswordRequest{ + Password: "SomeNewStrongPassword!", + }) + require.NoError(t, err, "user-admin should be able to update member password") + + // The old session token was deleted with the password reset, so the + // member client is unauthorized before it logs back in. + _, err = memberClient.APIKeyByID(ctx, member.ID.String(), apikey1ID) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusUnauthorized, cerr.StatusCode()) + + resp, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: member.Email, + Password: "SomeNewStrongPassword!", + }) + require.NoError(t, err, "member should login successfully with the new password") + + memberClient = codersdk.New( + memberClient.URL, + codersdk.WithSessionToken(resp.SessionToken), + codersdk.WithHTTPClient(coderdtest.NewIsolatedHTTPClient(memberClient.URL)), + ) + + // After re-authentication, the old API keys should be gone from storage. + _, err = memberClient.APIKeyByID(ctx, member.ID.String(), apikey1ID) + require.Error(t, err) + cerr = coderdtest.SDKError(t, err) + require.Equal(t, http.StatusNotFound, cerr.StatusCode()) + + _, err = memberClient.APIKeyByID(ctx, member.ID.String(), apikey2ID) + require.Error(t, err) + cerr = coderdtest.SDKError(t, err) + require.Equal(t, http.StatusNotFound, cerr.StatusCode()) + }) + t.Run("AuditorCantUpdateOtherUserPassword", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil)