Security contacts validation

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

@@ -367,6 +367,7 @@ func parseOwnersFile(oc ownersClient, path string, c github.PullRequestChange, l
 	var reviewers []string
 	var approvers []string
 	var labels []string
+	var securityContacts map[string]repoowners.ContactInfo
 
 	// by default we bind errors to line 1
 	lineNumber := 1
@@ -404,12 +405,16 @@ func parseOwnersFile(oc ownersClient, path string, c github.PullRequestChange, l
 			reviewers = append(reviewers, config.Reviewers...)
 			approvers = append(approvers, config.Approvers...)
 			labels = append(labels, config.Labels...)
+			for user, contactInfo := range config.SecurityContacts {
+				securityContacts[user] = contactInfo
+			}
 		}
 	} else {
 		// it's a SimpleConfig
 		reviewers = simple.Config.Reviewers
 		approvers = simple.Config.Approvers
 		labels = simple.Config.Labels
+		securityContacts = simple.Config.SecurityContacts
 	}
 	// Check labels against ban list
 	if sets.NewString(labels...).HasAny(bannedLabels...) {
@@ -425,6 +430,30 @@ func parseOwnersFile(oc ownersClient, path string, c github.PullRequestChange, l
 			fmt.Sprintf("No approvers defined in this root directory %s file.", filenames.Owners),
 		}, nil
 	}
+
+	// validate security contacts
+	slackRe := regexp.MustCompile(`[UW][A-Z0-9]+`)
+	for user, contactInfo := range securityContacts {
+		if &contactInfo == nil {
+			return &messageWithLine{
+				lineNumber,
+				fmt.Sprintf("security_contact %s has no contact details", user),
+			}, nil
+		}
+		if contactInfo.Email == "" && contactInfo.SlackID == "" {
+			return &messageWithLine{
+				lineNumber,
+				fmt.Sprintf("security_contact %s requires non-empty email or Slack ID", user),
+			}, nil
+		}
+		if len(contactInfo.SlackID) > 0 && slackRe.MatchString(contactInfo.SlackID) == false {
+			return &messageWithLine{
+				lineNumber,
+				fmt.Sprintf("security_contact %s's Slack ID does not match the expected format", user),
+			}, nil
+		}
+	}
+
 	owners := append(reviewers, approvers...)
 	return nil, owners
 }

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

@@ -87,6 +87,52 @@ reviewers:
 - bob
 labels:
 - lgtm
+`),
+	"slackDisplayNameInSecContacts": []byte(`approvers:
+- jdoe
+reviewers:
+- alice
+- bob
+security_contacts:
+  alice:
+    email: foo@bar.com
+    slack_id: alice
+`),
+	"slackIDInSecContacts": []byte(`approvers:
+- jdoe
+reviewers:
+- alice
+- bob
+security_contacts:
+  alice:
+    email: foo@bar.com
+    slack_id: U123ABC
+`),
+	"EmailOnlyInSecContacts": []byte(`approvers:
+- jdoe
+reviewers:
+- alice
+- bob
+security_contacts:
+  alice:
+    email: foo@bar.com
+`),
+	"slackIDOnlyInSecContacts": []byte(`approvers:
+- jdoe
+reviewers:
+- alice
+- bob
+security_contacts:
+  alice:
+    slack_id: U123ABC
+`),
+	"EmptyEntryInSecContacts": []byte(`approvers:
+- jdoe
+reviewers:
+- alice
+- bob
+security_contacts:
+  alice: {}
 `),
 	"invalidLabelsFilters": []byte(`filters:
   ".*":
@@ -443,6 +489,36 @@ func testHandle(clients localgit.Clients, t *testing.T) {
 			addedContent:        "toBeAddedAlias",
 			shouldLabel:         false,
 		},
+		{
+			name:         "security_contacts includes slack display name instead of id",
+			filesChanged: []string{"OWNERS"},
+			ownersFile:   "slackDisplayNameInSecContacts",
+			shouldLabel:  true,
+		},
+		{
+			name:         "security_contacts includes slack id",
+			filesChanged: []string{"OWNERS"},
+			ownersFile:   "slackIDInSecContacts",
+			shouldLabel:  false,
+		},
+		{
+			name:         "security_contacts includes email only",
+			filesChanged: []string{"OWNERS"},
+			ownersFile:   "EmailOnlyInSecContacts",
+			shouldLabel:  false,
+		},
+		{
+			name:         "security_contacts includes Slack ID only",
+			filesChanged: []string{"OWNERS"},
+			ownersFile:   "slackIDOnlyInSecContacts",
+			shouldLabel:  false,
+		},
+		{
+			name:         "security_contacts has empty contact info",
+			filesChanged: []string{"OWNERS"},
+			ownersFile:   "EmptyEntryInSecContacts",
+			shouldLabel:  true,
+		},
 	}
 	lg, c, err := clients()
 	if err != nil {

prow/repoowners/repoowners.go

@@ -52,12 +52,18 @@ type dirOptions struct {
 	AutoApproveUnownedSubfolders bool `json:"auto_approve_unowned_subfolders,omitempty"`
 }
 
+type ContactInfo struct {
+	Email   string `json:"email,omitempty"`
+	SlackID string `json:"slack_id,omitempty"`
+}
+
 // Config holds roles+usernames and labels for a directory considered as a unit of independent code
 type Config struct {
-	Approvers         []string `json:"approvers,omitempty"`
-	Reviewers         []string `json:"reviewers,omitempty"`
-	RequiredReviewers []string `json:"required_reviewers,omitempty"`
-	Labels            []string `json:"labels,omitempty"`
+	Approvers         []string               `json:"approvers,omitempty"`
+	Reviewers         []string               `json:"reviewers,omitempty"`
+	RequiredReviewers []string               `json:"required_reviewers,omitempty"`
+	Labels            []string               `json:"labels,omitempty"`
+	SecurityContacts  map[string]ContactInfo `json:"security_contacts,omitempty"`
 }
 
 // SimpleConfig holds options and Config applied to everything under the containing directory
@@ -244,6 +250,7 @@ type RepoOwners struct {
 	approvers         map[string]map[*regexp.Regexp]sets.String
 	reviewers         map[string]map[*regexp.Regexp]sets.String
 	requiredReviewers map[string]map[*regexp.Regexp]sets.String
+	securityContacts  map[string]map[*regexp.Regexp]map[string]ContactInfo
 	labels            map[string]map[*regexp.Regexp]sets.String
 	options           map[string]dirOptions
 
@@ -486,6 +493,7 @@ func loadOwnersFrom(baseDir string, mdYaml bool, aliases RepoAliases, dirIgnorel
 		approvers:         make(map[string]map[*regexp.Regexp]sets.String),
 		reviewers:         make(map[string]map[*regexp.Regexp]sets.String),
 		requiredReviewers: make(map[string]map[*regexp.Regexp]sets.String),
+		securityContacts:  make(map[string]map[*regexp.Regexp]map[string]ContactInfo),
 		labels:            make(map[string]map[*regexp.Regexp]sets.String),
 		options:           make(map[string]dirOptions),
 
@@ -625,6 +633,8 @@ func (o *RepoOwners) ParseSimpleConfig(path string) (SimpleConfig, error) {
 func LoadSimpleConfig(b []byte) (SimpleConfig, error) {
 	simple := new(SimpleConfig)
 	err := yaml.Unmarshal(b, simple)
+	fmt.Println(string(b))
+	fmt.Println(simple.Config.SecurityContacts)
 	return *simple, err
 }
 
@@ -697,6 +707,15 @@ func NormLogins(logins []string) sets.String {
 	return normed
 }
 
+func normSecurityContacts(contacts map[string]ContactInfo) map[string]ContactInfo {
+	normedSecurityContacts := make(map[string]ContactInfo)
+	for login, contact := range contacts {
+		normedLogin := github.NormLogin(login)
+		normedSecurityContacts[normedLogin] = contact
+	}
+	return normedSecurityContacts
+}
+
 var defaultDirOptions = dirOptions{}
 
 func (o *RepoOwners) applyConfigToPath(path string, re *regexp.Regexp, config *Config) {
@@ -718,6 +737,13 @@ func (o *RepoOwners) applyConfigToPath(path string, re *regexp.Regexp, config *C
 		}
 		o.requiredReviewers[path][re] = o.ExpandAliases(NormLogins(config.RequiredReviewers))
 	}
+	if len(config.SecurityContacts) > 0 {
+		if o.securityContacts[path] == nil {
+			o.securityContacts[path] = make(map[*regexp.Regexp]map[string]ContactInfo)
+		}
+		// don't check RepoAliases as security_contacts stores individuals' contact info
+		o.securityContacts[path][re] = normSecurityContacts(config.SecurityContacts)
+	}
 	if len(config.Labels) > 0 {
 		if o.labels[path] == nil {
 			o.labels[path] = make(map[*regexp.Regexp]sets.String)

prow/repoowners/repoowners_test.go

@@ -48,6 +48,10 @@ reviewers:
 - bob
 required_reviewers:
 - chris
+security_contacts:
+  bob:
+    email: bob@bob.com
+    slack_id: U123ABC
 labels:
 - EVERYTHING`),
 		"src/OWNERS": []byte(`approvers:
@@ -116,6 +120,11 @@ func patternAll(values ...string) map[string]sets.String {
 	return map[string]sets.String{"": sets.NewString(values...)}
 }
 
+// contactInfoAll is used to construct a default {regexp string -> values} mapping for ".*"
+func contactInfoAll(values map[string]ContactInfo) map[*regexp.Regexp]map[string]ContactInfo {
+	return map[*regexp.Regexp]map[string]ContactInfo{nil: values}
+}
+
 type cacheOptions struct {
 	hasAliases bool
 
@@ -467,6 +476,7 @@ func TestLoadRepoOwnersV2(t *testing.T) {
 
 func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 	t.Parallel()
+	expectedContactInfo := contactInfoAll(map[string]ContactInfo{"bob": ContactInfo{Email: "bob@bob.com", SlackID: "U123ABC"}})
 	tests := []struct {
 		name              string
 		mdEnabled         bool
@@ -478,6 +488,8 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 
 		expectedApprovers, expectedReviewers, expectedRequiredReviewers, expectedLabels map[string]map[string]sets.String
 
+		expectedSecurityContacts map[string]map[*regexp.Regexp]map[string]ContactInfo
+
 		expectedOptions  map[string]dirOptions
 		cacheOptions     *cacheOptions
 		expectedReusable bool
@@ -501,6 +513,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
@@ -532,6 +547,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
@@ -565,6 +583,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":             patternAll("EVERYTHING"),
 				"src/dir":      patternAll("src-code"),
@@ -603,6 +624,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
@@ -639,6 +663,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
@@ -670,6 +697,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
@@ -720,6 +750,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":             patternAll("EVERYTHING"),
 				"src/dir":      patternAll("src-code"),
@@ -755,6 +788,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":             patternAll("EVERYTHING"),
 				"src/dir":      patternAll("src-code"),
@@ -790,6 +826,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
@@ -825,6 +864,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
@@ -871,6 +913,9 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 				"":        patternAll("chris"),
 				"src/dir": patternAll("ben"),
 			},
+			expectedSecurityContacts: map[string]map[*regexp.Regexp]map[string]ContactInfo{
+				"":             expectedContactInfo,
+			},
 			expectedLabels: map[string]map[string]sets.String{
 				"":             patternAll("EVERYTHING"),
 				"src/dir":      patternAll("src-code"),
@@ -951,6 +996,11 @@ func testLoadRepoOwners(clients localgit.Clients, t *testing.T) {
 			check("reviewers", test.expectedReviewers, ro.reviewers)
 			check("required_reviewers", test.expectedRequiredReviewers, ro.requiredReviewers)
 			check("labels", test.expectedLabels, ro.labels)
+
+			if !reflect.DeepEqual(test.expectedSecurityContacts, ro.securityContacts) {
+				t.Errorf("Expected security_contacts to be:\n%#v\ngot:\n%#v.", test.expectedSecurityContacts, ro.securityContacts)
+			}
+
 			if !reflect.DeepEqual(test.expectedOptions, ro.options) {
 				t.Errorf("Expected options to be:\n%#v\ngot:\n%#v.", test.expectedOptions, ro.options)
 			}