- Notifications
You must be signed in to change notification settings - Fork927
chore: create type for unique role names#13506
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
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.
vibe check 👍
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review:string does not implementfmt.Stringer :(
Uh oh!
There was an error while loading.Please reload this page.
Using `string` was confusing when something should be combined withorg context, and when not to
3977989 tofa2f704CompareThere 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
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
follow-up for TODO?
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
follow-up for TODO
also, should we catch an attempt to lookup an org-scoped role here?
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.
5ccf508 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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