- Notifications
You must be signed in to change notification settings - Fork1k
feat: implement custom site-wide roles in the database#13289
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
b457981 toea9b9beCompare21f2cee to0d91366CompareThere 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.
| funcList[Fany,Tany](list []F,convertfunc(F)T) []T { | ||
| into:=make([]T,0,len(list)) | ||
| for_,item:=rangelist { | ||
| into=append(into,convert(item)) | ||
| returnListLazy(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. | ||
| funcListLazy[Fany,Tany](convertfunc(F)T)func(list []F) []T { | ||
| returnfunc(list []F) []T { | ||
| into:=make([]T,0,len(list)) | ||
| for_,item:=rangelist { | ||
| into=append(into,convert(item)) | ||
| } | ||
| returninto | ||
| } | ||
| } | ||
| funcMap[Kcomparable,Fany,Tany](paramsmap[K]F,convertfunc(F)T)map[K]T { | ||
| into:=make(map[K]T) | ||
| fork,item:=rangeparams { | ||
| into[k]=convert(item) | ||
| } | ||
| returninto | ||
| } |
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 incoderd/util
| typeRolestruct { | ||
| typeSlimRolestruct { | ||
| Namestring`json:"name"` | ||
| DisplayNamestring`json:"display_name"` | ||
| } | ||
| typeAssignableRolesstruct { | ||
| Role | ||
| SlimRole | ||
| Assignablebool`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 betweenRole andSlimRole?
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 |
Uh oh!
There was an error while loading.Please reload this page.
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