- Notifications
You must be signed in to change notification settings - Fork927
feat: add api for patching custom org roles#13357
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| func (api*API)updateOrganizationMemberRoles(ctx context.Context,args database.UpdateMemberRolesParams) (database.OrganizationMember,error) { | ||
| // Enforce only site wide roles | ||
| for_,r:=rangeargs.GrantedRoles { | ||
| // Must be an org role for the org in the args | ||
| orgID,ok:=rbac.IsOrgRole(r) | ||
| if!ok { | ||
| return database.OrganizationMember{},xerrors.Errorf("must only update organization roles") | ||
| } | ||
| roleOrg,err:=uuid.Parse(orgID) | ||
| iferr!=nil { | ||
| return database.OrganizationMember{},xerrors.Errorf("Role must have proper UUIDs for organization, %q does not",r) | ||
| } | ||
| ifroleOrg!=args.OrgID { | ||
| return database.OrganizationMember{},xerrors.Errorf("Must only pass roles for org %q",args.OrgID.String()) | ||
| } | ||
| if_,err:=rbac.RoleByName(r);err!=nil { | ||
| return database.OrganizationMember{},xerrors.Errorf("%q is not a supported organization role",r) | ||
| } | ||
| } | ||
| updatedUser,err:=api.Database.UpdateMemberRoles(ctx,args) | ||
| iferr!=nil { | ||
| return database.OrganizationMember{},xerrors.Errorf("Update site roles: %w",err) | ||
| } | ||
| returnupdatedUser,nil | ||
| } |
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.
This is all moved to dbauthz. The same is done for site wide roles in dbauthz already.
ddad37a tocd3ca65CompareEmyrk commentedMay 24, 2024 • 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.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
968cb76 to44dddddCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
enterprise/coderd/roles.go Outdated
| if len(role.OrganizationPermissions) > 1 { | ||
| httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid request, Only 1 organization can be assigned permissions", | ||
| Detail: "roles can only contain 1 organization", | ||
| }) | ||
| return codersdk.Role{}, false | ||
| } |
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.
If this is an invalid state, why is it representable in acodersdk.Role?
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.
Yea this is annoying. It is possible in the rbac library, but we never use this functionality. Essentially, making a role that has permissions in 2 orgs makes no sense imo. Because it is technically a possibility, if we hit this, and I strip it from the sdk, then if we ever hit it, I have to throw information out.
I'll make this impossible on the sdk. Let me see what happens on the BE when I hit the edge case that should never be hit.
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.
It silently omits permissions. Which feels a bit off.
coder/coderd/database/db2sdk/db2sdk.go
Lines 540 to 542 in553dca2
| // This is not perfect. If there are organization permissions in another | |
| // organization, they will be omitted. This should not be allowed, so | |
| // should never happen. |
Returning an error feels like it could have a single role "break" things. Wondering if I could include an extra field with likewarnings 🤔
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.
Returning an error doesn't have to specifically break things, we could exportErrNoMultiOrgRole andIsNoMultiOrgRoleError() in db2sdk and handle them appropriately by dropping an error log. This would at least allow us to detect this in tests.
However, it feels like the 'right' fix here is to just not allow multi-org roles at all inrbac. It's not a blocker to this PR, but it feels like something we should fix.
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.
Yea, it might be worth refactoring the rbac to just prevent this altogether 🤔. I think it could be done without trickling down to the rego.
I think that is the better approach, as I can't see a reason for it in the future.
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.
| r.Route("/users/roles", func(r chi.Router) { | ||
| r.Use( | ||
| apiKeyMiddleware, | ||
| ) | ||
| r.Group(func(r chi.Router) { | ||
| r.Use( | ||
| api.customRolesEnabledMW, | ||
| ) | ||
| r.Patch("/", api.patchRole) | ||
| }) | ||
| // Unfortunate, but this r.Route overrides the AGPL roles route. | ||
| // The AGPL does not have the entitlements to block the licensed | ||
| // routes, so we need to duplicate the AGPL here. | ||
| r.Get("/", api.AGPL.AssignableSiteRoles) | ||
| }) |
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.
This is why that interface was created. This was moved to the/organizations route, and would require duplicating all the routes. So instead the code lives in AGPL and enterprise just patches the interface.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
enterprise/coderd/roles.go Outdated
| if len(role.OrganizationPermissions) > 1 { | ||
| httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid request, Only 1 organization can be assigned permissions", | ||
| Detail: "roles can only contain 1 organization", | ||
| }) | ||
| return codersdk.Role{}, false | ||
| } |
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.
Yea this is annoying. It is possible in the rbac library, but we never use this functionality. Essentially, making a role that has permissions in 2 orgs makes no sense imo. Because it is technically a possibility, if we hit this, and I strip it from the sdk, then if we ever hit it, I have to throw information out.
I'll make this impossible on the sdk. Let me see what happens on the BE when I hit the edge case that should never be hit.
Uh oh!
There was an error while loading.Please reload this page.
469f74f to6eb1167Compare6eb1167 to5ac97f8CompareThere 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.
LGTM. I think we may need to do some follow-up changes so we don't need to worry about the multi-org role issue, but that's out of scope here.
afd9d3b intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
What this does
Adds apis to create custom roles for a given organization.
Removes site role patching
Custom site role creation was moved to custom org role creating. It was decided to do org roles first. Fixed the unit tests to do org roles rather than site.