feat: Support dynamic updating of users' groupings based on Oauth inf… - #4088
Closed
vinson-zhang wants to merge 3 commits into
Closed
feat: Support dynamic updating of users' groupings based on Oauth inf…#4088vinson-zhang wants to merge 3 commits into
vinson-zhang wants to merge 3 commits into
Conversation
Contributor
WalkthroughThis pull request adds OAuth group field extraction and automatic user group synchronization. It introduces a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant OAuthProvider as OAuth Provider
participant ExtractLogic as Group Extraction
participant Validation as Ratio Setting
participant UserDB as User Database
rect rgba(100, 150, 200, 0.5)
Note over OAuthProvider,UserDB: Existing User Flow
OAuthProvider->>ExtractLogic: User Info + GroupField path
ExtractLogic->>ExtractLogic: Extract group from JSON
ExtractLogic->>Validation: Check if group exists in config
alt Group valid & differs from current
Validation->>UserDB: Update user.Group
UserDB-->>UserDB: Persist changes
else Group invalid or matches current
Validation-->>ExtractLogic: Skip update (log reason)
end
end
rect rgba(150, 100, 200, 0.5)
Note over OAuthProvider,UserDB: New User Flow
OAuthProvider->>ExtractLogic: New User Info + GroupField path
ExtractLogic->>ExtractLogic: Extract group from JSON
ExtractLogic->>Validation: Check if group exists in config
alt Group is valid
Validation->>UserDB: Create user with assigned group
else Group not found in config
Validation->>UserDB: Create user with default group (log warning)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
oauth/generic.go (1)
273-274: Consider usinglogger.LogDebuginstead ofcommon.SysLogfor consistency.This changes the log level from DEBUG to INFO (SysLog). Logging successful user info retrieval on every OAuth login may be noisy in production. Additionally, the
groupvalue could contain sensitive role/permission information.♻️ Suggested change to maintain debug-level logging
- common.SysLog(fmt.Sprintf("[OAuth-Generic-%s] GetUserInfo success: id=%s, username=%s, name=%s, email=%s, group=%s", - p.config.Slug, userId, username, displayName, email, group)) + logger.LogDebug(ctx, "[OAuth-Generic-%s] GetUserInfo success: id=%s, username=%s, name=%s, email=%s, group=%s", + p.config.Slug, userId, username, displayName, email, group)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oauth/generic.go` around lines 273 - 274, Replace the call to common.SysLog in the OAuth success path with a debug-level logger call (e.g., logger.LogDebug or the package logger instance used in this file) so the message remains at DEBUG level; include p.config.Slug, userId, username, displayName and email but do not log the raw group value (either omit it or redact/mask it) to avoid verbose/noisy INFO logs and protect sensitive role/permission data—update the single statement that currently calls common.SysLog(...) to the debug logger and remove or redact the group variable in the formatted message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/custom_oauth.go`:
- Line 289: Change the GroupField field from string to *string in the
request/struct where GroupField is defined (to match WellKnown, AccessPolicy
patterns) so you can distinguish "not provided" vs "set to empty"; then update
any handlers and assignment logic that reference GroupField (e.g., where you
currently check if req.GroupField != "" and where you copy to the model) to use
nil checks (if req.GroupField != nil) and allow setting the target to
*req.GroupField (including empty string) to support clearing the existing value
— also update the corresponding JSON unmarshalling usage and any code paths
around GroupField at the other occurrence noted so they use the pointer
semantics.
In `@controller/oauth.go`:
- Around line 213-234: The current flow uses FillUserByProviderID then calls
user.Update(false), which does a full-row overwrite and can clobber concurrent
changes to UsedQuota and RequestCount; instead change the update to a targeted
column update that only modifies the Group field (or add a new method like
UpdateGroup / UpdateColumns that issues an UPDATE users SET group=... WHERE
id=...), and invoke that from the OAuth path where you currently call
user.Update(false) (keep the ratio_setting.ContainsGroupRatio check and logging,
but replace the full-row Update with a column-specific update to avoid
overwriting UsedQuota/RequestCount).
- Line 218: The code incorrectly uses
ratio_setting.ContainsGroupRatio(oauthUser.Group) to decide OAuth group
assignment; replace this with a new dedicated OAuth group validation that reads
from a separate configuration (e.g., oauth_allowed_groups or
OAuthGroupAllowlist) rather than group_ratio_setting. Add a function like
OAuthGroupAllowed(group string) bool (or a method on a new OAuthConfig struct)
and call that where oauthUser.Group is validated, ensure the new config is
loaded/validated at startup, and remove dependence on
ratio_setting.ContainsGroupRatio so pricing tiers do not control OAuth
assignment.
---
Nitpick comments:
In `@oauth/generic.go`:
- Around line 273-274: Replace the call to common.SysLog in the OAuth success
path with a debug-level logger call (e.g., logger.LogDebug or the package logger
instance used in this file) so the message remains at DEBUG level; include
p.config.Slug, userId, username, displayName and email but do not log the raw
group value (either omit it or redact/mask it) to avoid verbose/noisy INFO logs
and protect sensitive role/permission data—update the single statement that
currently calls common.SysLog(...) to the debug logger and remove or redact the
group variable in the formatted message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dbeddf0-f5b7-487a-8f17-a99661b43e02
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
controller/custom_oauth.gocontroller/oauth.gogo.modmodel/custom_oauth_provider.gooauth/generic.gooauth/types.goweb/src/components/settings/CustomOAuthSetting.jsx
| UsernameField string `json:"username_field"` | ||
| DisplayNameField string `json:"display_name_field"` | ||
| EmailField string `json:"email_field"` | ||
| GroupField string `json:"group_field"` |
Contributor
There was a problem hiding this comment.
GroupField cannot be cleared once set due to non-pointer type.
Unlike other optional fields (WellKnown, AccessPolicy, etc.) which use *string to distinguish "not provided" from "set to empty", GroupField is a plain string. The condition if req.GroupField != "" prevents clearing an existing value.
🔧 Proposed fix to allow clearing GroupField
type UpdateCustomOAuthProviderRequest struct {
...
- GroupField string `json:"group_field"`
+ GroupField *string `json:"group_field"`
...
}And update the handler:
- if req.GroupField != "" {
- provider.GroupField = req.GroupField
+ if req.GroupField != nil {
+ provider.GroupField = *req.GroupField
}Also applies to: 376-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/custom_oauth.go` at line 289, Change the GroupField field from
string to *string in the request/struct where GroupField is defined (to match
WellKnown, AccessPolicy patterns) so you can distinguish "not provided" vs "set
to empty"; then update any handlers and assignment logic that reference
GroupField (e.g., where you currently check if req.GroupField != "" and where
you copy to the model) to use nil checks (if req.GroupField != nil) and allow
setting the target to *req.GroupField (including empty string) to support
clearing the existing value — also update the corresponding JSON unmarshalling
usage and any code paths around GroupField at the other occurrence noted so they
use the pointer semantics.
Comment on lines
+213
to
+234
|
|
||
| // Update user's group if OAuth provides a different group and it's in available groups | ||
| if oauthUser.Group != "" && oauthUser.Group != user.Group { | ||
| common.SysLog(fmt.Sprintf("[OAuth] User %d current group: '%s', OAuth group: '%s'", user.Id, user.Group, oauthUser.Group)) | ||
| // Check if group exists in group ratio settings | ||
| if ratio_setting.ContainsGroupRatio(oauthUser.Group) { | ||
| user.Group = oauthUser.Group | ||
| if err := user.Update(false); err != nil { | ||
| common.SysError(fmt.Sprintf("[OAuth] Failed to update user %d group to '%s': %s", user.Id, oauthUser.Group, err.Error())) | ||
| } else { | ||
| common.SysLog(fmt.Sprintf("[OAuth] Updated user %d group to '%s' from OAuth provider", user.Id, oauthUser.Group)) | ||
| } | ||
| } else { | ||
| common.SysLog(fmt.Sprintf("[OAuth] OAuth group '%s' not in group ratio settings for user %d, keeping current group '%s'", oauthUser.Group, user.Id, user.Group)) | ||
| } | ||
| } else { | ||
| if oauthUser.Group == "" { | ||
| common.SysLog(fmt.Sprintf("[OAuth] User %d OAuth group is empty, skipping group update", user.Id)) | ||
| } else if oauthUser.Group == user.Group { | ||
| common.SysLog(fmt.Sprintf("[OAuth] User %d group '%s' already matches OAuth group, no update needed", user.Id, user.Group)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition: user.Update(false) can overwrite concurrent quota changes.
The Update(false) method (see model/user.go:494-510) performs a full-row update using the entire user struct. Between FillUserByProviderID (line 205) and Update (line 220), concurrent requests may have modified UsedQuota or RequestCount. The update will blindly overwrite those changes.
Consider using a targeted column update instead:
🔧 Proposed fix using targeted update
if ratio_setting.ContainsGroupRatio(oauthUser.Group) {
- user.Group = oauthUser.Group
- if err := user.Update(false); err != nil {
+ if err := model.DB.Model(user).Update("group", oauthUser.Group).Error; err != nil {
common.SysError(fmt.Sprintf("[OAuth] Failed to update user %d group to '%s': %s", user.Id, oauthUser.Group, err.Error()))
} else {
+ user.Group = oauthUser.Group // Update local copy after successful DB write
common.SysLog(fmt.Sprintf("[OAuth] Updated user %d group to '%s' from OAuth provider", user.Id, oauthUser.Group))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/oauth.go` around lines 213 - 234, The current flow uses
FillUserByProviderID then calls user.Update(false), which does a full-row
overwrite and can clobber concurrent changes to UsedQuota and RequestCount;
instead change the update to a targeted column update that only modifies the
Group field (or add a new method like UpdateGroup / UpdateColumns that issues an
UPDATE users SET group=... WHERE id=...), and invoke that from the OAuth path
where you currently call user.Update(false) (keep the
ratio_setting.ContainsGroupRatio check and logging, but replace the full-row
Update with a column-specific update to avoid overwriting
UsedQuota/RequestCount).
| if oauthUser.Group != "" && oauthUser.Group != user.Group { | ||
| common.SysLog(fmt.Sprintf("[OAuth] User %d current group: '%s', OAuth group: '%s'", user.Id, user.Group, oauthUser.Group)) | ||
| // Check if group exists in group ratio settings | ||
| if ratio_setting.ContainsGroupRatio(oauthUser.Group) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how groups are typically validated elsewhere in the codebase
rg -n --type go -C3 'ContainsGroupRatio|groupRatioMap' --glob '!*_test.go'Repository: QuantumNous/new-api
Length of output: 4535
🏁 Script executed:
#!/bin/bash
# Find group configuration files and what groups are defined
find . -type f -name "*.go" | xargs grep -l "defaultGroup" | head -10Repository: QuantumNous/new-api
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check what groups are defined in the ratio setting
rg -n "defaultGroupRatio|defaultGroupSpecialUsableGroup" --type go -A 10 | head -80Repository: QuantumNous/new-api
Length of output: 3317
🏁 Script executed:
#!/bin/bash
# Check if there's any OAuth-specific group configuration
rg -n "oauth.*group|OAuth.*[Gg]roup" --type go | grep -i configRepository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look for other group validation patterns in the codebase
rg -n "user\.Group\s*=|Group.*=" --type go controller/ | head -20Repository: QuantumNous/new-api
Length of output: 1408
🏁 Script executed:
#!/bin/bash
# Check the oauth.go file context around lines 215-295 to understand the group assignment flow
sed -n '210,300p' controller/oauth.goRepository: QuantumNous/new-api
Length of output: 3629
🏁 Script executed:
#!/bin/bash
# Check if there's OAuth provider configuration for allowed groups
rg -n "oauth.*config|oauthUser|GetUserInfoFromOAuth" --type go -B 2 -A 5 | head -100Repository: QuantumNous/new-api
Length of output: 6025
🏁 Script executed:
#!/bin/bash
# Look for OAuthUser struct definition to understand what Group field represents
rg -n "type.*OAuthUser|oauthUser.*Group" --type go -B 2 -A 8 | head -80Repository: QuantumNous/new-api
Length of output: 4314
🏁 Script executed:
#!/bin/bash
# Check the full OAuthUser struct definition to see if there's any group-related field
rg -n "type OAuthUser" --type go -A 15 oauth/Repository: QuantumNous/new-api
Length of output: 944
🏁 Script executed:
#!/bin/bash
# Look for where OAuthUser.Group is populated from OAuth provider
rg -n "\.Group\s*=" --type go oauth/ | head -20Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if there's any OAuth provider configuration related to allowed groups
rg -n "config.*[Gg]roup|allowedGroup|groupMapping" --type go -i | head -30Repository: QuantumNous/new-api
Length of output: 537
🏁 Script executed:
#!/bin/bash
# Check the GenericOAuthProvider config to understand GroupField better
rg -n "type.*Provider|GroupField" --type go oauth/ -B 2 -A 2 | head -50Repository: QuantumNous/new-api
Length of output: 1828
🏁 Script executed:
#!/bin/bash
# Look for GenericOAuthProvider struct and its config
rg -n "type GenericOAuthProvider|type.*Config" --type go oauth/generic.go -A 20 | head -80Repository: QuantumNous/new-api
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Check what GroupField is used for in the generic provider
sed -n '240,280p' oauth/generic.goRepository: QuantumNous/new-api
Length of output: 1670
Replace ratio_setting.ContainsGroupRatio() with dedicated OAuth group validation.
Using ContainsGroupRatio() to validate OAuth-provided groups is semantically incorrect—it checks pricing tier configuration rather than authorization policy. This couples billing configuration to user assignment logic, creating maintenance risks:
- Adding a pricing tier implicitly makes it OAuth-assignable
- Removing a tier silently breaks existing OAuth group assignments
- The validation doesn't reflect its actual purpose
Introduce a dedicated "OAuth allowed groups" configuration (separate from group_ratio_setting) to explicitly control which groups can be auto-assigned via OAuth, independent of pricing tiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/oauth.go` at line 218, The code incorrectly uses
ratio_setting.ContainsGroupRatio(oauthUser.Group) to decide OAuth group
assignment; replace this with a new dedicated OAuth group validation that reads
from a separate configuration (e.g., oauth_allowed_groups or
OAuthGroupAllowlist) rather than group_ratio_setting. Add a function like
OAuthGroupAllowed(group string) bool (or a method on a new OAuthConfig struct)
and call that where oauthUser.Group is validated, ensure the new config is
loaded/validated at startup, and remove dependence on
ratio_setting.ContainsGroupRatio so pricing tiers do not control OAuth
assignment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit