Conversation
Contributor
📝 WalkthroughWalkthroughInjects the DogTags service into JIT OIDC and SAML provisioning flows; JIT upsert/create functions gain a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Handler as Auth Callback Handler
participant DogTags as dogtags.Service
participant UserStore as User creation/upsert
Client->>API_Handler: OAuth/SAML callback (auth data, roles)
API_Handler->>DogTags: GetFlag(ETAC_ENABLED)
API_Handler->>API_Handler: determine roles, hasValidRolesForETAC(roles)
alt ETAC enabled
API_Handler->>UserStore: jitUserCreate/upsert(..., dogTagsService)
Note right of UserStore: user.AllEnvironments = !hasValidRolesForETAC
else ETAC disabled
API_Handler->>UserStore: jitUserCreate/upsert(..., dogTagsService)
Note right of UserStore: user.AllEnvironments = true
end
UserStore-->>API_Handler: created/updated user + session
API_Handler-->>Client: redirect with session cookie
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 docstrings
🧪 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 |
Flake85
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/api/src/api/v2/auth/saml.go (1)
537-556: Consider: propagatingdogTagsServicevia context or struct rather than plumbing through signatures.Threading
dogtags.ServicethroughjitSAMLUserUpsert→jitSAMLUserCreateworks, but as more feature flags get added the signature will continue to grow. An alternative is to attach it to a small provisioning struct (or keep it onManagementResourceand make these methods rather than free functions). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/api/v2/auth/saml.go` around lines 537 - 556, The current issue is that dogTagsService is being passed through function signatures (jitSAMLUserUpsert -> jitSAMLUserCreate), which will bloat signatures as more dependencies are added; refactor by grouping provisioning dependencies into a small struct (e.g., Provisioner or attach to ManagementResource) and convert jitSAMLUserUpsert and jitSAMLUserCreate into receiver methods (or accept the new Provisioner) so they read the dogTagsService from that struct instead of a parameter; update callers to construct/pass the Provisioner or use the ManagementResource receiver and remove dogTagsService from the free function signatures (keep SanitizeAndGetRoles, LookupUser, UpdateUser uses intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/api/src/api/v2/auth/saml.go`:
- Around line 537-556: The current issue is that dogTagsService is being passed
through function signatures (jitSAMLUserUpsert -> jitSAMLUserCreate), which will
bloat signatures as more dependencies are added; refactor by grouping
provisioning dependencies into a small struct (e.g., Provisioner or attach to
ManagementResource) and convert jitSAMLUserUpsert and jitSAMLUserCreate into
receiver methods (or accept the new Provisioner) so they read the dogTagsService
from that struct instead of a parameter; update callers to construct/pass the
Provisioner or use the ManagementResource receiver and remove dogTagsService
from the free function signatures (keep SanitizeAndGetRoles, LookupUser,
UpdateUser uses intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0408ded2-5405-4a4f-9f91-f92f23512ebf
📒 Files selected for processing (3)
cmd/api/src/api/v2/auth/oidc.gocmd/api/src/api/v2/auth/saml.gocmd/api/src/api/v2/auth/saml_test.go
Flake85
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/api/v2/auth/saml_test.go`:
- Around line 3061-3139: The test currently enables ETAC but stubs LookupUser to
return an existing user so JIT provisioning isn't exercised; change this case
(or add a new one) to simulate JIT creation by making
mock.mockDatabase.EXPECT().LookupUser(...) return database.ErrNotFound, then
expect mock.mockDatabase.EXPECT().CreateUser(...) to be called and capture the
created model.User to assert its AllEnvironments field (and role handling for
regular vs admin/power) when dogtags.TestOverrides enables ETAC; keep the rest
of the flow (ParseResponse, CreateUserSession, CreateAuditLog, response
assertions) but replace the LookupUser return and add the CreateUser expectation
and assertion against AllEnvironments.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 29fa744e-d4d4-45eb-a61a-bcbefbcd894d
📒 Files selected for processing (1)
cmd/api/src/api/v2/auth/saml_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/api/v2/auth/saml_test.go`:
- Around line 3101-3251: The ETAC tests currently set
SSOProvider.Config.AutoProvision.RoleProvision=false and both mocked roles
include the auth:ManageUsers permission, so claim-driven role resolution and the
regular-user branch aren’t exercised; update the fixtures used in the two test
cases (the SSOProvider returned by GetSSOProviderBySlug and the roles returned
by GetAllRoles) to set AutoProvision.RoleProvision=true so SAML role attributes
are used, and change the regular-user role (the model.Role with Name
auth.RoleUser in the GetAllRoles mock) to omit the ManageUsers permission (give
it a distinct, non-admin permission) while keeping the admin role with
ManageUsers, so ParseResponse-driven role assignment in the SAML assertion paths
(and the CreateUser gomock.Cond checks on AllEnvironments) are actually
validated.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b054d8e5-9a79-42c5-8ca4-45c7b94707aa
📒 Files selected for processing (1)
cmd/api/src/api/v2/auth/saml_test.go
mvlipka
approved these changes
Apr 20, 2026
Contributor
mvlipka
left a comment
mvlipka
left a comment
There was a problem hiding this comment.
I reviewed Flechas' code and verified the testing
I wrote the unit tests, though, so will wait for another engineer's approval before merging.
ALCooper12
approved these changes
Apr 20, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Description
We need to update SSO and OIDC account creation to respect ETAC controls
When ETAC is enabled, handle account creation with respect to the roles in the claims
If a user is a privileged user, set AllEnvironments to true
If a user is a regular user then set AllEnvironments to false
When ETAC is disabled, always set AllEnvironments to true
Motivation and Context
Resolves BED-7761
Why is this change required? What problem does it solve?
all_environments is always set to false when creating an account via SSO and it doesn't respect ETAC.
How Has This Been Tested?
Authentik was used locally so we can have an SSO provider. I set the default user role using BHE's admin SSO configuration for testing the created user via SSO and setting the different default roles to each of the different roles provided. I also manually enabled/disabled ETAC in the parameters table. After each test, I would confirm that all_environments was set to the correct value.
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
I tested this on BHE using my local environment. As mentioned above, I used authentik for my provider.
Screenshots (optional):
ETAC Disabled:
read only:
regular user:
admin:
power user:
ETAC Enabled:
read only:
regular user:
admin:
power user:
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests