Permissions testing, better handling of "everyone" privileges #1067
Conversation
…rmissions_testing
|
Hi, Thanks a lot for the PR. Please can you update the title and description to fully cover what the PR does, as it's a bit more than just adding tests. e.g. there are some very welcome improvements to error messages, and also some special casing of group "everyone" that wasn't there before. Thanks, Rob |
|
Please can you add unit tests for the removal of "everyone", including the case where "everyone" is not present (which looks like it will fail currently). I agree adding the groups to queue perms is a bit funny, I believe GR has an internal use case that needs this. Someone such as @JamesMurkin may know more. I would love to remove auto queue creation eventually. Thanks, Rob |
|
Hi, I think we all agree that we want to drop auto queue creation, sadly we have an internal use case that relies on it. We'll have to migrate them off before we can remove it. I will initiate this internally however I can't give a timeline quite yet, so we'll have to keep it. |
internal/armada/server/submit.go
Outdated
| code = e.Code() | ||
| } | ||
|
|
||
| return nil, status.Errorf(code, "[SubmitJobs] couldn't get/make queue: %s", err) |
There was a problem hiding this comment.
I think we have moved away from adding the function name into the error messages
There was a problem hiding this comment.
Understood. Should we remove the [FuncName] in the other error strings?
There was a problem hiding this comment.
Please do if you have time, however I just would ideally try to avoid adding more.
internal/armada/server/submit.go
Outdated
| return nil, err | ||
| code := codes.Unknown | ||
| if e, ok := status.FromError(err); ok { | ||
| code = e.Code() |
There was a problem hiding this comment.
Maybe one for a separate PR but we now have something that magically sets the return code base on error type:
https://github.com/G-Research/armada/blob/master/internal/common/armadaerrors/errors.go#L251
I think the plan was to move to using the appropriate error types rather than setting codes explicitly
internal/armada/server/submit.go
Outdated
|
|
||
| groupNames := principal.GetGroupNames() | ||
| everyone_index := slices.Index(groupNames, "everyone") | ||
| groupNames = append(groupNames[:everyone_index], groupNames[everyone_index+1:]...) |
There was a problem hiding this comment.
Several nit picks here:
- Make make remove string from slice a separate function so it can be tested (there are some util libraries in common
- Use the "everyone" constant https://github.com/G-Research/armada/blob/master/internal/common/auth/authorization/common.go#L23
- Have a comment saying why we want to remove this
andrewpcone
changed the title
|
@dejanzele OK to merge? |
This pull request addresses #702