- Notifications
You must be signed in to change notification settings - Fork925
feat: allow TemplateAdmin to delete prebuilds via auth layer#18333
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
e05480d todb80b4dComparedb80b4d to2ba15c5CompareUh 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.
Uh 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…resolve import cycle
| // PrebuiltWorkspaces are a subset of Workspaces. | ||
| // Explicitly setting PrebuiltWorkspace permissions for clarity. | ||
| // Note: even without PrebuiltWorkspace permissions, access is still granted via Workspace permissions. | ||
| ResourcePrebuiltWorkspace.Type: {policy.ActionUpdate, policy.ActionDelete}, |
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.
👍 Always +1 for clarity.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -180,7 +180,7 @@ func ExtractOrganizationMember(ctx context.Context, auth func(r *http.Request, a | |||
| organizationMembers, err := db.OrganizationMembers(ctx, database.OrganizationMembersParams{ | |||
| OrganizationID: orgID, | |||
| UserID: user.ID, | |||
| IncludeSystem:false, | |||
| IncludeSystem:true, | |||
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.
review: this is required in order to be able to delete a prebuilt workspace from the CLI, as it needs to fetch it first.
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.
Added to the PR description as well:
- Update middleware ExtractOrganizationMember to include system user members.
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.
Nice work! I don't have any blocking comments.
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.
Requiringworkspace.read simplifies the problem. 👍
| if (action==policy.ActionUpdate||action==policy.ActionDelete)&&workspace.IsPrebuild() { | ||
| // Try prebuilt-specific authorization first | ||
| ifprebuiltErr=q.authorizeContext(ctx,action,workspace.AsPrebuild());prebuiltErr==nil { | ||
| returnnil | ||
| } | ||
| } | ||
| // Fallback to normal workspace authorization check | ||
| iferr:=q.authorizeContext(ctx,action,workspace);err!=nil { | ||
| returnxerrors.Errorf("authorize context: %w",errors.Join(prebuiltErr,err)) | ||
| } |
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 think the normal workspace check is more likely to succeed then the prebuild. So it might be a slight optimization to flip these checks.
But that will probably break some of the unit tests? No need to change this, just a thought.
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 had considered that optimization as well. I believe it might require some test updates, so I'd prefer to merge this PR and address the optimization in a follow-up PR.
| @@ -5564,3 +5564,63 @@ func (s *MethodTestSuite) TestChat() { | |||
| }).Asserts(c, policy.ActionUpdate) | |||
| })) | |||
| } | |||
| func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() { | |||
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.
👍
72f7d70 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Description
This PR adds support for deleting prebuilt workspaces via the authorization layer. It introduces special-case handling to ensure that
prebuilt_workspacepermissions are evaluated when attempting to delete a prebuilt workspace, falling back to the standardworkspaceresource as needed.Prebuilt workspaces are a subset of workspaces, identified by having
owner_idset toPREBUILD_SYSTEM_USER.This means:
prebuilt_workspace.deletepermission is allowed todelete only prebuilt workspaces.workspace.deletepermission candelete both normal and prebuilt workspaces.prebuilt_workspaceresource.To delete a workspace, users must have the following permissions:
workspace.read: to read the current workspace stateupdate: to modify workspace metadata and related resources during deletion (e.g., updating thedeletedfield in the database)delete: to perform the actual deletion of the workspaceChanges
authorizeWorkspace()helper to handle prebuilt workspace authorization logic.prebuilt_workspaceandworkspacepermissions are checked.SystemUserIDconstant from theprebuildspackage to thedatabasepackagePrebuildsSystemUserIDto resolve an import cycle (commitf24e4ab).ExtractOrganizationMemberto include system user members.