Skip to content

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 24, 2025
Merged

chore: reorder prebuilt workspace authorization logic #18506

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a4b2e8d
chore: reorder prebuilt workspace authorization logic
ssncferreira Jun 23, 2025
471d3e8
chore: update error message
ssncferreira Jun 23, 2025
1f0e33a
chore: improve error message for failed workspace and prebuilt author…
ssncferreira Jun 24, 2025
2c387ce
refactor: simplify workspace type assertion
ssncferreira Jun 24, 2025
4b8775f
Merge remote-tracking branch 'origin/main' into ssncferreira/chore-pr…
ssncferreira Jun 24, 2025
f3f0183
refactor: extract PrebuiltWorkspaceResource interface for prebuilt RB…
ssncferreira Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: update error message
  • Loading branch information
@ssncferreira
ssncferreira committed Jun 23, 2025
commit 471d3e82d3fa917f0677849e0779e76b48e62cab
2 changes: 1 addition & 1 deletion coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func (q *querier) authorizePrebuiltWorkspace(ctx context.Context, action policy.
if prebuiltErr = q.authorizeContext(ctx, action, workspace.AsPrebuild()); prebuiltErr == nil {
return nil
}
return xerrors.Errorf("authorize context: %w", errors.Join(workspaceErr, prebuiltErr))
return xerrors.Errorf("authorize context as prebuild: %w", errors.Join(workspaceErr, prebuiltErr))
Copy link
Member

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 the prebuildErr, since that is a subset. Your call in the end. xerrors.As and xerrors.Is does handle joined errors. It takes the first one it looks like.

Copy link
Contributor Author

@ssncferreira ssncferreira Jun 24, 2025 •
edited
Loading

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 the workspaceErr 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 using errors.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 in 1f0e33a

Copy link
Member

@Emyrk Emyrk Jun 24, 2025 •
edited
Loading

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 to rbac: forbidden

errUnauthorized = "rbac: forbidden"

coder/coderd/rbac/error.go

Lines 78 to 83 in f3f0183

func (e UnauthorizedError) Error() string {
if flag.Lookup("test.v") != nil {
return e.longError()
}
return errUnauthorized
}

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 👍

}

return xerrors.Errorf("authorize context: %w", errors.Join(workspaceErr))
Expand Down
Loading