- Notifications
You must be signed in to change notification settings - Fork925
chore: reorder prebuilt workspace authorization logic#18506
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
Conversation
0e5bcd0 toa4b2e8dCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
coderd/database/dbauthz/dbauthz.go Outdated
| if prebuiltErr = q.authorizeContext(ctx, action, workspace.AsPrebuild()); prebuiltErr == nil { | ||
| return nil | ||
| } | ||
| return xerrors.Errorf("authorize context as prebuild: %w", errors.Join(workspaceErr, prebuiltErr)) |
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 see what you are doing, but idk how much more value it is to show both errors.
Maybe just show theprebuildErr, since that is a subset. Your call in the end.xerrors.As andxerrors.Is does handle joined errors. It takes the first one it looks like.
ssncferreiraJun 24, 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.
Good point 🤔 I wanted to keep theworkspaceErr to make it explicit that both the workspace and prebuilt authorization paths failed. I think this could be useful for debugging and for quickly identifying that the error comes from this special-case handling.
Instead of usingerrors.Join, maybe I could make it more readable by updating the message to something like:
return xerrors.Errorf("authorize context failed for workspace (%v) and prebuilt (%w)", workspaceErr, prebuiltErr)Wdyt? Addressed in1f0e33a
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.
That works. In production, the%v just translates torbac: forbidden
errUnauthorized = "rbac: forbidden"
Lines 78 to 83 inf3f0183
| func (eUnauthorizedError)Error()string { | |
| ifflag.Lookup("test.v")!=nil { | |
| returne.longError() | |
| } | |
| returnerrUnauthorized | |
| } |
So the message is like:
authorize context failed for workspace (rbac: forbidden) and prebuilt (rbac: forbidden)
(assuming no error chains, which is probably there)
That all looks good to me imo 👍
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
f44969b intomainUh oh!
There was an error while loading.Please reload this page.
Description
Follow-up from PR#18333
Related with:#18333 (comment)
This changes the authorization logic to first try the normal workspace authorization check, and only if the resource is a prebuilt workspace, fall back to the prebuilt workspace authorization check. Since prebuilt workspaces are a subset of workspaces, the normal workspace check is more likely to succeed. This is a small optimization to reduce unnecessary prebuilt authorization calls.