-
Notifications
You must be signed in to change notification settings - Fork 265
Support registration in authz executor #1145
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
Updates the authorization executor so registration flows inferred from authentication flows don’t immediately fail with “User is not authenticated”.
Changes:
- Bypasses the unauthenticated-user failure check when
ctx.FlowTypeisREGISTRATION.
Comments suppressed due to low confidence (1)
backend/internal/flow/executor/authz_executor.go:79
- In registration flows where
requested_permissionsis present (e.g., OAuth authorization request that triggers registration), this change skips the unauthenticated guard but still proceeds to call the authorization service withuserID == ""and no groups. The role service rejects that withErrorMissingUserOrGroups, which will still fail the flow (now with "Authorization validation failure"). Consider explicitly short-circuiting authorization for registration when the user is not authenticated (e.g., returnExecCompletewith emptyauthorized_permissions), or only invoking the authz service once the user is authenticated in the flow.
if ctx.FlowType != common.FlowTypeRegistration && !ctx.AuthenticatedUser.IsAuthenticated {
execResp.Status = common.ExecFailure
execResp.FailureReason = failureReasonUserNotAuthenticated
return execResp, nil
}
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #1145 +/- ##
==========================================
- Coverage 89.39% 82.00% -7.39%
==========================================
Files 555 555
Lines 37480 37480
Branches 1636 1636
==========================================
- Hits 33504 30736 -2768
- Misses 2331 5390 +3059
+ Partials 1645 1354 -291
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4398c62 to
97e904a
Compare
97e904a to
b3f1290
Compare
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
backend/internal/flow/executor/authz_executor.go:79
- Registration flows bypass the unauthenticated check, but the executor can still call AuthorizationService with an empty UserID and empty GroupIDs if
requested_permissionsis present. The authz service/engine path treats “missing both user and groups” as an error (seeinternal/authz/service_test.go), which would still make registration fail. Consider explicitly skipping authorization evaluation for unauthenticated registration flows (e.g., immediately returnExecCompletewith empty authorized permissions), or gate the service call on having a non-empty userID or at least one groupID.
if ctx.FlowType != common.FlowTypeRegistration && !ctx.AuthenticatedUser.IsAuthenticated {
execResp.Status = common.ExecFailure
execResp.FailureReason = failureReasonUserNotAuthenticated
return execResp, nil
}
| } | ||
|
|
||
| if !ctx.AuthenticatedUser.IsAuthenticated { | ||
| if ctx.FlowType != common.FlowTypeRegistration && !ctx.AuthenticatedUser.IsAuthenticated { |
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.
Is it expected to execute rest of the logic of authz executor for registration flows? Or should we simply check if it is registration flow, and return complete status?
Purpose
Update the authz_executor to support registration. In registration flow inferred from the authentication flow, the flow fails with user is not authenticated.
This fix resolves that issue
#1075