chore: refactor user subject logic to be in 1 place

coder · Emyrk · Jun 21, 2024 · Jun 21, 2024 · Jun 21, 2024 · Jun 21, 2024
commit 9466f74865bfa8f3ae80b3937652aa2408b57f10
diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go
@@ -406,16 +406,15 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
 	// If the key is valid, we also fetch the user roles and status.
 	// The roles are used for RBAC authorize checks, and the status
 	// is to block 'suspended' users from accessing the platform.
-	//nolint:gocritic // system needs to update user roles
-	roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), key.UserID)
+	actor, userStatus, err := UserRBACSubject(ctx, cfg.DB, key.UserID, rbac.ScopeName(key.Scope))
 	if err != nil {
 		return write(http.StatusUnauthorized, codersdk.Response{
 			Message: internalErrorMessage,
 			Detail:  fmt.Sprintf("Internal error fetching user's roles. %s", err.Error()),
 		})
 	}
 
-	if roles.Status == database.UserStatusDormant {
+	if userStatus == database.UserStatusDormant {
 		// If coder confirms that the dormant user is valid, it can switch their account to active.
 		// nolint:gocritic
 		u, err := cfg.DB.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{
@@ -429,47 +428,50 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
 				Detail:  fmt.Sprintf("can't activate a dormant user: %s", err.Error()),
 			})
 		}
-		roles.Status = u.Status
+		userStatus = u.Status
 	}
 
-	if roles.Status != database.UserStatusActive {
+	if userStatus != database.UserStatusActive {
 		return write(http.StatusUnauthorized, codersdk.Response{
-			Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", roles.Status),
+			Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", userStatus),
 		})
 	}
 
+	if cfg.PostAuthAdditionalHeadersFunc != nil {
+		cfg.PostAuthAdditionalHeadersFunc(actor, rw.Header())
+	}
+
+	return key, &actor, true
+}
+
+// UserRBACSubject fetches a user's rbac.Subject from the database. It pulls all roles from both
+// site and organization scopes. It also pulls the groups, and the user's status.
+func UserRBACSubject(ctx context.Context, db database.Store, userID uuid.UUID, scope rbac.ExpandableScope) (rbac.Subject, database.UserStatus, error) {
+	//nolint:gocritic // system needs to update user roles
+	roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), userID)
+	if err != nil {
+		return rbac.Subject{}, "", xerrors.Errorf("get authorization user roles: %w", err)
+	}
+
 	roleNames, err := roles.RoleNames()
 	if err != nil {
-		return write(http.StatusInternalServerError, codersdk.Response{
-			Message: "Internal Server Error",
-			Detail:  err.Error(),
-		})
+		return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err)
 	}
 
 	//nolint:gocritic // Permission to lookup custom roles the user has assigned.
-	rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), cfg.DB, roleNames)
+	rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), db, roleNames)
 	if err != nil {
-		return write(http.StatusInternalServerError, codersdk.Response{
-			Message:     "Failed to expand authenticated user roles",
-			Detail:      err.Error(),
-			Validations: nil,
-		})
+		return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err)
 	}
 
-	// Actor is the user's authorization context.
 	actor := rbac.Subject{
 		FriendlyName: roles.Username,
-		ID:           key.UserID.String(),
+		ID:           userID.String(),
 		Roles:        rbacRoles,
 		Groups:       roles.Groups,
-		Scope:        rbac.ScopeName(key.Scope),
+		Scope:        scope,
 	}.WithCachedASTValue()
-
-	if cfg.PostAuthAdditionalHeadersFunc != nil {
-		cfg.PostAuthAdditionalHeadersFunc(actor, rw.Header())
-	}
-
-	return key, &actor, true
+	return actor, roles.Status, nil
 }
 
 // APITokenFromRequest returns the api token from the request.

diff --git a/coderd/identityprovider/tokens.go b/coderd/identityprovider/tokens.go
@@ -209,27 +209,14 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
 	}
 
 	// Grab the user roles so we can perform the exchange as the user.
-	//nolint:gocritic // In the token exchange, there is no user actor.
-	roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), dbCode.UserID)
+	actor, _, err := httpmw.UserRBACSubject(ctx, db, dbCode.UserID, rbac.ScopeAll)
 	if err != nil {
-		return oauth2.Token{}, err
-	}
-
-	roleNames, err := roles.RoleNames()
-	if err != nil {
-		return oauth2.Token{}, xerrors.Errorf("role names: %w", err)
-	}
-
-	userSubj := rbac.Subject{
-		ID:     dbCode.UserID.String(),
-		Roles:  rbac.RoleIdentifiers(roleNames),
-		Groups: roles.Groups,
-		Scope:  rbac.ScopeAll,
+		return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err)
 	}
 
 	// Do the actual token exchange in the database.
 	err = db.InTx(func(tx database.Store) error {
-		ctx := dbauthz.As(ctx, userSubj)
+		ctx := dbauthz.As(ctx, actor)
 		err = tx.DeleteOAuth2ProviderAppCodeByID(ctx, dbCode.ID)
 		if err != nil {
 			return xerrors.Errorf("delete oauth2 app code: %w", err)
@@ -311,22 +298,10 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
 	if err != nil {
 		return oauth2.Token{}, err
 	}
-	//nolint:gocritic // There is no user yet so we must use the system.
-	roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), prevKey.UserID)
-	if err != nil {
-		return oauth2.Token{}, err
-	}
 
-	roleNames, err := roles.RoleNames()
+	actor, _, err := httpmw.UserRBACSubject(ctx, db, prevKey.UserID, rbac.ScopeAll)
 	if err != nil {
-		return oauth2.Token{}, xerrors.Errorf("role names: %w", err)
-	}
-
-	userSubj := rbac.Subject{
-		ID:     prevKey.UserID.String(),
-		Roles:  rbac.RoleIdentifiers(roleNames),
-		Groups: roles.Groups,
-		Scope:  rbac.ScopeAll,
+		return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err)
 	}
 
 	// Generate a new refresh token.
@@ -351,7 +326,7 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
 
 	// Replace the token.
 	err = db.InTx(func(tx database.Store) error {
-		ctx := dbauthz.As(ctx, userSubj)
+		ctx := dbauthz.As(ctx, actor)
 		err = tx.DeleteAPIKeyByID(ctx, prevKey.ID) // This cascades to the token.
 		if err != nil {
 			return xerrors.Errorf("delete oauth2 app token: %w", err)

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -231,7 +231,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	user, roles, ok := api.loginRequest(ctx, rw, loginWithPassword)
+	user, actor, ok := api.loginRequest(ctx, rw, loginWithPassword)
 	// 'user.ID' will be empty, or will be an actual value. Either is correct
 	// here.
 	aReq.UserID = user.ID
@@ -240,21 +240,8 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	roleNames, err := roles.RoleNames()
-	if err != nil {
-		httpapi.InternalServerError(rw, err)
-		return
-	}
-
-	userSubj := rbac.Subject{
-		ID:     user.ID.String(),
-		Roles:  rbac.RoleIdentifiers(roleNames),
-		Groups: roles.Groups,
-		Scope:  rbac.ScopeAll,
-	}
-
 	//nolint:gocritic // Creating the API key as the user instead of as system.
-	cookie, key, err := api.createAPIKey(dbauthz.As(ctx, userSubj), apikey.CreateParams{
+	cookie, key, err := api.createAPIKey(dbauthz.As(ctx, actor), apikey.CreateParams{
 		UserID:          user.ID,
 		LoginType:       database.LoginTypePassword,
 		RemoteAddr:      r.RemoteAddr,
@@ -284,7 +271,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
 //
 // The user struct is always returned, even if authentication failed. This is
 // to support knowing what user attempted to login.
-func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, database.GetAuthorizationUserRolesRow, bool) {
+func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, rbac.Subject, bool) {
 	logger := api.Logger.Named(userAuthLoggerName)
 
 	//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
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Internal error.",
 		})
-		return user, database.GetAuthorizationUserRolesRow{}, false
+		return user, rbac.Subject{}, false
 	}
 
 	// 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
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Internal error.",
 		})
-		return user, database.GetAuthorizationUserRolesRow{}, false
+		return user, rbac.Subject{}, false
 	}
 
 	if !equal {
@@ -315,7 +302,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
 		httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
 			Message: "Incorrect email or password.",
 		})
-		return user, database.GetAuthorizationUserRolesRow{}, false
+		return user, rbac.Subject{}, false
 	}
 
 	// 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
 		httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
 			Message: "Password authentication is disabled.",
 		})
-		return user, database.GetAuthorizationUserRolesRow{}, false
+		return user, rbac.Subject{}, false
 	}
 
 	if user.LoginType != database.LoginTypePassword {
 		httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
 			Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType),
 		})
-		return user, database.GetAuthorizationUserRolesRow{}, false
+		return user, rbac.Subject{}, false
 	}
 
 	if user.Status == database.UserStatusDormant {
@@ -346,29 +333,28 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co
 			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 				Message: "Internal error occurred. Try again later, or contact an admin for assistance.",
 			})
-			return user, database.GetAuthorizationUserRolesRow{}, false
+			return user, rbac.Subject{}, false
 		}
 	}
 
-	//nolint:gocritic // System needs to fetch user roles in order to login user.
-	roles, err := api.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID)
+	subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll)
 	if err != nil {
 		logger.Error(ctx, "unable to fetch authorization user roles", slog.Error(err))
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Internal error.",
 		})
-		return user, database.GetAuthorizationUserRolesRow{}, false
+		return user, rbac.Subject{}, false
 	}
 
 	// If the user logged into a suspended account, reject the login request.
-	if roles.Status != database.UserStatusActive {
+	if userStatus != database.UserStatusActive {
 		httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
-			Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", roles.Status),
+			Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", userStatus),
 		})
-		return user, database.GetAuthorizationUserRolesRow{}, false
+		return user, rbac.Subject{}, false
 	}
 
-	return user, roles, true
+	return user, subject, true
 }
 
 // Clear the user's session cookie.

diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go
@@ -683,6 +683,49 @@
 	}
 }
 
+func TestUserLogin(t *testing.T) {
+	t.Run("CustomRole", func(t *testing.T) {
+		t.Parallel()
+		dv := coderdtest.DeploymentValues(t)
+		dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)}
+		ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{
+			Options: &coderdtest.Options{
+				DeploymentValues: dv,
+			},
+			LicenseOptions: &coderdenttest.LicenseOptions{
+				Features: license.Features{
+					codersdk.FeatureCustomRoles: 1,
+				},
+			},
+		})
+
+		ctx := testutil.Context(t, testutil.WaitShort)
+		//nolint:gocritic // owner required
+		customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{
+			Name:                    "custom-role",
+			OrganizationID:          owner.OrganizationID.String(),
+			OrganizationPermissions: []codersdk.Permission{},
+		})
+		require.NoError(t, err, "create custom role")
+
+		anotherClient, anotherUser := coderdtest.CreateAnotherUserMutators(t, ownerClient, owner.OrganizationID, []rbac.RoleIdentifier{
+			{
+				Name:           customRole.Name,
+				OrganizationID: owner.OrganizationID,
+			},
+		}, func(r *codersdk.CreateUserRequest) {
+			r.Password = ""
+			r.UserLoginType = codersdk.LoginTypeNone
+		})
+
+		_, err = anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{
+			Email:    anotherUser.Email,
+			Password: "SomeSecurePassword!",
+		})
+		require.Error(t, err)
+	})
+}
+
 // oidcTestRunner is just a helper to setup and run oidc tests.
 // An actual Coderd instance is used to run the tests.
 type oidcTestRunner struct {