Skip to content

Commit 9466f74

Browse files
EmyrkEmyrk
committed
chore: refactor user subject logic to be in 1 place
1 parent 0213df9 commit 9466f74

File tree

4 files changed

+91
-85
lines changed

4 files changed

+91
-85
lines changed

‎coderd/httpmw/apikey.go

Lines changed: 27 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -406,16 +406,15 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
406406
// If the key is valid, we also fetch the user roles and status.
407407
// The roles are used for RBAC authorize checks, and the status
408408
// is to block 'suspended' users from accessing the platform.
409-
//nolint:gocritic // system needs to update user roles
410-
roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), key.UserID)
409+
actor, userStatus, err := UserRBACSubject(ctx, cfg.DB, key.UserID, rbac.ScopeName(key.Scope))
411410
if err != nil {
412411
return write(http.StatusUnauthorized, codersdk.Response{
413412
Message: internalErrorMessage,
414413
Detail: fmt.Sprintf("Internal error fetching user's roles. %s", err.Error()),
415414
})
416415
}
417416

418-
if roles.Status == database.UserStatusDormant {
417+
if userStatus == database.UserStatusDormant {
419418
// If coder confirms that the dormant user is valid, it can switch their account to active.
420419
// nolint:gocritic
421420
u, err := cfg.DB.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{
@@ -429,47 +428,50 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
429428
Detail: fmt.Sprintf("can't activate a dormant user: %s", err.Error()),
430429
})
431430
}
432-
roles.Status = u.Status
431+
userStatus = u.Status
433432
}
434433

435-
if roles.Status != database.UserStatusActive {
434+
if userStatus != database.UserStatusActive {
436435
return write(http.StatusUnauthorized, codersdk.Response{
437-
Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", roles.Status),
436+
Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", userStatus),
438437
})
439438
}
440439

440+
if cfg.PostAuthAdditionalHeadersFunc != nil {
441+
cfg.PostAuthAdditionalHeadersFunc(actor, rw.Header())
442+
}
443+
444+
return key, &actor, true
445+
}
446+
447+
// UserRBACSubject fetches a user's rbac.Subject from the database. It pulls all roles from both
448+
// site and organization scopes. It also pulls the groups, and the user's status.
449+
func UserRBACSubject(ctx context.Context, db database.Store, userID uuid.UUID, scope rbac.ExpandableScope) (rbac.Subject, database.UserStatus, error) {
450+
//nolint:gocritic // system needs to update user roles
451+
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), userID)
452+
if err != nil {
453+
return rbac.Subject{}, "", xerrors.Errorf("get authorization user roles: %w", err)
454+
}
455+
441456
roleNames, err := roles.RoleNames()
442457
if err != nil {
443-
return write(http.StatusInternalServerError, codersdk.Response{
444-
Message: "Internal Server Error",
445-
Detail: err.Error(),
446-
})
458+
return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err)
447459
}
448460

449461
//nolint:gocritic // Permission to lookup custom roles the user has assigned.
450-
rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), cfg.DB, roleNames)
462+
rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), db, roleNames)
451463
if err != nil {
452-
return write(http.StatusInternalServerError, codersdk.Response{
453-
Message: "Failed to expand authenticated user roles",
454-
Detail: err.Error(),
455-
Validations: nil,
456-
})
464+
return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err)
457465
}
458466

459-
// Actor is the user's authorization context.
460467
actor := rbac.Subject{
461468
FriendlyName: roles.Username,
462-
ID: key.UserID.String(),
469+
ID: userID.String(),
463470
Roles: rbacRoles,
464471
Groups: roles.Groups,
465-
Scope: rbac.ScopeName(key.Scope),
472+
Scope: scope,
466473
}.WithCachedASTValue()
467-
468-
if cfg.PostAuthAdditionalHeadersFunc != nil {
469-
cfg.PostAuthAdditionalHeadersFunc(actor, rw.Header())
470-
}
471-
472-
return key, &actor, true
474+
return actor, roles.Status, nil
473475
}
474476

475477
// APITokenFromRequest returns the api token from the request.

‎coderd/identityprovider/tokens.go

Lines changed: 6 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -209,27 +209,14 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
209209
}
210210

211211
// Grab the user roles so we can perform the exchange as the user.
212-
//nolint:gocritic // In the token exchange, there is no user actor.
213-
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), dbCode.UserID)
212+
actor, _, err := httpmw.UserRBACSubject(ctx, db, dbCode.UserID, rbac.ScopeAll)
214213
if err != nil {
215-
return oauth2.Token{}, err
216-
}
217-
218-
roleNames, err := roles.RoleNames()
219-
if err != nil {
220-
return oauth2.Token{}, xerrors.Errorf("role names: %w", err)
221-
}
222-
223-
userSubj := rbac.Subject{
224-
ID: dbCode.UserID.String(),
225-
Roles: rbac.RoleIdentifiers(roleNames),
226-
Groups: roles.Groups,
227-
Scope: rbac.ScopeAll,
214+
return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err)
228215
}
229216

230217
// Do the actual token exchange in the database.
231218
err = db.InTx(func(tx database.Store) error {
232-
ctx := dbauthz.As(ctx, userSubj)
219+
ctx := dbauthz.As(ctx, actor)
233220
err = tx.DeleteOAuth2ProviderAppCodeByID(ctx, dbCode.ID)
234221
if err != nil {
235222
return xerrors.Errorf("delete oauth2 app code: %w", err)
@@ -311,22 +298,10 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
311298
if err != nil {
312299
return oauth2.Token{}, err
313300
}
314-
//nolint:gocritic // There is no user yet so we must use the system.
315-
roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), prevKey.UserID)
316-
if err != nil {
317-
return oauth2.Token{}, err
318-
}
319301

320-
roleNames, err := roles.RoleNames()
302+
actor, _, err := httpmw.UserRBACSubject(ctx, db, prevKey.UserID, rbac.ScopeAll)
321303
if err != nil {
322-
return oauth2.Token{}, xerrors.Errorf("role names: %w", err)
323-
}
324-
325-
userSubj := rbac.Subject{
326-
ID: prevKey.UserID.String(),
327-
Roles: rbac.RoleIdentifiers(roleNames),
328-
Groups: roles.Groups,
329-
Scope: rbac.ScopeAll,
304+
return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err)
330305
}
331306

332307
// Generate a new refresh token.
@@ -351,7 +326,7 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
351326

352327
// Replace the token.
353328
err = db.InTx(func(tx database.Store) error {
354-
ctx := dbauthz.As(ctx, userSubj)
329+
ctx := dbauthz.As(ctx, actor)
355330
err = tx.DeleteAPIKeyByID(ctx, prevKey.ID) // This cascades to the token.
356331
if err != nil {
357332
return xerrors.Errorf("delete oauth2 app token: %w", err)

‎coderd/userauth.go

Lines changed: 15 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
231231
return
232232
}
233233

234-
user, roles, ok := api.loginRequest(ctx, rw, loginWithPassword)
234+
user, actor, ok := api.loginRequest(ctx, rw, loginWithPassword)
235235
// 'user.ID' will be empty, or will be an actual value. Either is correct
236236
// here.
237237
aReq.UserID = user.ID
@@ -240,21 +240,8 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
240240
return
241241
}
242242

243-
roleNames, err := roles.RoleNames()
244-
if err != nil {
245-
httpapi.InternalServerError(rw, err)
246-
return
247-
}
248-
249-
userSubj := rbac.Subject{
250-
ID: user.ID.String(),
251-
Roles: rbac.RoleIdentifiers(roleNames),
252-
Groups: roles.Groups,
253-
Scope: rbac.ScopeAll,
254-
}
255-
256243
//nolint:gocritic // Creating the API key as the user instead of as system.
257-
cookie, key, err := api.createAPIKey(dbauthz.As(ctx, userSubj), apikey.CreateParams{
244+
cookie, key, err := api.createAPIKey(dbauthz.As(ctx, actor), apikey.CreateParams{
258245
UserID: user.ID,
259246
LoginType: database.LoginTypePassword,
260247
RemoteAddr: r.RemoteAddr,
@@ -284,7 +271,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
284271
//
285272
// The user struct is always returned, even if authentication failed. This is
286273
// to support knowing what user attempted to login.
287-
func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, database.GetAuthorizationUserRolesRow, bool) {
274+
func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, rbac.Subject, bool) {
288275
logger := api.Logger.Named(userAuthLoggerName)
289276

290277
//nolint:gocritic // In order to login, we need to get the user first!
@@ -296,7 +283,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
296283
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
297284
Message: "Internal error.",
298285
})
299-
return user, database.GetAuthorizationUserRolesRow{}, false
286+
return user, rbac.Subject{}, false
300287
}
301288

302289
// If the user doesn't exist, it will be a default struct.
@@ -306,7 +293,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
306293
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
307294
Message: "Internal error.",
308295
})
309-
return user, database.GetAuthorizationUserRolesRow{}, false
296+
return user, rbac.Subject{}, false
310297
}
311298

312299
if !equal {
@@ -315,7 +302,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
315302
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
316303
Message: "Incorrect email or password.",
317304
})
318-
return user, database.GetAuthorizationUserRolesRow{}, false
305+
return user, rbac.Subject{}, false
319306
}
320307

321308
// If password authentication is disabled and the user does not have the
@@ -324,14 +311,14 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
324311
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
325312
Message: "Password authentication is disabled.",
326313
})
327-
return user, database.GetAuthorizationUserRolesRow{}, false
314+
return user, rbac.Subject{}, false
328315
}
329316

330317
if user.LoginType != database.LoginTypePassword {
331318
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
332319
Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType),
333320
})
334-
return user, database.GetAuthorizationUserRolesRow{}, false
321+
return user, rbac.Subject{}, false
335322
}
336323

337324
if user.Status == database.UserStatusDormant {
@@ -346,29 +333,28 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
346333
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
347334
Message: "Internal error occurred. Try again later, or contact an admin for assistance.",
348335
})
349-
return user, database.GetAuthorizationUserRolesRow{}, false
336+
return user, rbac.Subject{}, false
350337
}
351338
}
352339

353-
//nolint:gocritic // System needs to fetch user roles in order to login user.
354-
roles, err := api.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID)
340+
subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll)
355341
if err != nil {
356342
logger.Error(ctx, "unable to fetch authorization user roles", slog.Error(err))
357343
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
358344
Message: "Internal error.",
359345
})
360-
return user, database.GetAuthorizationUserRolesRow{}, false
346+
return user, rbac.Subject{}, false
361347
}
362348

363349
// If the user logged into a suspended account, reject the login request.
364-
if roles.Status != database.UserStatusActive {
350+
if userStatus != database.UserStatusActive {
365351
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
366-
Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", roles.Status),
352+
Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", userStatus),
367353
})
368-
return user, database.GetAuthorizationUserRolesRow{}, false
354+
return user, rbac.Subject{}, false
369355
}
370356

371-
return user, roles, true
357+
return user, subject, true
372358
}
373359

374360
// Clear the user's session cookie.

‎enterprise/coderd/userauth_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -683,6 +683,49 @@ func TestGroupSync(t *testing.T) {
683683
}
684684
}
685685

686+
func TestUserLogin(t *testing.T) {
687+
t.Run("CustomRole", func(t *testing.T) {
688+
t.Parallel()
689+
dv := coderdtest.DeploymentValues(t)
690+
dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
691+
ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{
692+
Options: &coderdtest.Options{
693+
DeploymentValues: dv,
694+
},
695+
LicenseOptions: &coderdenttest.LicenseOptions{
696+
Features: license.Features{
697+
codersdk.FeatureCustomRoles: 1,
698+
},
699+
},
700+
})
701+
702+
ctx := testutil.Context(t, testutil.WaitShort)
703+
//nolint:gocritic // owner required
704+
customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
705+
Name: "custom-role",
706+
OrganizationID: owner.OrganizationID.String(),
707+
OrganizationPermissions: []codersdk.Permission{},
708+
})
709+
require.NoError(t, err, "create custom role")
710+
711+
anotherClient, anotherUser := coderdtest.CreateAnotherUserMutators(t, ownerClient, owner.OrganizationID, []rbac.RoleIdentifier{
712+
{
713+
Name: customRole.Name,
714+
OrganizationID: owner.OrganizationID,
715+
},
716+
}, func(r *codersdk.CreateUserRequest) {
717+
r.Password = ""
718+
r.UserLoginType = codersdk.LoginTypeNone
719+
})
720+
721+
_, err = anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{
722+
Email: anotherUser.Email,
723+
Password: "SomeSecurePassword!",
724+
})
725+
require.Error(t, err)
726+
})
727+
}
728+
686729
// oidcTestRunner is just a helper to setup and run oidc tests.
687730
// An actual Coderd instance is used to run the tests.
688731
type oidcTestRunner struct {

0 commit comments

Comments
 (0)