-
Notifications
You must be signed in to change notification settings - Fork 577
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement custom site-wide roles in the database #13289
Conversation
b457981
to
ea9b9be
Compare
21f2cee
to
0d91366
Compare
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 PR is a bit too large for me to review comforably.
Can you split it up a bit?
Here's my suggestion, but feel free to modify:
- Introduce new DB types / queries etc.
- Add required dbauthz changes
- Plumb in new HTTP middleware etc.
func List[F any, T any](list []F, convert func(F) T) []T { | ||
into := make([]T, 0, len(list)) | ||
for _, item := range list { | ||
into = append(into, convert(item)) | ||
return ListLazy(convert)(list) | ||
} | ||
|
||
// ListLazy returns the converter function for a list, but does not eval | ||
// the input. Helpful for combining the Map and the List functions. | ||
func ListLazy[F any, T any](convert func(F) T) func(list []F) []T { | ||
return func(list []F) []T { | ||
into := make([]T, 0, len(list)) | ||
for _, item := range list { | ||
into = append(into, convert(item)) | ||
} | ||
return into | ||
} | ||
} | ||
|
||
func Map[K comparable, F any, T any](params map[K]F, convert func(F) T) map[K]T { | ||
into := make(map[K]T) | ||
for k, item := range params { | ||
into[k] = convert(item) | ||
} | ||
return into | ||
} |
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.
All of these should probably be in coderd/util
type Role struct { | ||
type SlimRole struct { | ||
Name string `json:"name"` | ||
DisplayName string `json:"display_name"` | ||
} | ||
|
||
type AssignableRoles struct { | ||
Role | ||
SlimRole | ||
Assignable bool `json:"assignable"` | ||
} |
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.
Can you provide a comment explaining the difference between Role
and SlimRole
?
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'd like to see more tests:
- What happens if we have existing custom roles and our license expires?
- What happens if we patch a role and leave out certain fields in the request? Do the unspecified values get overwritten in the DB?
Split into a stack: #13298 |
What this does
Adds the ability to create custom roles at the site level. Intentionally not doing org scoped roles yet, as org roles cannot be seen on the UI yet.
This includes the api endpoint to create a custom role. Ideally that would be another PR, but I wanted to actually test the custom roles through the api actor logic. Roles and the actor are so intertwined, if the custom roles do not work to actually grant perms at the api layer, then the feature is moot.
Future work