chore: create type for unique role names #13506
Conversation
| @@ -192,7 +192,7 @@ func (r *RootCmd) newCreateAdminUserCommand() *serpent.Command { | |||
| HashedPassword: []byte(hashedPassword), | |||
| CreatedAt: dbtime.Now(), | |||
| UpdatedAt: dbtime.Now(), | |||
| RBACRoles: []string{rbac.RoleOwner()}, | |||
| RBACRoles: []string{rbac.RoleOwner().String()}, | |||
There was a problem hiding this comment.
review: string does not implement fmt.Stringer :(
Using `string` was confusing when something should be combined with org context, and when not to
3977989 to
fa2f704
Compare
| @@ -199,7 +199,8 @@ func (api *API) convertAuditLog(ctx context.Context, dblog database.GetAuditLogs | |||
| Roles: []codersdk.SlimRole{}, | |||
| } | |||
|
|
|||
| for _, roleName := range dblog.UserRoles { | |||
| for _, input := range dblog.UserRoles { | |||
| roleName, _ := rbac.RoleNameFromString(input) | |||
There was a problem hiding this comment.
Should we at least drop an error log if we were unable to covert any of the roles rom the db?
There was a problem hiding this comment.
Since this is in the convert audit log, I think that could get quite noisy. I agree this is not ideal
| // TODO: Currently the api only returns site wide roles. | ||
| // Should it return organization roles? | ||
| rbacRole, err := rbac.RoleByName(rbac.RoleIdentifier{ | ||
| Name: roleName, | ||
| OrganizationID: uuid.Nil, | ||
| }) |
There was a problem hiding this comment.
I'm not going to do this until we get farther in the organization work.
Right now this api is used for the /users page on the UI. If I were to add it, then organization roles would pop up there, which is probably incorrect behavior.
So this is a genuine question if we should expand the roles returned, and on the UI filter out the organization ones. But at present, showing org roles at all is incorrect as orgs do not really exist.
| // TODO: This only supports mapping deployment wide roles. Organization scoped roles | ||
| // are unsupported. | ||
| if _, err := rbac.RoleByName(rbac.RoleIdentifier{Name: role}); err == nil { |
There was a problem hiding this comment.
follow-up for TODO
also, should we catch an attempt to lookup an org-scoped role here?
Before
RoleNamewas a string that was formatted<role_name>[:<org_id>]. This was super loose and hard to tell when a string had the org_id and when it did not.This change makes the string a struct. So now all
stringnames are just the name, without theorg_idscope.Why do this?
The database and the sdk store roles as a struct, so it makes sense the rbac package should also. It makes the organizationID an obvious field vs something implied from an arbitrary string format.
It also disambiguates the format at any given point in the code. The old
stringwas almost arbitrary when it was formatted with the org id, and when not