DEVPROD-5753: Implement and apply requireHostAccess GQL directive #8337

SupaJoon · 2024-09-23T19:45:23Z

Description

The actual added code to review is less than 540 lines.
These code changes implement and apply the @requireHostAccess directive to these mutations:

updateSpawnHostStatus
editSpawnHost
reprovisionToNew
restartJasper
updateHostStatus

and these queries:

host
hostEvents

I also applied the directive permission logic to the hosts query 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

… DEVROD-5753

graphql/query_resolver.go

minnakt · 2024-09-26T18:11:33Z

graphql/query_resolver.go

+	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()))


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)

You can either do that or include a formatted list of all the forbidden hosts in the error message here

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

graphql/resolver.go

minnakt · 2024-09-26T18:15:02Z

graphql/resolver.go

+			return nil, ResourceNotFound.Send(ctx, "host not specified")
+		}
+		hostId, hasHostId := args["hostId"].(string)
+		hostIdsInterface, hasHostIds := args["hostIds"].([]interface{})


does it make sense to do this so you can skip the extra loop for hostIds 👀

Suggested change

hostIdsInterface, hasHostIds := args["hostIds"].([]interface{})

hostIds, hasHostIds := args["hostIds"].([]string)

I tried this and hostIds was always empty

hadjri · 2024-09-26T18:27:49Z

graphql/directive_test.go

@@ -218,7 +234,87 @@ func setupPermissions(t *testing.T) {
 	}
 	require.NoError(t, roleManager.UpdateRole(logViewRole))
 }
-
+func TestRequireHostAccess(t *testing.T) {


The references to .Equal or .EqualError in this file should use assert rather than require for codebase style consistency.

hadjri · 2024-09-26T18:30:03Z

graphql/query_resolver.go

+	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()))


You can either do that or include a formatted list of all the forbidden hosts in the error message here

hadjri · 2024-09-26T18:31:47Z

graphql/resolver.go

+		if !hasHostId && !hasHostIds {
+			return nil, ResourceNotFound.Send(ctx, "host not specified")
+		}
+		hostIdsToCheck := []string{}


nit: initialize as []string{hostId} and remove L56

hadjri · 2024-09-26T18:32:47Z

graphql/resolver.go

+		var requiredLevel int
+		if access == HostAccessLevelEdit {
+			requiredLevel = evergreen.HostsEdit.Value
+		} else if access == HostAccessLevelView {


is access not being specified a valid case? If not I would use else rather than else if

hadjri · 2024-09-26T18:33:49Z

graphql/resolver.go

+			return nil, ResourceNotFound.Send(ctx, "No matching hosts found")
+		}
+		for _, h := range hostsToCheck {
+			if !userHasHostPermission(user, h.Distro.Id, requiredLevel) {


This should be consistent with the logic here (as in whether we keep track of a list of forbidden hosts or not)

I omitted the forbidden hosts from the error message from your link because those hostIDs are the result of a query. I think it makes sense to include the forbidden hostIDs in the directive error message because those hosts were supplied by the user. This could make it easier for the user to ask for help in the support channel.

Co-authored-by: minnakt <47064971+minnakt@users.noreply.github.com>

SupaJoon · 2024-10-02T19:46:09Z

evergreen retry

minnakt · 2024-10-03T14:06:50Z

graphql/resolver.go

+		if err != nil {
+			return nil, InternalServerError.Send(ctx, "Error getting hosts")
+		}


should we include the err message here?

minnakt · 2024-10-03T14:07:53Z

graphql/resolver.go

+		if len(forbiddenHosts) == 1 {
+			return nil, Forbidden.Send(ctx, fmt.Sprintf("user '%s' does not have permission to access host '%v'", user.Username(), forbiddenHosts[0]))
+		} else if len(forbiddenHosts) > 1 {
+			hostsString := strings.Join(forbiddenHosts, ", ")
+			return nil, Forbidden.Send(ctx, fmt.Sprintf("user '%s' does not have permission to access hosts: '%v'", user.Username(), hostsString))
+		}


could this be consolidated? I also think using %s might be slightly better than using %v since it's a string

Suggested change

if len(forbiddenHosts) == 1 {

return nil, Forbidden.Send(ctx, fmt.Sprintf("user '%s' does not have permission to access host '%v'", user.Username(), forbiddenHosts[0]))

} else if len(forbiddenHosts) > 1 {

hostsString := strings.Join(forbiddenHosts, ", ")

return nil, Forbidden.Send(ctx, fmt.Sprintf("user '%s' does not have permission to access hosts: '%v'", user.Username(), hostsString))

}

if len(forbiddenHosts) > 0 {

hostsString := strings.Join(forbiddenHosts, ", ")

return nil, Forbidden.Send(ctx, fmt.Sprintf("user '%s' does not have permission to access host(s): '%s'", user.Username(), hostsString))

}

This could be consolidated but I like how the copy is more specific in the original code. I updated %v to %s.

graphql/resolver.go

Co-authored-by: minnakt <47064971+minnakt@users.noreply.github.com>

…into DEVPROD-5753

SupaJoon added 7 commits September 22, 2024 22:05

schema changes + codegen

0c5c11b

directive implementation

4112897

non-directive permission gating for Hosts query

18763cf

directive unit test

3941c02

directive integration test

2c78735

update test data

f728c82

Merge branch 'main' of https://github.com/evergreen-ci/evergreen into…

a3978e4

… DEVROD-5753

SupaJoon requested review from a team September 24, 2024 13:59

SupaJoon added 4 commits September 24, 2024 13:25

update test data

c4c2517

update error string

6914a7b

update test data and tests

39f5917

error message + tests

be7e118

SupaJoon mentioned this pull request Sep 25, 2024

DEVPROD-5753: Update test to accommodate updated test data evergreen-ci/ui#410

Merged

minnakt reviewed Sep 26, 2024

View reviewed changes

hadjri requested changes Sep 26, 2024

View reviewed changes

SupaJoon and others added 9 commits September 27, 2024 10:27

Update graphql/query_resolver.go

e697b96

Co-authored-by: minnakt <47064971+minnakt@users.noreply.github.com>

Update graphql/resolver.go

0589ea2

Co-authored-by: minnakt <47064971+minnakt@users.noreply.github.com>

refactor

0cd6c26

make host view default req

fbe89cb

optimize looping

9ea7c53

optimize looping

571bd84

require -> assert

64f90af

update error copy

3656959

update test

95f1449

SupaJoon requested review from minnakt and hadjri October 2, 2024 18:42

hadjri approved these changes Oct 3, 2024

View reviewed changes

minnakt approved these changes Oct 3, 2024

View reviewed changes

SupaJoon and others added 3 commits October 3, 2024 15:47

Update graphql/resolver.go

d4dcddb

Co-authored-by: minnakt <47064971+minnakt@users.noreply.github.com>

cr

a61d497

Merge branch 'DEVPROD-5753' of https://github.com/SupaJoon/evergreen …

20e52da

…into DEVPROD-5753

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEVPROD-5753: Implement and apply requireHostAccess GQL directive #8337

DEVPROD-5753: Implement and apply requireHostAccess GQL directive #8337

SupaJoon commented Sep 23, 2024 •

edited

Loading

minnakt Sep 26, 2024

hadjri Sep 26, 2024

SupaJoon Sep 27, 2024 •

edited

Loading

minnakt Sep 26, 2024

SupaJoon Sep 27, 2024

hadjri Sep 26, 2024

hadjri Sep 26, 2024

hadjri Sep 26, 2024

hadjri Sep 26, 2024

hadjri Sep 26, 2024

SupaJoon Sep 27, 2024

SupaJoon commented Oct 2, 2024

minnakt Oct 3, 2024

minnakt Oct 3, 2024 •

edited

Loading

SupaJoon Oct 3, 2024

	hostIdsInterface, hasHostIds := args["hostIds"].([]interface{})
	hostIds, hasHostIds := args["hostIds"].([]string)

DEVPROD-5753: Implement and apply requireHostAccess GQL directive #8337

Are you sure you want to change the base?

DEVPROD-5753: Implement and apply requireHostAccess GQL directive #8337

Conversation

SupaJoon commented Sep 23, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon commented Oct 2, 2024

Choose a reason for hiding this comment

minnakt Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SupaJoon commented Sep 23, 2024 •

edited

Loading

SupaJoon Sep 27, 2024 •

edited

Loading

minnakt Oct 3, 2024 •

edited

Loading