OSAC-1812: Auto create IdP user - #782
Conversation
|
@CrystalChun: This pull request references OSAC-1812 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a gRPC JIT user provisioning interceptor and a DAO-backed ChangesJIT user provisioning
Unrelated cleanup
Sequence Diagram(s)sequenceDiagram
participant Client
participant GrpcAuthzInterceptor
participant GrpcJitProvisioningInterceptor
participant UserProvisioner
participant usersDAO
Client->>GrpcAuthzInterceptor: RPC request
GrpcAuthzInterceptor->>GrpcAuthzInterceptor: ContextWithSubject(ctx, subject)
GrpcAuthzInterceptor->>GrpcJitProvisioningInterceptor: ctx with Subject + Token
GrpcJitProvisioningInterceptor->>GrpcJitProvisioningInterceptor: provisionIfNeeded(ctx)
GrpcJitProvisioningInterceptor->>UserProvisioner: Provision(ctx, username, tenant, claims)
UserProvisioner->>usersDAO: List(filter: spec.username==username, limit 1)
alt user exists
usersDAO-->>UserProvisioner: []User (non-empty)
UserProvisioner-->>GrpcJitProvisioningInterceptor: nil
else user missing
usersDAO-->>UserProvisioner: []User (empty)
UserProvisioner->>usersDAO: Create(privatev1.User)
UserProvisioner-->>GrpcJitProvisioningInterceptor: nil
end
GrpcJitProvisioningInterceptor->>Client: invoke handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ 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 |
|
/cc @jhernand |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/grpc_authz_interceptor_jit_test.go`:
- Around line 33-42: The test file formatting is out of sync with Go style, so
update the declarations around GrpcAuthzInterceptor test setup and run gofmt on
internal/auth/grpc_authz_interceptor_jit_test.go to normalize spacing and
alignment. Make sure the variable block containing ctx, ctrl, interceptor,
MockUserProvisioner, and the provisioning state variables matches gofmt output.
In `@internal/auth/grpc_authz_interceptor.go`:
- Around line 616-620: The tenant selection in grpc authz provisioning is
nondeterministic because subject.Tenants.Inclusions()[0] depends on map
iteration order. Update the tenant derivation logic in the interceptor to use a
deterministic tenant source from the claims/policy, or explicitly avoid JIT
provisioning when subject.Tenants contains more than one tenant. Keep the fix
centered around the tenant selection block in the grpc_authz_interceptor flow.
In `@internal/database/dao/user_provisioner_test.go`:
- Around line 312-324: The EnsureUserExists failure-case test is using a context
that fails too early in the List path, so it never reaches the create-failure
branch. Update the test in user_provisioner_test.go around EnsureUserExists to
use a context/mocks setup where the lookup succeeds and only the DAO Create().Do
call fails, so the non-panicking behavior is exercised for the create error
path.
In `@internal/database/dao/user_provisioner.go`:
- Around line 75-79: The user lookup filter in user_provisioner’s provisioning
flow is built by concatenating the JWT-derived username directly into the
predicate, which allows quote or operator injection and can break the query.
Update the lookup in the provisioner method that calls
usersDAO.List().SetFilter(...).SetLimit(1).Do(ctx) to use an escaping or
parameterized filter helper instead of fmt.Sprintf/string concatenation, and
validate the username at the trust boundary with an allow-list before
constructing the filter.
- Around line 74-90: The existence check in UserProvisioner.ProvisionUser
currently only filters by username, which can block provisioning across
different tenants. Update the list lookup in p.usersDAO.List() to include the
same tenant/organization scope used by the create path, using the existing
username plus the tenant/org field from the current request so only users in
that tenant suppress JIT provisioning.
- Around line 143-147: The provisioning log in user_provisioner.go is emitting
PII by including the email address in the InfoContext call. Update the Created
user via just-in-time provisioning log in the UserProvisioner flow to omit the
email field entirely, or if it must remain, change the key to a protected !email
field so the slog redaction handler can strip it. Keep the username and tenant
fields as the audit context.
- Around line 58-69: The DAOUserProvisionerBuilder.Build method currently only
validates logger and trusts usersDAO, which can defer a configuration error
until runtime. Update Build in DAOUserProvisionerBuilder to fail fast by
checking usersDAO for nil before constructing DAOUserProvisioner, and return a
clear error alongside the existing logger validation so the misconfiguration is
caught during startup instead of on the first request.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 12321953-64cf-4e2b-ab62-fb304c3a0d50
📒 Files selected for processing (6)
internal/auth/grpc_authz_interceptor.gointernal/auth/grpc_authz_interceptor_jit_test.gointernal/auth/user_provisioner_mock.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/database/dao/user_provisioner.gointernal/database/dao/user_provisioner_test.go
| It("Does not crash when DAO create fails", func() { | ||
| // Create a context without a transaction to make DAO operations fail: | ||
| ctxNoTx := context.Background() | ||
|
|
||
| claims := jwt.MapClaims{ | ||
| "email": "iris@example.com", | ||
| "name": "Iris Chang", | ||
| } | ||
|
|
||
| // Should not panic: | ||
| Expect(func() { | ||
| provisioner.EnsureUserExists(ctxNoTx, "iris", "tenant9", claims) | ||
| }).ToNot(Panic()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test never reaches the create-failure branch.
With ctxNoTx, EnsureUserExists returns from the List().Do(ctx) error path, so Create().Do(ctx) is never exercised. Please make the lookup succeed and only the create call fail if you want coverage for lines 130-140.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/database/dao/user_provisioner_test.go` around lines 312 - 324, The
EnsureUserExists failure-case test is using a context that fails too early in
the List path, so it never reaches the create-failure branch. Update the test in
user_provisioner_test.go around EnsureUserExists to use a context/mocks setup
where the lookup succeeds and only the DAO Create().Do call fails, so the
non-panicking behavior is exercised for the create error path.
| // Try to find the user by username using a filter | ||
| filter := fmt.Sprintf("this.spec.username=='%s'", username) | ||
| listResponse, err := p.usersDAO.List(). | ||
| SetFilter(filter). | ||
| SetLimit(1). | ||
| Do(ctx) | ||
| if err != nil { | ||
| p.logger.InfoContext(ctx, "Failed to check if user exists (degraded mode - continuing without JIT provisioning)", | ||
| slog.String("username", username), | ||
| slog.Any("error", err), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| // If user already exists, nothing to do | ||
| if listResponse.GetSize() > 0 { | ||
| p.logger.DebugContext(ctx, "User already exists, skipping JIT provisioning", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope the existence check to the same tenant you later persist.
The create path stores both username and tenant/organization, but Line 75 only checks username. Any existing row for the same username suppresses provisioning for other tenants, so first login can silently skip user creation in the wrong org.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/database/dao/user_provisioner.go` around lines 74 - 90, The
existence check in UserProvisioner.ProvisionUser currently only filters by
username, which can block provisioning across different tenants. Update the list
lookup in p.usersDAO.List() to include the same tenant/organization scope used
by the create path, using the existing username plus the tenant/org field from
the current request so only users in that tenant suppress JIT provisioning.
| filter := fmt.Sprintf("this.spec.username=='%s'", username) | ||
| listResponse, err := p.usersDAO.List(). | ||
| SetFilter(filter). | ||
| SetLimit(1). | ||
| Do(ctx) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't splice raw JWT usernames into the filter expression.
Line 75 builds the lookup predicate from an untrusted claim. A username containing ' or filter operators can change the predicate or make it invalid before it reaches the DAO layer. Use an escaping/parameterization helper instead of string concatenation. As per path instructions, "SQL: parameterized queries only; no string concatenation" and "Validate at trust boundaries with allow-lists, not deny-lists".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/database/dao/user_provisioner.go` around lines 75 - 79, The user
lookup filter in user_provisioner’s provisioning flow is built by concatenating
the JWT-derived username directly into the predicate, which allows quote or
operator injection and can break the query. Update the lookup in the provisioner
method that calls usersDAO.List().SetFilter(...).SetLimit(1).Do(ctx) to use an
escaping or parameterized filter helper instead of fmt.Sprintf/string
concatenation, and validate the username at the trust boundary with an
allow-list before constructing the filter.
Source: Path instructions
|
💀 CI Triage: Root cause: osac-installer main references unmerged osac-operator commit 08991cc; image was never published to ghcr.io Causal chain:
Evidence:
Suggestion: Revert osac-installer commit 768174e OR merge osac-operator PR #320 and manually trigger container image build workflow to publish sha-08991cc image to ghcr.io, then rebuild vmaas-helm snapshot Prow job | Build For deeper investigation, use the |
|
/retest |
|
/test ci/prow/e2e-vmaas |
| //go:generate mockgen -destination=user_provisioner_mock.go -package=auth . UserProvisioner | ||
| type UserProvisioner interface { | ||
| // EnsureUserExists checks if a user exists and creates them if not. This is best-effort and should not return | ||
| // errors - failures are logged but don't block authentication. |
There was a problem hiding this comment.
Why the best-effort approach? Can't we return an error when the provisioning fails?
There was a problem hiding this comment.
Makes sense, returning an error now. Thanks!
| type UserProvisioner interface { | ||
| // EnsureUserExists checks if a user exists and creates them if not. This is best-effort and should not return | ||
| // errors - failures are logged but don't block authentication. | ||
| EnsureUserExists(ctx context.Context, username, tenant string, claims jwt.MapClaims) |
There was a problem hiding this comment.
If this is a provisioner I'd say the method should be named Provision.
There was a problem hiding this comment.
Gotcha, thanks Juan! Updated to Provision method
| // makes their first request, the interceptor will automatically create a user record in the database if one doesn't | ||
| // exist. This is optional - if not set, JIT provisioning is disabled. | ||
| func (b *GrpcAuthzInterceptorBuilder) SetUserProvisioner(value UserProvisioner) *GrpcAuthzInterceptorBuilder { | ||
| b.userProvisioner = value |
There was a problem hiding this comment.
In setters that pass an interface it is good practice to use reflection.NormalizeNil to avoid interfaces that aren't nil but have values that are nil.
There was a problem hiding this comment.
Gotcha, thank you for letting me know! Updated to use reflection.NormalizeNil in the setter.
| } | ||
|
|
||
| // ensureUserExists performs just-in-time user provisioning for JWT-authenticated users by delegating to the | ||
| // configured UserProvisioner. This function never returns an error - failures are logged but don't block the request. |
There was a problem hiding this comment.
I think it is better to fail if the user can't be created. Why do you think it is better to just continue?
There was a problem hiding this comment.
I guess to allow the workflow to continue, but you're right it's better to ensure that the user does exist in our system. Updated to return an error. Thank you!
| // configured UserProvisioner. This function never returns an error - failures are logged but don't block the request. | ||
| func (i *GrpcAuthzInterceptor) ensureUserExists(ctx context.Context, subject *Subject, token *jwt.Token) { | ||
| // Skip JIT provisioning if no user provisioner is configured | ||
| if i.userProvisioner == nil { |
There was a problem hiding this comment.
This ^ is why you want to use reflection.NormalizeNil in the builder. I you don't then this may be true and still you will get a nil pointer exception when trying to use the value.
| if s, ok := orgSlice[0].(string); ok { | ||
| tenant = s | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it would be better to use the Tenants field from the subject. That is the set of tenants that the user can see. If it is exactly one tenant then that is the tenant that you have to use. If empty, more than one, or infinite, then you know this isn't a regular tenant user and you cannot a user record.
There was a problem hiding this comment.
Makes sense, thank you for pointing that out! I've modified it to only store users with a finite set with exactly one tenant.
There was a problem hiding this comment.
I understand that you are putting the implementation of the provisioner in the dao package to avoid circular dependencies. But it doesn't feel right. I'd prefer to keep logic out of the database package, if possible. Can we move this implementation to a new provisioners package, for example?
There was a problem hiding this comment.
That makes sense, I've moved it out. Thank you!
There was a problem hiding this comment.
Looks like the provisioning logic is mostly independent of the rest of the logic of this interceptor: it only happens at the very end, and it only uses the information from the result of this interceptor. Would it make sense to have a separate interceptor for this?
There was a problem hiding this comment.
That makes sense! I created a separate interceptor as suggested. How does it look? Thank you for your feedback.
| }) | ||
|
|
||
| // createUsersTableSQL creates the `users` table for testing purposes. | ||
| const createUsersTableSQL = ` |
There was a problem hiding this comment.
Try to not do this. If the users table is going to be needed, then it should be created by a migration.
3928e2a to
2cd95a5
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Bumps the OSAC_VERSION in the Containerfile to use latest CLI changes from PR osac-project/fulfillment-service#798 to unblock failing e2e test suite in PR osac-project/fulfillment-service#782
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/auth/grpc_jit_provisioning_interceptor.go (1)
104-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
SubjectFromContextpanics — thenilguard is unreachable.
SubjectFromContextpanics when no subject is in context (seeauth_context.go), so theif subject == nilbypass never fires. Any request that reaches this interceptor without a subject (e.g. auth-exempt methods like health/reflection) panics and gets recovered into an Internal error instead of skipping provisioning. Use a non-panicking lookup.🛠️ Suggested fix
- // Get subject from context (set by authz interceptor) - subject := SubjectFromContext(ctx) - if subject == nil { + // Get subject from context (set by authz interceptor) + subject, ok := ctx.Value(subjectContextKey).(*Subject) + if !ok || subject == nil { // No subject means anonymous or unauthenticated request - skip provisioning return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/grpc_jit_provisioning_interceptor.go` around lines 104 - 109, `SubjectFromContext` in `grpc_jit_provisioning_interceptor.go` is the wrong lookup here because it panics when no subject exists, so the `nil` check never protects auth-exempt calls. Update the interceptor to use a non-panicking context lookup for the subject, then keep the existing skip-provisioning path when no subject is present. Make sure the fix is applied in the provisioning interceptor flow where `SubjectFromContext` is currently used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/auth/grpc_jit_provisioning_interceptor.go`:
- Around line 104-109: `SubjectFromContext` in
`grpc_jit_provisioning_interceptor.go` is the wrong lookup here because it
panics when no subject exists, so the `nil` check never protects auth-exempt
calls. Update the interceptor to use a non-panicking context lookup for the
subject, then keep the existing skip-provisioning path when no subject is
present. Make sure the fix is applied in the provisioning interceptor flow where
`SubjectFromContext` is currently used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5979272c-6ae5-4492-9440-799dd35d7d28
📒 Files selected for processing (8)
internal/auth/grpc_authz_interceptor.gointernal/auth/grpc_jit_provisioning_interceptor.gointernal/auth/grpc_jit_provisioning_interceptor_test.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/idp/keycloak/client_test.gointernal/provisioners/user_provisioner.gointernal/provisioners/user_provisioner_test.gointernal/rendering/table_renderer.go
💤 Files with no reviewable changes (5)
- internal/idp/keycloak/client_test.go
- internal/provisioners/user_provisioner.go
- internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
- internal/provisioners/user_provisioner_test.go
- internal/rendering/table_renderer.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/rendering/table_renderer.go (1)
253-266: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish "not found" from other read failures.
loadTablenow collapses everyfs.ReadFileerror into a generic "not found" case, logged only at Debug and silently resolved via the default table. This masks genuinely unexpected failures (e.g. embed corruption, path issues) that previously surfaced as errors. Consider checkingerrors.Is(err, fs.ErrNotExist)and only swallowing that specific case, propagating other errors soRender's Warn-level fallback (and visibility) still applies to them.🔧 Proposed fix
data, err := fs.ReadFile(tablesFS, path.Join("tables", file)) if err != nil { - // If the file doesn't exist, that's okay - we'll use the default table. - // This handles both fs.ErrNotExist and any path-related errors. - r.logger.Debug( - "Table definition not found, will use default", - slog.String("type", string(helper.FullName())), - slog.String("file", file), - ) - return nil, nil + if errors.Is(err, fs.ErrNotExist) { + r.logger.Debug( + "Table definition not found, will use default", + slog.String("type", string(helper.FullName())), + slog.String("file", file), + ) + return nil, nil + } + return nil, fmt.Errorf("failed to read table definition file %q: %w", file, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/rendering/table_renderer.go` around lines 253 - 266, `TableRenderer.loadTable` is swallowing every `fs.ReadFile` failure as if the table file were missing, which hides real read errors. Update the error handling after `fs.ReadFile(tablesFS, path.Join("tables", file))` to only treat `fs.ErrNotExist` as the default-table case, and return any other error so `Render` can surface it through its existing warning/fallback path. Keep the existing debug log for the missing-file branch, but preserve unexpected errors instead of converting them to nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/provisioners/user_provisioner.go`:
- Around line 61-96: The List()-then-Create() flow in user_provisioner.go is not
atomic and can race for concurrent first-logins. Update the user provisioning
logic in the CreateUser path to use a single idempotent write via
p.usersDAO.Create() (or an upsert equivalent), and treat an “already exists”
duplicate error as success instead of returning failure. Keep the existing check
around p.usersDAO.List()/Do(ctx) only if needed for a fast-path, but the
authoritative outcome should come from the create operation and the user should
be considered provisioned when the record already exists.
---
Outside diff comments:
In `@internal/rendering/table_renderer.go`:
- Around line 253-266: `TableRenderer.loadTable` is swallowing every
`fs.ReadFile` failure as if the table file were missing, which hides real read
errors. Update the error handling after `fs.ReadFile(tablesFS,
path.Join("tables", file))` to only treat `fs.ErrNotExist` as the default-table
case, and return any other error so `Render` can surface it through its existing
warning/fallback path. Keep the existing debug log for the missing-file branch,
but preserve unexpected errors instead of converting them to nil.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 71d86eb3-925c-4bd8-9eec-d7aee71a5cec
📒 Files selected for processing (8)
internal/auth/grpc_authz_interceptor.gointernal/auth/grpc_jit_provisioning_interceptor.gointernal/auth/grpc_jit_provisioning_interceptor_test.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/idp/keycloak/client_test.gointernal/provisioners/user_provisioner.gointernal/provisioners/user_provisioner_test.gointernal/rendering/table_renderer.go
| // Check if user exists | ||
| filter := fmt.Sprintf("this.spec.username==%q", username) | ||
| listResponse, err := p.usersDAO.List(). | ||
| SetFilter(filter). | ||
| SetLimit(1). | ||
| Do(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to check if user exists: %w", err) | ||
| } | ||
|
|
||
| // User already exists | ||
| if listResponse.GetSize() > 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Extract claims | ||
| email, _ := claims["email"].(string) | ||
|
|
||
| // Create user | ||
| user := privatev1.User_builder{ | ||
| Metadata: privatev1.Metadata_builder{ | ||
| Name: username, | ||
| Tenant: tenant, | ||
| }.Build(), | ||
| Spec: privatev1.UserSpec_builder{ | ||
| Username: username, | ||
| Email: email, | ||
| Enabled: true, | ||
| }.Build(), | ||
| }.Build() | ||
|
|
||
| _, err = p.usersDAO.Create(). | ||
| SetObject(user). | ||
| Do(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create user: %w", err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Collapse the existence check and create into one idempotent write.
This List()-then-Create() flow races on first login. Two concurrent requests for the same user can both observe “missing”, then the loser gets a duplicate/create error and the interceptor rejects that request even though the user now exists. Treat already exists from Create() as success, or switch this path to an atomic upsert.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/provisioners/user_provisioner.go` around lines 61 - 96, The
List()-then-Create() flow in user_provisioner.go is not atomic and can race for
concurrent first-logins. Update the user provisioning logic in the CreateUser
path to use a single idempotent write via p.usersDAO.Create() (or an upsert
equivalent), and treat an “already exists” duplicate error as success instead of
returning failure. Keep the existing check around p.usersDAO.List()/Do(ctx) only
if needed for a fast-path, but the authoritative outcome should come from the
create operation and the user should be considered provisioned when the record
already exists.
When an IdP user first logs in, we should create the user in our database for persistent records. This creates a new interceptor to handle this case. Assisted-by: Claude Code <noreply@anthropic.com>
Importing NormalizeNil into the util directory to prevent an import cycle. Assisted-by: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
CC @jhernand there was an import cycle somehow
auth → reflection → config → auth
and the suggestion was to move NormalizeNil into a separate utility package. Is this ok or should I explore a different solution?
There was a problem hiding this comment.
I see. I tried to break the dependency between reflection and config in #820. But merge this and I will refactor it later.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-vmaas |
|
Triggered: E2E VMaaS Full Install |
5 participants
Description
When an IdP user first logs in, we should create the user in our database for persistent records. This creates a new
interceptor to handle this case.
Testing
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit