feat: Implied 'member' roles for site and organization #1917
Conversation
There was a problem hiding this comment.
I was worried initially that this concept of "implied roles" makes things too complicated.
As the whole organization-member:<org_id> is just an internal implementation detail of the Rego, I had wondered if we could just stuff that whole issue inside the rbac package and keep it locked away in there.
But we still need to handle cases where users are a member of an org but do not have any additional roles in the org, just their membership.
We don't want to introduce any database dependencies in the rbac package at all, so the rbac package has no real way of just "magically" fetching the user org memberships.
I then thought about passing in the organization IDs of which the subject is a member into authz.Authorize as another argument, but then you can get into weird situations where a user does not have membership of an org but still has roles in that org.
So the solution here makes sense to me.
| @@ -69,7 +69,7 @@ type UpdateUserPasswordRequest struct { | |||
| } | |||
|
|
|||
| type UpdateRoles struct { | |||
| Roles []string `json:"roles" validate:"required"` | |||
| Roles []string `json:"roles" validate:""` | |||
There was a problem hiding this comment.
Does sending { "roles": [] } now mean "remove all roles assigned to the user but keep their membership"?
There was a problem hiding this comment.
Yes. The UI and user will never see the member role. That role is only fetched for authorization.
| Name string `json:"name"` | ||
| // DisplayName is used for UI purposes. If the role has no display name, | ||
| // that means the UI should never display it. | ||
| DisplayName string `json:"display_name"` |
There was a problem hiding this comment.
Can we just not send this role to the UI at all? I think the whole org-member: thing is just the Rego leaking out a bit.
There was a problem hiding this comment.
I don't send it to the UI. I just use the DisplayName to indicate that.
Pretty much in the current form all role lists are compiled from the builtin const. So that list needs to be filtered for the UI. I thought it was easy to just use this field, since it has no purpose for the member roles now
Pretty much had a similar thought process. |
coderd/httpmw/apikey.go
Outdated
| // UserAuthorizationRoles returns the roles used for authorization. | ||
| // Comes from the ExtractAPIKey handler. | ||
| func UserAuthorizationRoles(r *http.Request) database.GetAuthorizationUserRolesRow { |
There was a problem hiding this comment.
These roles are only to be used for authorization now. (they already were, this is just different then the UI displayed roles now)
| @@ -69,7 +69,7 @@ type UpdateUserPasswordRequest struct { | |||
| } | |||
|
|
|||
| type UpdateRoles struct { | |||
| Roles []string `json:"roles" validate:"required"` | |||
| Roles []string `json:"roles" validate:""` | |||
There was a problem hiding this comment.
Yes. The UI and user will never see the member role. That role is only fetched for authorization.
* feat: Member roles are implied and never exlpicitly added * Rename "GetAllUserRoles" to "GetAuthorizationRoles" * feat: Add migration to remove implied roles * rename user auth role middleware
What this does
This modifies the sql query that fetchs roles for authorization to include 'member' and 'organization-member' automatically. This implication means you can never be a user of the platform, and not be a member. And if you are in an organization, you are a organization-member of that org.
This just makes sense, as a user with no roles doesn't make much sense. This just makes sure a user with "no roles" never exists. An organization-member always implies they have their member role. It was weird you could have a user/member with no roles.
UI implications
'member' is never shown on the ui anymore. TBD if that is good UX
As an admin
As a member
UX implications
You can never assign or remove any 'member' roles. They are automatic