- Notifications
You must be signed in to change notification settings - Fork4
fix: add proper OIDC user role validation#247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
When creating OIDC users, the provider was calling UpdateUserRoleseven with empty roles due to the default schema value, causing theserver error "User Role Field is set in the OIDC configuration".OIDC users should get their roles exclusively from the OIDC provider'srole mapping, not from explicit API calls. This fix:- Errors if explicit roles are provided for OIDC users- Skips role assignment entirely for OIDC users- Provides clear error messaging about OIDC role behavior🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>
ethanndicksonAug 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we need to updateRead as well here, right? I don't have a Coder deployment w/ an IDP handy*, but I assume if you gave the user managed by Terraform roles via OIDC, Terraform would complain about config drift on every subsequent apply.
*For the same reason, we probably won't be able to have a test for this :( All our provider tests use a containerized coder, and adding a fake IDP for those tests sounds painful.
Update the Read function to not populate roles from server response for OIDC users.This prevents Terraform from detecting config drift when OIDC users have rolesassigned by the OIDC provider but an empty roles list in the Terraform config.Addresses review comment about config drift in PR#247.Co-authored-by: angrycub <464492+angrycub@users.noreply.github.com>
Update OIDC user role handling to use cleaner Go style:- Use negative conditions (loginType != codersdk.LoginTypeOIDC) for better readability- Simplify comments to be more concise and inline- Maintain all existing validation logic and functionalityCo-authored-by: angrycub <464492+angrycub@users.noreply.github.com>
Fix formatting issues found by go fmt, specifically the closing braceplacement in the ImportState function.Co-authored-by: angrycub <464492+angrycub@users.noreply.github.com>
ethanndickson commentedAug 18, 2025
Unfortunately, this won't work, as the API rejects user role updates based off: Since it's possible to have an OIDC user who's roles are managed via the API, we'll instead go with a slightly different approach (in#250). That approach is similar to what we offer for |
When creating OIDC users, the provider was calling UpdateUserRoles even with empty roles due to the default schema value, causing the server error "User Role Field is set in the OIDC configuration".
OIDC users should get their roles exclusively from the OIDC provider's role mapping, not from explicit API calls. This fix:
🤖 Generated withClaude Code