Merge pull request #17658 from michaelmdresser/trusteduser-reason

Trigger's TrustedUser returns a reason for untrusted. Consumed by verify-owners.

prow/plugins/dco/dco.go

@@ -113,11 +113,11 @@ func filterTrustedUsers(gc gitHubClient, l *logrus.Entry, skipDCOCheckForCollabo
 	untrustedCommits := make([]github.RepositoryCommit, 0, len(allCommits))
 
 	for _, commit := range allCommits {
-		trusted, err := trigger.TrustedUser(gc, !skipDCOCheckForCollaborators, trustedOrg, commit.Author.Login, org, repo)
+		trustedResponse, err := trigger.TrustedUser(gc, !skipDCOCheckForCollaborators, trustedOrg, commit.Author.Login, org, repo)
 		if err != nil {
 			return nil, fmt.Errorf("Error checking is member trusted: %v", err)
 		}
-		if !trusted {
+		if !trustedResponse.IsTrusted {
 			l.Debugf("Member %s is not trusted", commit.Author.Login)
 			untrustedCommits = append(untrustedCommits, commit)
 		}

prow/plugins/retitle/retitle.go

@@ -74,7 +74,8 @@ func handleGenericCommentEvent(pc plugins.Agent, e github.GenericCommentEvent) e
 	)
 	return handleGenericComment(pc.GitHubClient, func(user string) (bool, error) {
 		t := pc.PluginConfig.TriggerFor(org, repo)
-		return trigger.TrustedUser(pc.GitHubClient, t.OnlyOrgMembers, t.TrustedOrg, user, org, repo)
+		trustedResponse, err := trigger.TrustedUser(pc.GitHubClient, t.OnlyOrgMembers, t.TrustedOrg, user, org, repo)
+		return trustedResponse.IsTrusted, err
 	}, pc.PluginConfig.Retitle.AllowClosedIssues, pc.Logger, e)
 }
 

prow/plugins/trigger/generic-comment.go

@@ -94,10 +94,12 @@ func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCo
 	}
 
 	// Skip untrusted users comments.
-	trusted, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, commentAuthor, org, repo)
+	trustedResponse, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, commentAuthor, org, repo)
 	if err != nil {
 		return fmt.Errorf("error checking trust of %s: %v", commentAuthor, err)
 	}
+
+	trusted := trustedResponse.IsTrusted
 	var l []github.Label
 	if !trusted {
 		// Skip untrusted PRs.

prow/plugins/trigger/pull-request.go

@@ -71,7 +71,8 @@ func handlePR(c Client, trigger plugins.Trigger, pr github.PullRequestEvent) err
 		// When a PR is opened, if the author is in the org then build it.
 		// Otherwise, ask for "/ok-to-test". There's no need to look for previous
 		// "/ok-to-test" comments since the PR was just opened!
-		member, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo)
+		trustedResponse, err := TrustedUser(c.GitHubClient, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo)
+		member := trustedResponse.IsTrusted
 		if err != nil {
 			return fmt.Errorf("could not check membership: %s", err)
 		}
@@ -307,9 +308,9 @@ func draftMsg(ghc githubClient, pr github.PullRequest) error {
 // If already known, GitHub labels should be provided to save tokens. Otherwise, it fetches them.
 func TrustedPullRequest(tprc trustedPullRequestClient, trigger plugins.Trigger, author, org, repo string, num int, l []github.Label) ([]github.Label, bool, error) {
 	// First check if the author is a member of the org.
-	if orgMember, err := TrustedUser(tprc, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo); err != nil {
+	if trustedResponse, err := TrustedUser(tprc, trigger.OnlyOrgMembers, trigger.TrustedOrg, author, org, repo); err != nil {
 		return l, false, fmt.Errorf("error checking %s for trust: %v", author, err)
-	} else if orgMember {
+	} else if trustedResponse.IsTrusted {
 		return l, true, nil
 	}
 	// Then check if PR has ok-to-test label

prow/plugins/trigger/trigger.go

@@ -38,6 +38,33 @@ const (
 	PluginName = "trigger"
 )
 
+// untrustedReason represents a combination (by ORing the appropriate consts) of reasons
+// why a user is not trusted by TrustedUser. It is used to generate messaging for users.
+type untrustedReason int
+
+const (
+	notMember untrustedReason = 1 << iota
+	notCollaborator
+	notSecondaryMember
+)
+
+// String constructs a string explaining the reason for a user's denial of trust
+// from untrustedReason as described above.
+func (u untrustedReason) String() string {
+	var response string
+	if u&notMember != 0 {
+		response += "User is not a member of the org. "
+	}
+	if u&notCollaborator != 0 {
+		response += "User is not a collaborator. "
+	}
+	if u&notSecondaryMember != 0 {
+		response += "User is not a member of the trusted secondary org. "
+	}
+	response += "Satisfy at least one of these conditions to make the user trusted."
+	return response
+}
+
 func init() {
 	plugins.RegisterGenericCommentHandler(PluginName, handleGenericCommentEvent, helpProvider)
 	plugins.RegisterPullRequestHandler(PluginName, handlePullRequest, helpProvider)
@@ -160,38 +187,61 @@ func handlePush(pc plugins.Agent, pe github.PushEvent) error {
 	return handlePE(getClient(pc), pe)
 }
 
+// TrustedUserResponse is a response from TrustedUser. It contains the boolean response for trust as well
+// a reason for denial if the user is not trusted.
+type TrustedUserResponse struct {
+	IsTrusted bool
+	// Reason contains the reason that a user is not trusted if IsTrusted is false
+	Reason string
+}
+
 // TrustedUser returns true if user is trusted in repo.
 // Trusted users are either repo collaborators, org members or trusted org members.
-func TrustedUser(ghc trustedUserClient, onlyOrgMembers bool, trustedOrg, user, org, repo string) (bool, error) {
+func TrustedUser(ghc trustedUserClient, onlyOrgMembers bool, trustedOrg, user, org, repo string) (TrustedUserResponse, error) {
+	errorResponse := TrustedUserResponse{IsTrusted: false}
+	okResponse := TrustedUserResponse{IsTrusted: true}
+
 	// First check if user is a collaborator, assuming this is allowed
 	if !onlyOrgMembers {
 		if ok, err := ghc.IsCollaborator(org, repo, user); err != nil {
-			return false, fmt.Errorf("error in IsCollaborator: %v", err)
+			return errorResponse, fmt.Errorf("error in IsCollaborator: %v", err)
 		} else if ok {
-			return true, nil
+			return okResponse, nil
 		}
 	}
 
 	// TODO(fejta): consider dropping support for org checks in the future.
 
 	// Next see if the user is an org member
 	if member, err := ghc.IsMember(org, user); err != nil {
-		return false, fmt.Errorf("error in IsMember(%s): %v", org, err)
+		return errorResponse, fmt.Errorf("error in IsMember(%s): %v", org, err)
 	} else if member {
-		return true, nil
+		return okResponse, nil
 	}
 
-	// Determine if there is a second org to check
+	// Determine if there is a second org to check. If there is no secondary org or they are the same, the result
+	// is the same because the user already failed the check for the primary org.
 	if trustedOrg == "" || trustedOrg == org {
-		return false, nil // No trusted org and/or it is the same
+		// the if/else is only to improve error messaging
+		if onlyOrgMembers {
+			return TrustedUserResponse{IsTrusted: false, Reason: notMember.String()}, nil // No trusted org and/or it is the same
+		}
+		return TrustedUserResponse{IsTrusted: false, Reason: (notMember | notCollaborator).String()}, nil // No trusted org and/or it is the same
 	}
 
 	// Check the second trusted org.
 	member, err := ghc.IsMember(trustedOrg, user)
 	if err != nil {
-		return false, fmt.Errorf("error in IsMember(%s): %v", trustedOrg, err)
+		return errorResponse, fmt.Errorf("error in IsMember(%s): %v", trustedOrg, err)
+	} else if member {
+		return okResponse, nil
+	}
+
+	// the if/else is only to improve error messaging
+	if onlyOrgMembers {
+		return TrustedUserResponse{IsTrusted: false, Reason: (notMember | notSecondaryMember).String()}, nil
 	}
-	return member, nil
+	return TrustedUserResponse{IsTrusted: false, Reason: (notMember | notSecondaryMember | notCollaborator).String()}, nil
 }
 
 func skippedStatusFor(context string) github.Status {

prow/plugins/trigger/trigger_test.go

@@ -270,6 +270,7 @@ func TestTrustedUser(t *testing.T) {
 		repo string
 
 		expectedTrusted bool
+		expectedReason  string
 	}{
 		{
 			name:            "user is member of trusted org",
@@ -302,6 +303,7 @@ func TestTrustedUser(t *testing.T) {
 			org:             "kubernetes",
 			repo:            "kubernetes",
 			expectedTrusted: false,
+			expectedReason:  (notMember).String(),
 		},
 		{
 			name:            "user is trusted org member",
@@ -319,6 +321,27 @@ func TestTrustedUser(t *testing.T) {
 			org:             "kubernetes",
 			repo:            "kubernetes",
 			expectedTrusted: false,
+			expectedReason:  (notMember | notCollaborator).String(),
+		},
+		{
+			name:            "user is not org member or trusted org member",
+			onlyOrgMembers:  false,
+			trustedOrg:      "kubernetes-sigs",
+			user:            "test-2",
+			org:             "kubernetes",
+			repo:            "kubernetes",
+			expectedTrusted: false,
+			expectedReason:  (notMember | notCollaborator | notSecondaryMember).String(),
+		},
+		{
+			name:            "user is not org member or trusted org member, onlyOrgMembers true",
+			onlyOrgMembers:  true,
+			trustedOrg:      "kubernetes-sigs",
+			user:            "test-2",
+			org:             "kubernetes",
+			repo:            "kubernetes",
+			expectedTrusted: false,
+			expectedReason:  (notMember | notSecondaryMember).String(),
 		},
 	}
 
@@ -331,12 +354,15 @@ func TestTrustedUser(t *testing.T) {
 				Collaborators: []string{"test-collaborator"},
 			}
 
-			trusted, err := TrustedUser(fc, tc.onlyOrgMembers, tc.trustedOrg, tc.user, tc.org, tc.repo)
+			trustedResponse, err := TrustedUser(fc, tc.onlyOrgMembers, tc.trustedOrg, tc.user, tc.org, tc.repo)
 			if err != nil {
 				t.Errorf("For case %s, didn't expect error from TrustedUser: %v", tc.name, err)
 			}
-			if trusted != tc.expectedTrusted {
-				t.Errorf("For case %s, expect result: %v, but got: %v", tc.name, tc.expectedTrusted, trusted)
+			if trustedResponse.IsTrusted != tc.expectedTrusted {
+				t.Errorf("For case %s, expect trusted: %v, but got: %v", tc.name, tc.expectedTrusted, trustedResponse.IsTrusted)
+			}
+			if trustedResponse.Reason != tc.expectedReason {
+				t.Errorf("For case %s, expect trusted reason: %v, but got: %v", tc.name, tc.expectedReason, trustedResponse.Reason)
 			}
 		})
 	}

prow/plugins/verify-owners/verify-owners.go

@@ -41,14 +41,19 @@ import (
 
 const (
 	// PluginName defines this plugin's registered name.
-	PluginName                    = "verify-owners"
-	ownersFileName                = "OWNERS"
-	ownersAliasesFileName         = "OWNERS_ALIASES"
-	nonCollaboratorResponseFormat = `The following users are mentioned in %s file(s) but are not members of the %s org.
-
-Once all users have been added as [members](%s) of the org, you can trigger verification by writing ` + "`/verify-owners`" + ` in a comment.`
+	PluginName              = "verify-owners"
+	ownersFileName          = "OWNERS"
+	ownersAliasesFileName   = "OWNERS_ALIASES"
+	untrustedResponseFormat = `The following users are mentioned in %s file(s) but are untrusted for the following reasons. One way to make the user trusted is to add them as [members](%s) of the %s org. You can then trigger verification by writing ` + "`/verify-owners`" + ` in a comment.`
 )
 
+type nonTrustedReasons struct {
+	// files is a list of files they are being added in
+	files []string
+	// triggerReason is the reason that trigger's TrustedUser responds with for a failed trust check
+	triggerReason string
+}
+
 var (
 	verifyOwnersRe = regexp.MustCompile(`(?mi)^/verify-owners\s*$`)
 )
@@ -324,7 +329,7 @@ func handle(ghc githubClient, gc git.ClientFactory, roc repoownersClient, log *l
 
 		// prune old comments before adding a new one
 		cp.PruneComments(func(comment github.IssueComment) bool {
-			return strings.Contains(comment.Body, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org, triggerConfig.JoinOrgURL))
+			return strings.Contains(comment.Body, fmt.Sprintf(untrustedResponseFormat, ownersFileName, triggerConfig.JoinOrgURL, org))
 		})
 		if err := ghc.CreateComment(org, repo, number, markdownFriendlyComment(org, triggerConfig.JoinOrgURL, nonTrustedUsers)); err != nil {
 			log.WithError(err).Errorf("Could not create comment for listing non-collaborators in %s files", ownersFileName)
@@ -338,7 +343,7 @@ func handle(ghc githubClient, gc git.ClientFactory, roc repoownersClient, log *l
 			return fmt.Errorf("failed removing %s label: %v", labels.InvalidOwners, err)
 		}
 		cp.PruneComments(func(comment github.IssueComment) bool {
-			return strings.Contains(comment.Body, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org, triggerConfig.JoinOrgURL))
+			return strings.Contains(comment.Body, fmt.Sprintf(untrustedResponseFormat, ownersFileName, triggerConfig.JoinOrgURL, org))
 		})
 	}
 
@@ -411,23 +416,24 @@ func parseOwnersFile(oc ownersClient, path string, c github.PullRequestChange, l
 	return nil, owners
 }
 
-func markdownFriendlyComment(org, joinOrgURL string, nonTrustedUsers map[string][]string) string {
+func markdownFriendlyComment(org, joinOrgURL string, nonTrustedUsers map[string]nonTrustedReasons) string {
 	var commentLines []string
-	commentLines = append(commentLines, fmt.Sprintf(nonCollaboratorResponseFormat, ownersFileName, org, joinOrgURL))
+	commentLines = append(commentLines, fmt.Sprintf(untrustedResponseFormat, ownersFileName, joinOrgURL, org))
 
-	for user, ownersFiles := range nonTrustedUsers {
+	for user, reasons := range nonTrustedUsers {
 		commentLines = append(commentLines, fmt.Sprintf("- %s", user))
-		for _, filename := range ownersFiles {
+		commentLines = append(commentLines, fmt.Sprintf("  - %s", reasons.triggerReason))
+		for _, filename := range reasons.files {
 			commentLines = append(commentLines, fmt.Sprintf("  - %s", filename))
 		}
 	}
 	return strings.Join(commentLines, "\n")
 }
 
-func nonTrustedUsersInOwnersAliases(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, dir, patch string, ownerAliasesModified, skipTrustedUserCheck bool) (map[string][]string, sets.String, repoowners.RepoAliases, error) {
+func nonTrustedUsersInOwnersAliases(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, dir, patch string, ownerAliasesModified, skipTrustedUserCheck bool) (map[string]nonTrustedReasons, sets.String, repoowners.RepoAliases, error) {
 	repoAliases := make(repoowners.RepoAliases)
-	// nonTrustedUsers is a map of non-trusted users to the list of files they are being added in
-	nonTrustedUsers := map[string][]string{}
+	// nonTrustedUsers is a map of non-trusted users to the reasons they were not trusted
+	nonTrustedUsers := map[string]nonTrustedReasons{}
 	trustedUsers := sets.String{}
 	var err error
 
@@ -458,7 +464,7 @@ func nonTrustedUsersInOwnersAliases(ghc githubClient, log *logrus.Entry, trigger
 	return nonTrustedUsers, trustedUsers, repoAliases, nil
 }
 
-func nonTrustedUsersInOwners(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, patch, fileName string, owners []string, nonTrustedUsers map[string][]string, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string][]string, error) {
+func nonTrustedUsersInOwners(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, org, repo, patch, fileName string, owners []string, nonTrustedUsers map[string]nonTrustedReasons, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string]nonTrustedReasons, error) {
 	var err error
 	for _, owner := range owners {
 		// ignore if owner is an alias
@@ -476,7 +482,8 @@ func nonTrustedUsersInOwners(ghc githubClient, log *logrus.Entry, triggerConfig
 
 // checkIfTrustedUser looks for newly addded owners by checking if they are in the patch
 // and then checks if the owner is a trusted user.
-func checkIfTrustedUser(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, owner, patch, fileName, org, repo string, nonTrustedUsers map[string][]string, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string][]string, error) {
+// returns a map from user to reasons for not being trusted
+func checkIfTrustedUser(ghc githubClient, log *logrus.Entry, triggerConfig plugins.Trigger, owner, patch, fileName, org, repo string, nonTrustedUsers map[string]nonTrustedReasons, trustedUsers sets.String, repoAliases repoowners.RepoAliases) (map[string]nonTrustedReasons, error) {
 	// cap the number of checks to avoid exhausting tokens in case of large OWNERS refactors.
 	if len(nonTrustedUsers)+trustedUsers.Len() > 50 {
 		return nonTrustedUsers, nil
@@ -488,33 +495,42 @@ func checkIfTrustedUser(ghc githubClient, log *logrus.Entry, triggerConfig plugi
 	}
 
 	// if we already flagged the owner for the current file, return early
-	if ownersFiles, ok := nonTrustedUsers[owner]; ok {
-		for _, file := range ownersFiles {
+	if reasons, ok := nonTrustedUsers[owner]; ok {
+		for _, file := range reasons.files {
 			if file == fileName {
 				return nonTrustedUsers, nil
 			}
 		}
-		nonTrustedUsers[owner] = append(ownersFiles, fileName)
+		// have to separate assignment from map update due to map implementation (see "index expressions")
+		reasons.files = append(reasons.files, fileName)
+		nonTrustedUsers[owner] = reasons
 		return nonTrustedUsers, nil
 	}
 
-	isTrustedUser := trustedUsers.Has(owner)
-	if !isTrustedUser {
-		var err error
-		isTrustedUser, err = trigger.TrustedUser(ghc, triggerConfig.OnlyOrgMembers, triggerConfig.TrustedOrg, owner, org, repo)
+	isAlreadyTrusted := trustedUsers.Has(owner)
+	var err error
+	var triggerTrustedResponse trigger.TrustedUserResponse
+	if !isAlreadyTrusted {
+		triggerTrustedResponse, err = trigger.TrustedUser(ghc, triggerConfig.OnlyOrgMembers, triggerConfig.TrustedOrg, owner, org, repo)
 		if err != nil {
 			return nonTrustedUsers, err
 		}
 	}
 
-	if isTrustedUser {
+	if !isAlreadyTrusted && triggerTrustedResponse.IsTrusted {
 		trustedUsers.Insert(owner)
-	} else {
-		if ownersFiles, ok := nonTrustedUsers[owner]; ok {
-			nonTrustedUsers[owner] = append(ownersFiles, fileName)
+	} else if !isAlreadyTrusted && !triggerTrustedResponse.IsTrusted {
+		if reasons, ok := nonTrustedUsers[owner]; ok {
+			reasons.triggerReason = triggerTrustedResponse.Reason
+			nonTrustedUsers[owner] = reasons
 		} else {
-			nonTrustedUsers[owner] = []string{fileName}
+			nonTrustedUsers[owner] = nonTrustedReasons{
+				// ensure that files is initialized to avoid nil pointer
+				files:         []string{},
+				triggerReason: triggerTrustedResponse.Reason,
+			}
 		}
 	}
+
 	return nonTrustedUsers, nil
 }