hub: re-evaluate admin role on every login and token refresh#363
Conversation
Previously admin status was only fully evaluated when a user record was first created. The API login path (provisionUser) had promotion-only logic, and the web OAuth callback had no re-evaluation at all. Token refresh also preserved the original role from the JWT claims. Now all three paths call determineUserRole / getUserRole on every invocation, supporting both promotion and demotion when the admin emails configuration changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic re-evaluation of user roles (both promotion and demotion) during login and token refresh, along with a test verifying admin demotion. Feedback on these changes highlights two critical security gaps: the OAuth callback handler does not reject suspended users during login, and the token refresh handler fails to verify if a user has been suspended or deleted. Additionally, a background token refresh middleware bypasses these new role and status checks entirely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Re-evaluate admin status on every login | ||
| newRole := determineUserRole(userInfo.Email, ws.config.AdminEmails) | ||
| if user.Role != newRole { | ||
| slog.Info("User role changed on login", "email", userInfo.Email, "old_role", user.Role, "new_role", newRole) | ||
| user.Role = newRole | ||
| } |
There was a problem hiding this comment.
The Web OAuth callback path (handleOAuthCallback) is missing a check to reject suspended users when an existing user logs in. Without this check, a suspended user can still successfully log in via the Web OAuth flow.
We should reject suspended users here, matching the behavior in provisionUser (in pkg/hub/handlers_auth.go).
// Reject suspended users
if user.Status == "suspended" {
slog.Warn("login rejected: user is suspended", "email", userInfo.Email, "user_id", user.ID)
http.Redirect(w, r, "/login?error=user_suspended", http.StatusFound)
return
}
// Re-evaluate admin status on every login
newRole := determineUserRole(userInfo.Email, ws.config.AdminEmails)
if user.Role != newRole {
slog.Info("User role changed on login", "email", userInfo.Email, "old_role", user.Role, "new_role", newRole)
user.Role = newRole
}| // Re-evaluate admin status on token refresh | ||
| role := claims.Role | ||
| if newRole := s.getUserRole(claims.Email); role != newRole { | ||
| slog.Info("User role changed on token refresh", "email", claims.Email, "old_role", role, "new_role", newRole) | ||
| role = newRole | ||
| // Persist the role change | ||
| if user, err := s.store.GetUserByEmail(r.Context(), claims.Email); err == nil && user.Role != role { | ||
| user.Role = role | ||
| _ = s.store.UpdateUser(r.Context(), user) | ||
| } | ||
| } |
There was a problem hiding this comment.
On token refresh, we should always load the user from the database to verify they still exist and are not suspended. Currently, if a user is suspended or deleted, they can still successfully refresh their token and obtain a new access token as long as their refresh token is valid.
Additionally, please note that the web server's background token refresh path in sessionToBearerMiddleware (in pkg/hub/web.go) still calls ws.userTokenSvc.RefreshTokens(refreshToken) directly, which completely bypasses this new role re-evaluation and status check logic. We should refactor both paths to use a shared helper or ensure the web middleware also performs these checks.
| // Re-evaluate admin status on token refresh | |
| role := claims.Role | |
| if newRole := s.getUserRole(claims.Email); role != newRole { | |
| slog.Info("User role changed on token refresh", "email", claims.Email, "old_role", role, "new_role", newRole) | |
| role = newRole | |
| // Persist the role change | |
| if user, err := s.store.GetUserByEmail(r.Context(), claims.Email); err == nil && user.Role != role { | |
| user.Role = role | |
| _ = s.store.UpdateUser(r.Context(), user) | |
| } | |
| } | |
| // Always load the user from the database to verify they still exist and are active | |
| user, err := s.store.GetUserByEmail(r.Context(), claims.Email) | |
| if err != nil { | |
| writeError(w, http.StatusUnauthorized, ErrCodeUnauthorized, "user not found", nil) | |
| return | |
| } | |
| if user.Status == "suspended" { | |
| writeError(w, http.StatusForbidden, "user_suspended", "your account has been suspended", nil) | |
| return | |
| } | |
| // Re-evaluate admin status on token refresh | |
| role := user.Role | |
| if newRole := s.getUserRole(claims.Email); role != newRole { | |
| slog.Info("User role changed on token refresh", "email", claims.Email, "old_role", role, "new_role", newRole) | |
| role = newRole | |
| user.Role = role | |
| _ = s.store.UpdateUser(r.Context(), user) | |
| } |
1 participant
Summary
provisionUser): changed promotion-only logic to full re-evaluation (supports both promotion and demotion)handleOAuthCallback): added role re-evaluation for existing users (was completely missing)handleAuthRefresh): now re-evaluates role from current admin emails config instead of preserving stale JWT claimsTest plan
TestProvisionUser/promotes_member_to_admin_when_config_changesstill passesTestProvisionUser/demotes_admin_to_member_when_removed_from_admin_emailstest added and passesTestProvisionUsersubtests pass (9/9)go build ./pkg/hub/compiles cleanly