feat: add api for patching custom org roles #13357
Conversation
| func (api *API) updateOrganizationMemberRoles(ctx context.Context, args database.UpdateMemberRolesParams) (database.OrganizationMember, error) { | ||
| // Enforce only site wide roles | ||
| for _, r := range args.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) | ||
| if err != nil { | ||
| return database.OrganizationMember{}, xerrors.Errorf("Role must have proper UUIDs for organization, %q does not", r) | ||
| } | ||
|
|
||
| if roleOrg != 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) | ||
| if err != nil { | ||
| return database.OrganizationMember{}, xerrors.Errorf("Update site roles: %w", err) | ||
| } | ||
| return updatedUser, nil | ||
| } |
There was a problem hiding this comment.
This is all moved to dbauthz. The same is done for site wide roles in dbauthz already.
Emyrk
changed the title
Emyrk
force-pushed
the
stevenmasley/patch_org_roles_rebased
branch
from
ddad37a
to
cd3ca65
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Emyrk
force-pushed
the
stevenmasley/patch_org_roles_rebased
branch
from
968cb76
to
44ddddd
Compare
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.
If this is an invalid state, why is it representable in a codersdk.Role?
There was a problem hiding this comment.
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.
It silently omits permissions. Which feels a bit off.
coder/coderd/database/db2sdk/db2sdk.go
Lines 540 to 542 in 553dca2
Returning an error feels like it could have a single role "break" things. Wondering if I could include an extra field with like warnings 🤔
There was a problem hiding this comment.
Returning an error doesn't have to specifically break things, we could export ErrNoMultiOrgRole and IsNoMultiOrgRoleError() 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 in rbac. It's not a blocker to this PR, but it feels like something we should fix.
There was a problem hiding this comment.
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.
Emyrk
changed the title
| 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.
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.
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.
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.
Emyrk
force-pushed
the
stevenmasley/patch_org_roles_rebased
branch
from
469f74f
to
6eb1167
Compare
Emyrk
force-pushed
the
stevenmasley/patch_org_roles_rebased
branch
from
6eb1167
to
5ac97f8
Compare
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.