Repository: palantir/policy-bot
Pull request #297 (2021-05-11 04:11:49)

Fix conflict between direct and inherited perms
Previously, if the aggregate permission for a collaborator was higher than a permission granted directly, the direct permission was discarded and the user would not be selected for reviews. This was particularly bad for org admins who are also members of a team with lower permissions.

In the revised model, return both the aggregate permission and additional lower permissions that are granted directly so that they may still be used for reviewer assignment. However, for the approval to actually count, the rule must specify the minimum permission and all higher permissions that selected users may have.

For example, if a team has `maintain` permission and a member of the team is an org admin, that member's approval will only count if the rule requires both `admin` and `maintain` permission.

This is making me think that the `permissions` key should be interpreted as a minimum permission for approval, but as an exact list of permissions for requests. That way, you can list `maintain` and both maintainers and admins can approve but if a review is requested, it will only request people who explicitly and directly have `maintain` permission. While it sounds more complicated, I think the actual result will be less confusing - exact permissions don't make a lot of sense for approval.

I still need to add some tests for the new behavior.


Patch:
diff --git a/README.md b/README.md
index 1353592..e8e313e 100644
--- a/README.md
+++ b/README.md
@@ -343,10 +343,14 @@ requires:
   organizations: ["org1", "org2"]
   teams: ["org1/team1", "org2/team2"]
 
-  # A user must have a permission in this list for their approval to count for
-  # this rule. Valid permissions are "admin", "maintain", "write", "triage",
-  # and "read".
-  permissions: ["admin", "maintain", "write"]
+  # A user must have at least the minimum permission in this list for their
+  # approval to count for this rule. Valid permissions are "admin", "maintain",
+  # "write", "triage", and "read".
+  #
+  # Specifying more than one permission is only useful to control which users
+  # or teams are selected for review requests. See the documentation on review
+  # requests for details.
+  permissions: ["write"]
 
   # Deprecated: use 'permissions: ["admin"]'
   #
@@ -551,11 +555,14 @@ The set of requested reviewers will not include the author of the pull request o
 users who are not collaborators on the repository.
 
 When requesting reviews for rules that use repository permissions to select
-approvers, only users who are either direct collaborators or members of
-repository teams are eligible for review selection. For example, if a rule can
-be approved by any user with `admin` permission, only direct or team admins
-will be selected for review. Users who inherit repository `admin` permissions
-as organization owners are not selected.
+approvers, only users who are direct collaborators or members of
+repository teams are eligible for review selection. The users or their teams
+must be granted an exact permission specified in the `permissions` list of the
+rule.
+
+For example, if a rule can be approved by any user with `admin` permission,
+only direct or team admins are selected for review. Users who inherit
+repository `admin` permissions as organization owners are not selected.
 
 #### Automatically Requesting Reviewers Example
 
diff --git a/policy/common/actor.go b/policy/common/actor.go
index 347e61a..60fc7f1 100644
--- a/policy/common/actor.go
+++ b/policy/common/actor.go
@@ -94,7 +94,7 @@ func (a *Actors) IsActor(ctx context.Context, prctx pull.Context, user string) (
 	}
 
 	for _, p := range perms {
-		if userPerm == p {
+		if userPerm >= p {
 			return true, nil
 		}
 	}
diff --git a/policy/reviewer/reviewer.go b/policy/reviewer/reviewer.go
index 8585378..56d0687 100644
--- a/policy/reviewer/reviewer.go
+++ b/policy/reviewer/reviewer.go
@@ -249,8 +249,10 @@ func selectUserReviewers(ctx context.Context, prctx pull.Context, selection *Sel
 	if len(result.ReviewRequestRule.Permissions) > 0 {
 		logger.Debug().Msg("Selecting from collaborators by permission for review")
 		for _, c := range collaborators {
-			if c.PermissionViaRepo && requestsPermission(result, c.Permission) {
-				allUsers[c.Name] = struct{}{}
+			for _, cp := range c.Permissions {
+				if cp.ViaRepo && requestsPermission(result, cp.Permission) {
+					allUsers[c.Name] = struct{}{}
+				}
 			}
 		}
 	}
diff --git a/pull/context.go b/pull/context.go
index 77beada..16d164b 100644
--- a/pull/context.go
+++ b/pull/context.go
@@ -220,10 +220,14 @@ type Reviewer struct {
 }
 
 type Collaborator struct {
-	Name       string
+	Name        string
+	Permissions []CollaboratorPermission
+}
+
+type CollaboratorPermission struct {
 	Permission Permission
 
 	// True if Permission is granted by a direct or team association with the
 	// repository. If false, the permisssion is granted by the organization.
-	PermissionViaRepo bool
+	ViaRepo bool
 }
diff --git a/pull/github.go b/pull/github.go
index e061b57..425c3bb 100644
--- a/pull/github.go
+++ b/pull/github.go
@@ -388,8 +388,10 @@ func (ghc *GitHubContext) RepositoryCollaborators() ([]*Collaborator, error) {
 				}
 
 				collaborators = append(collaborators, &Collaborator{
-					Name:       name,
-					Permission: p,
+					Name: name,
+					Permissions: []CollaboratorPermission{
+						{Permission: p},
+					},
 				})
 			}
 			if !q.Repository.AllCollaborators.PageInfo.UpdateCursor(qvars, "allCursor") {
@@ -421,20 +423,35 @@ func (ghc *GitHubContext) RepositoryCollaborators() ([]*Collaborator, error) {
 			}
 		}
 
-		isPermissionViaRepo := func(c *Collaborator) bool {
-			if directPerms[c.Name] >= c.Permission {
-				return true
+		fillPermissions := func(c *Collaborator) {
+			overall := c.Permissions[0].Permission // from above, every collaborator has at least one permission
+
+			if dp, ok := directPerms[c.Name]; ok {
+				if dp >= overall {
+					c.Permissions[0].ViaRepo = true
+				} else if dp > PermissionNone {
+					c.Permissions = append(c.Permissions, CollaboratorPermission{
+						Permission: dp,
+						ViaRepo:    true,
+					})
+				}
 			}
+
 			for _, team := range teamMembership[c.Name] {
-				if teamPerms[team] >= c.Permission {
-					return true
+				tp := teamPerms[team]
+				if tp >= overall {
+					c.Permissions[0].ViaRepo = true
+				} else if tp > PermissionNone {
+					c.Permissions = append(c.Permissions, CollaboratorPermission{
+						Permission: tp,
+						ViaRepo:    true,
+					})
 				}
 			}
-			return false
 		}
 
 		for _, c := range collaborators {
-			c.PermissionViaRepo = isPermissionViaRepo(c)
+			fillPermissions(c)
 		}
 		ghc.collaborators = collaborators
 	}


Test patch:
diff --git a/policy/common/actor_test.go b/policy/common/actor_test.go
index b2ea648..5db83f7 100644
--- a/policy/common/actor_test.go
+++ b/policy/common/actor_test.go
@@ -34,8 +34,18 @@ func TestIsActor(t *testing.T) {
 			"mhaypenny": {"cool-org", "regular-org"},
 		},
 		CollaboratorsValue: []*pull.Collaborator{
-			{Name: "mhaypenny", Permission: pull.PermissionAdmin},
-			{Name: "jstrawnickel", Permission: pull.PermissionWrite},
+			{
+				Name: "mhaypenny",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionAdmin},
+				},
+			},
+			{
+				Name: "jstrawnickel",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionWrite},
+				},
+			},
 		},
 	}
 
@@ -90,13 +100,13 @@ func TestIsActor(t *testing.T) {
 		a := &Actors{WriteCollaborators: true}
 
 		assertActor(t, a, "jstrawnickel")
-		assertNotActor(t, a, "mhaypenny")
+		assertActor(t, a, "mhaypenny")
 		assertNotActor(t, a, "ttest")
 	})
 
 	t.Run("permissions", func(t *testing.T) {
 		a := &Actors{
-			Permissions: []pull.Permission{pull.PermissionAdmin, pull.PermissionWrite},
+			Permissions: []pull.Permission{pull.PermissionTriage},
 		}
 
 		assertActor(t, a, "mhaypenny")
diff --git a/policy/reviewer/reviewer_test.go b/policy/reviewer/reviewer_test.go
index 1049686..bbc3315 100644
--- a/policy/reviewer/reviewer_test.go
+++ b/policy/reviewer/reviewer_test.go
@@ -239,9 +239,11 @@ func TestSelectReviewers_UserPermission(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, selection.Teams, 0, "policy should request no teams")
 
-	require.Len(t, selection.Users, 2, "policy should request two users")
+	require.Len(t, selection.Users, 4, "policy should request two users")
 	require.Contains(t, selection.Users, "maintainer", "maintainer selected")
 	require.Contains(t, selection.Users, "triager", "triager selected")
+	require.Contains(t, selection.Users, "org-owner-team-maintainer", "triager selected")
+	require.Contains(t, selection.Users, "direct-write-team-maintainer", "triager selected")
 }
 
 func TestSelectReviewers_TeamPermission(t *testing.T) {
@@ -373,18 +375,94 @@ func makeContext() pull.Context {
 			"review-approver":       {"everyone", "even-cooler-org"},
 		},
 		CollaboratorsValue: []*pull.Collaborator{
-			{Name: "mhaypenny", Permission: pull.PermissionAdmin},
-			{Name: "org-owner", Permission: pull.PermissionAdmin},
-			{Name: "user-team-admin", Permission: pull.PermissionAdmin, PermissionViaRepo: true},
-			{Name: "user-direct-admin", Permission: pull.PermissionAdmin, PermissionViaRepo: true},
-			{Name: "user-team-write", Permission: pull.PermissionWrite, PermissionViaRepo: true},
-			{Name: "contributor-committer", Permission: pull.PermissionWrite},
-			{Name: "contributor-author", Permission: pull.PermissionWrite},
-			{Name: "review-approver", Permission: pull.PermissionWrite},
-			{Name: "maintainer", Permission: pull.PermissionMaintain, PermissionViaRepo: true},
-			{Name: "indirect-maintainer", Permission: pull.PermissionMaintain}, // note: currently not possible in GitHub
-			{Name: "triager", Permission: pull.PermissionTriage, PermissionViaRepo: true},
-			{Name: "indirect-triager", Permission: pull.PermissionTriage}, // note: currently not possible in GitHub
+			{
+				Name: "mhaypenny",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionAdmin},
+				},
+			},
+			{
+				Name: "org-owner",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionAdmin},
+				},
+			},
+			{
+				Name: "user-team-admin",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionAdmin, ViaRepo: true},
+				},
+			},
+			{
+				Name: "user-direct-admin",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionAdmin, ViaRepo: true},
+				},
+			},
+			{
+				Name: "user-team-write",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionWrite, ViaRepo: true},
+				},
+			},
+			{
+				Name: "contributor-committer",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionWrite},
+				},
+			},
+			{
+				Name: "contributor-author",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionWrite},
+				},
+			},
+			{
+				Name: "review-approver",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionWrite},
+				},
+			},
+			{
+				Name: "maintainer",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionMaintain, ViaRepo: true},
+				},
+			},
+			{
+				// note: currently not possible in GitHub
+				Name: "indirect-maintainer",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionMaintain},
+				},
+			},
+			{
+				Name: "triager",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionTriage, ViaRepo: true},
+				},
+			},
+			{
+				// note: currently not possible in GitHub
+				Name: "indirect-triager",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionTriage},
+				},
+			},
+			{
+				Name: "org-owner-team-maintainer",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionAdmin, ViaRepo: false},
+					{Permission: pull.PermissionMaintain, ViaRepo: true},
+				},
+			},
+			{
+				Name: "direct-write-team-maintainer",
+				Permissions: []pull.CollaboratorPermission{
+					{Permission: pull.PermissionMaintain, ViaRepo: true},
+					{Permission: pull.PermissionWrite, ViaRepo: true},
+				},
+			},
 		},
 		TeamsValue: map[string]pull.Permission{
 			"team-write":    pull.PermissionWrite,
diff --git a/pull/github_test.go b/pull/github_test.go
index fb90bc7..e652444 100644
--- a/pull/github_test.go
+++ b/pull/github_test.go
@@ -493,38 +493,58 @@ func TestRepositoryCollaborators(t *testing.T) {
 	collaborators, err := ctx.RepositoryCollaborators()
 	require.NoError(t, err)
 
-	require.Len(t, collaborators, 6, "incorrect number of collaborators")
+	require.Len(t, collaborators, 8, "incorrect number of collaborators")
 	sort.Slice(collaborators, func(i, j int) bool { return collaborators[i].Name < collaborators[j].Name })
 
 	c0 := collaborators[0]
 	assert.Equal(t, "direct-admin", c0.Name)
-	assert.Equal(t, PermissionAdmin, c0.Permission)
-	assert.Equal(t, true, c0.PermissionViaRepo)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionAdmin, ViaRepo: true},
+	}, c0.Permissions)
 
 	c1 := collaborators[1]
 	assert.Equal(t, "direct-triage", c1.Name)
-	assert.Equal(t, PermissionTriage, c1.Permission)
-	assert.Equal(t, true, c1.PermissionViaRepo)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionTriage, ViaRepo: true},
+	}, c1.Permissions)
 
 	c2 := collaborators[2]
-	assert.Equal(t, "org-owner", c2.Name)
-	assert.Equal(t, PermissionAdmin, c2.Permission)
-	assert.Equal(t, false, c2.PermissionViaRepo)
+	assert.Equal(t, "direct-write-team-maintain", c2.Name)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionMaintain, ViaRepo: true},
+		{Permission: PermissionWrite, ViaRepo: true},
+	}, c2.Permissions)
 
 	c3 := collaborators[3]
-	assert.Equal(t, "org-read", c3.Name)
-	assert.Equal(t, PermissionRead, c3.Permission)
-	assert.Equal(t, false, c3.PermissionViaRepo)
+	assert.Equal(t, "org-owner", c3.Name)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionAdmin, ViaRepo: false},
+	}, c3.Permissions)
 
 	c4 := collaborators[4]
-	assert.Equal(t, "team-admin", c4.Name)
-	assert.Equal(t, PermissionAdmin, c4.Permission)
-	assert.Equal(t, true, c4.PermissionViaRepo)
+	assert.Equal(t, "org-owner-team-maintain", c4.Name)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionAdmin, ViaRepo: false},
+		{Permission: PermissionMaintain, ViaRepo: true},
+	}, c4.Permissions)
 
 	c5 := collaborators[5]
-	assert.Equal(t, "team-maintain", c5.Name)
-	assert.Equal(t, PermissionMaintain, c5.Permission)
-	assert.Equal(t, true, c5.PermissionViaRepo)
+	assert.Equal(t, "org-read", c5.Name)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionRead, ViaRepo: false},
+	}, c5.Permissions)
+
+	c6 := collaborators[6]
+	assert.Equal(t, "team-admin", c6.Name)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionAdmin, ViaRepo: true},
+	}, c6.Permissions)
+
+	c7 := collaborators[7]
+	assert.Equal(t, "team-maintain", c7.Name)
+	assert.Equal(t, []CollaboratorPermission{
+		{Permission: PermissionMaintain, ViaRepo: true},
+	}, c7.Permissions)
 }
 
 func makeContext(t *testing.T, rp *ResponsePlayer, pr *github.PullRequest) Context {
diff --git a/pull/pulltest/context.go b/pull/pulltest/context.go
index a53ddd3..c9241c6 100644
--- a/pull/pulltest/context.go
+++ b/pull/pulltest/context.go
@@ -163,7 +163,7 @@ func (c *Context) CollaboratorPermission(user string) (pull.Permission, error) {
 	}
 	for _, collab := range c.CollaboratorsValue {
 		if collab.Name == user {
-			return collab.Permission, nil
+			return collab.Permissions[0].Permission, nil
 		}
 	}
 	return pull.PermissionNone, nil
diff --git a/pull/testdata/responses/repo_collaborators.yml b/pull/testdata/responses/repo_collaborators.yml
index 23ca9cf..a8443fd 100644
--- a/pull/testdata/responses/repo_collaborators.yml
+++ b/pull/testdata/responses/repo_collaborators.yml
@@ -6,7 +6,7 @@
         "repository": {
           "direct": {
             "pageInfo": {
-              "endCursor": "2",
+              "endCursor": "3",
               "hasNextPage": false
             },
             "edges": [
@@ -15,6 +15,9 @@
               },
               {
                 "permission": "TRIAGE"
+              },
+              {
+                "permission": "WRITE"
               }
             ],
             "nodes": [
@@ -25,12 +28,16 @@
               {
                 "__typename": "User",
                 "login": "direct-triage"
+              },
+              {
+                "__typename": "User",
+                "login": "direct-write-team-maintain"
               }
             ]
           },
           "all": {
             "pageInfo": {
-              "endCursor": "3",
+              "endCursor": "4",
               "hasNextPage": true
             },
             "edges": [
@@ -40,6 +47,9 @@
               {
                 "permission": "READ"
               },
+              {
+                "permission": "ADMIN"
+              },
               {
                 "permission": "ADMIN"
               }
@@ -53,6 +63,10 @@
                 "__typename": "User",
                 "login": "org-read"
               },
+              {
+                "__typename": "User",
+                "login": "org-owner-team-maintain"
+              },
               {
                 "__typename": "User",
                 "login": "direct-admin"
@@ -70,7 +84,7 @@
         "repository": {
           "direct": {
             "pageInfo": {
-              "endCursor": "2",
+              "endCursor": "3",
               "hasNextPage": false
             },
             "edges": [],
@@ -78,7 +92,7 @@
           },
           "all": {
             "pageInfo": {
-              "endCursor": "6",
+              "endCursor": "8",
               "hasNextPage": false
             },
             "edges": [
@@ -88,6 +102,9 @@
               {
                 "permission": "MAINTAIN"
               },
+              {
+                "permission": "MAINTAIN"
+              },
               {
                 "permission": "ADMIN"
               }
@@ -97,6 +114,10 @@
                 "__typename": "User",
                 "login": "direct-triage"
               },
+              {
+                "__typename": "User",
+                "login": "direct-write-team-maintain"
+              },
               {
                 "__typename": "User",
                 "login": "team-maintain"
diff --git a/pull/testdata/responses/repo_team_members_maintainers.yml b/pull/testdata/responses/repo_team_members_maintainers.yml
index ebb34b0..7d662a0 100644
--- a/pull/testdata/responses/repo_team_members_maintainers.yml
+++ b/pull/testdata/responses/repo_team_members_maintainers.yml
@@ -4,6 +4,14 @@
       {
         "login": "team-maintain",
         "type": "User"
+      },
+      {
+        "login": "org-owner-team-maintain",
+        "type": "User"
+      },
+      {
+        "login": "direct-write-team-maintain",
+        "type": "User"
       }
     ]