DEVPROD-5753: Implement and apply requireHostAccess GQL directive #8337
Conversation
graphql/query_resolver.go
Outdated
| forbiddenHosts := []string{} | ||
| for _, h := range hosts { | ||
| if !userHasHostPermission(usr, h.Distro.Id, evergreen.HostsView.Value) { | ||
| forbiddenHosts = append(forbiddenHosts, h.Id) | ||
| } | ||
| } | ||
| if len(forbiddenHosts) > 0 { | ||
| return nil, Forbidden.Send(ctx, fmt.Sprintf("user '%s' does not have permission to access one or more hosts.", usr.Username())) |
There was a problem hiding this comment.
since we don't really do anything with the forbiddenHosts, I wonder if it makes sense to do this check in the same loop where we're building the APIHosts below (and just return immediately if you encounter no permissions)
There was a problem hiding this comment.
You can either do that or include a formatted list of all the forbidden hosts in the error message here
There was a problem hiding this comment.
I originally considered returning a list of hosts that are inaccessible but I think that is insecure since the query parameters match the hostIds in the error message which makes the error message really useful to someone who doesn't have host permissions
| return nil, ResourceNotFound.Send(ctx, "host not specified") | ||
| } | ||
| hostId, hasHostId := args["hostId"].(string) | ||
| hostIdsInterface, hasHostIds := args["hostIds"].([]interface{}) |
There was a problem hiding this comment.
does it make sense to do this so you can skip the extra loop for hostIds 👀
| hostIdsInterface, hasHostIds := args["hostIds"].([]interface{}) | |
| hostIds, hasHostIds := args["hostIds"].([]string) |
| @@ -218,7 +234,87 @@ func setupPermissions(t *testing.T) { | |||
| } | |||
| require.NoError(t, roleManager.UpdateRole(logViewRole)) | |||
| } | |||
|
|
|||
| func TestRequireHostAccess(t *testing.T) { | |||
There was a problem hiding this comment.
The references to .Equal or .EqualError in this file should use assert rather than require for codebase style consistency.
graphql/query_resolver.go
Outdated
| forbiddenHosts := []string{} | ||
| for _, h := range hosts { | ||
| if !userHasHostPermission(usr, h.Distro.Id, evergreen.HostsView.Value) { | ||
| forbiddenHosts = append(forbiddenHosts, h.Id) | ||
| } | ||
| } | ||
| if len(forbiddenHosts) > 0 { | ||
| return nil, Forbidden.Send(ctx, fmt.Sprintf("user '%s' does not have permission to access one or more hosts.", usr.Username())) |
There was a problem hiding this comment.
You can either do that or include a formatted list of all the forbidden hosts in the error message here
graphql/resolver.go
Outdated
| if !hasHostId && !hasHostIds { | ||
| return nil, ResourceNotFound.Send(ctx, "host not specified") | ||
| } | ||
| hostIdsToCheck := []string{} |
There was a problem hiding this comment.
nit: initialize as []string{hostId} and remove L56
graphql/resolver.go
Outdated
| var requiredLevel int | ||
| if access == HostAccessLevelEdit { | ||
| requiredLevel = evergreen.HostsEdit.Value | ||
| } else if access == HostAccessLevelView { |
There was a problem hiding this comment.
is access not being specified a valid case? If not I would use else rather than else if
| return nil, ResourceNotFound.Send(ctx, "No matching hosts found") | ||
| } | ||
| for _, h := range hostsToCheck { | ||
| if !userHasHostPermission(user, h.Distro.Id, requiredLevel) { |
There was a problem hiding this comment.
This should be consistent with the logic here (as in whether we keep track of a list of forbidden hosts or not)
Co-authored-by: minnakt <[email protected]>
Co-authored-by: minnakt <[email protected]>
DEVPROD-5753
Description
The actual added code to review is less than 540 lines.
These code changes implement and apply the
@requireHostAccessdirective to these mutations:and these queries:
I also applied the directive permission logic to the
hostsquery resolver instead of the schema since there is are no host IDs available before the host db query takes place.These code changes combine the permission for editing spawn hosts and non-spawn hosts. Currently in Evergreen, the difference between the two implementations is that spawn hosts permission is granted if the host startedBy field is the same as the requesting user and the user has the host edit permission (code) whereas modifying a non-spawn host only checks the edit permission (code). I confirmed in staging that I am able to modify my spawn host with the current code changes and I could use edge case clarification from an App engineer.
e2e test is fixed in this UI PR: evergreen-ci/ui#410