fix(coderd): allow user-admin password resets to succeed - #26537
Conversation
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
Split generated token values before calling APIKeyByID, and confirm the keys resolve before password reset so later 404s prove the records were removed.
jscottmiller
left a comment
jscottmiller
left a comment
There was a problem hiding this comment.
I don't have much wisdom on our auth code, but the approach seems reasonable. Initially I was surprised that the "user admin" role wasn't expanded, but it seems like we lack the granularity to restrict the permission to keys owned by actual users (vs. system accounts), plus granting that priv generally means that a user admin can delete individual keys outside of a reset (perhaps good, perhaps bad, but somewhat out of scope).
I'm curious how someone more familiar with the AsXXX methods and their intended use cases see this change. Being new, it's hard to see if this pattern is working as intended or if we are hacking around a rigid core.
Fair. I don't see a lot of AsXXX helpers in the code either. To me it made sense to introduce one here since it's already the second time we need it (and the use of |
|
Makes sense, and it's probably the best path. And definitely better than |
jscottmiller
left a comment
jscottmiller
left a comment
There was a problem hiding this comment.
lgtm; I'm curious if @Emyrk has any auth-thoughts.
3 participants
Problem
Resetting a password revokes all of the target user's API keys via
DeleteAPIKeysByUserID, whosedbauthzcheck requiresapi_key:deleteon those keys. Theuser-adminrole has full access toResourceUserbut no permissions onResourceApiKey, so the revocation failed authorization inside the transaction and surfaced as a 500. Owners worked only because they hold wildcard permissions.Fix
Add a narrowly-scoped
AsAPIKeyRevokerdbauthzactor that can delete only the API keys owned by a specific user, and nothing else. Use it for the key revocation in both the admin password-reset path (putUserPassword) and the one-time-passcode reset path, which previously leaned on the much broaderAsSystemRestricted.Refs: https://linear.app/codercom/issue/PLAT-316