Repository: openshift-pipelines/pipelines-as-code
Pull request #43 (2021-05-07 06:51:17)

Only run a PR if the sender is part of the org or repo owner
If PR sender is a public member (because we can't otherwise with github
app to have that info) on the organization where the pr has been sent.

If user that submitted the PR is owner of the repo then allow it too.

There is going to be a bit more logic here for OWNERS but let not go too
ahead of ourselves.

partial addressing #39

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>



Patch:
diff --git a/README.md b/README.md
index 86ac433e..684cc705 100644
--- a/README.md
+++ b/README.md
@@ -40,7 +40,7 @@ Pipelines as code leverage on this technologies :
   - Use GitHUB blobs and objects API to get configuration files directly.
     (instead of checking the repo locally)
 
-## Usage
+## User usage
 
 ### GitHub apps Configuration
 
@@ -104,7 +104,9 @@ into the same namespace are where we want to execute them.
   the same namespace without risk of conflicts.
 
 - Everything that runs your pipeline should be self contained inside the
-  `.tekton/` directory including the tasks. Optionally it supports a file called
+  `.tekton/` directory including the tasks. If pipelines as code cannot resolve the referenced tasks in the `Pipeline` or `PipelineSpec` it will fails.
+
+- Optionally `Pipelines as Code` supports a configuration file called
   `tekton.yaml` which allows you to integrate *remote* tasks directly on your
   pipeline. For example if you have this :
 
@@ -128,10 +130,13 @@ into the same namespace are where we want to execute them.
 
 ### Running the Pipeline
 
-You simply send your Pull Request and Pipelines as Run will apply your pipeline
-and run it.
+User create a Pull Request.
+
+If the user sending the Pull Request is not the owner of the repository or not a public member of the organization where the repository belong to, `Pipeline as Code` will not run.
+
+If the user is allowed, `Pipelines as Code` will start creating the `PipelineRun` in the target user namespace.
 
-You can follow the execution of your pipeline with the
+The user can follow the execution of your pipeline with the
 [tkn](https://github.com/tektoncd/cli) cli :
 
 ```bash
diff --git a/TODO.md b/TODO.md
index 7433bc3c..1f80230d 100644
--- a/TODO.md
+++ b/TODO.md
@@ -1,4 +1,6 @@
-# Pipelines as Code - TODO List
+# Pipelines as Code 
+
+See GitHub issue for more details
 
 ## Installation
 
diff --git a/pkg/cmd/pipelineascode/pipelineascode.go b/pkg/cmd/pipelineascode/pipelineascode.go
index e90e47a7..f60840eb 100644
--- a/pkg/cmd/pipelineascode/pipelineascode.go
+++ b/pkg/cmd/pipelineascode/pipelineascode.go
@@ -52,6 +52,7 @@ func Command(p cli.Params) *cobra.Command {
 	cmd.Flags().StringVarP(&opts.RunInfo.Repository, "webhook-repository", "", os.Getenv("PAC_REPOSITORY_NAME"), "Repository Name of the repository to test")
 	cmd.Flags().StringVarP(&opts.RunInfo.DefaultBranch, "webhook-defaultbranch", "", os.Getenv("PAC_DEFAULTBRANCH"), "DefaultBranch of the repository to test")
 	cmd.Flags().StringVarP(&opts.RunInfo.Branch, "webhook-target-branch", "", os.Getenv("PAC_BRANCH"), "Target branch of the repository to test")
+	cmd.Flags().StringVarP(&opts.RunInfo.Sender, "webhook-sender", "", os.Getenv("PAC_Sender"), "Sender for the commit/pr")
 	cmd.Flags().StringVarP(&opts.RunInfo.URL, "webhook-url", "", os.Getenv("PAC_URL"), "URL of the repository to test")
 
 	cmd.Flags().StringVarP(&opts.Payload, "payload", "", os.Getenv("PAC_PAYLOAD"), "The payload from webhook")
diff --git a/pkg/pipelineascode/acl.go b/pkg/pipelineascode/acl.go
new file mode 100644
index 00000000..6fca52dd
--- /dev/null
+++ b/pkg/pipelineascode/acl.go
@@ -0,0 +1,29 @@
+package pipelineascode
+
+import (
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/cli"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/webvcs"
+)
+
+// aclCheck check if we are allowed to run pipeline
+func aclCheck(cs *cli.Clients, runinfo *webvcs.RunInfo) (bool, error) {
+	// If the user who submitted the PR is the same as the Owner of the repo
+	// then allow it.
+	if runinfo.Owner == runinfo.Sender {
+		return true, nil
+	}
+
+	// If the user who has submitted the pr is a owner on the repo then allows
+	// the CI to be run.
+	isUserMemberRepo, err := cs.GithubClient.CheckSenderOrgMembership(runinfo)
+	if err != nil {
+		return false, err
+	}
+
+	if isUserMemberRepo {
+		return true, nil
+	}
+
+	// TODO: there is going to be more stuff in there
+	return false, err
+}
diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go
index f8972cef..d90c0931 100644
--- a/pkg/pipelineascode/pipelineascode.go
+++ b/pkg/pipelineascode/pipelineascode.go
@@ -63,6 +63,20 @@ func Run(cs *cli.Clients, k8int cli.KubeInteractionIntf, runinfo *webvcs.RunInfo
 		return err
 	}
 	runinfo.CheckRunID = checkRun.ID
+
+	allowed, err := aclCheck(cs, runinfo)
+	if err != nil {
+		return err
+	}
+
+	if !allowed {
+		_, _ = cs.GithubClient.CreateStatus(runinfo, "completed", "skipped",
+			fmt.Sprintf("User %s is not allowed to run CI on this repo.", runinfo.Sender),
+			"https://tenor.com/search/police-cat-gifs")
+		cs.Log.Infof("User %s is not allowed to run CI on this repo", runinfo.Sender)
+		return nil
+	}
+
 	maintektonyaml, _ := cs.GithubClient.GetFileFromDefaultBranch(filepath.Join(tektonDir, tektonConfigurationFile), runinfo)
 	if maintektonyaml != "" {
 		maintekton, err = processTektonYaml(cs, runinfo, maintektonyaml)
diff --git a/pkg/webvcs/github.go b/pkg/webvcs/github.go
index a07e1bff..a8f60ccc 100644
--- a/pkg/webvcs/github.go
+++ b/pkg/webvcs/github.go
@@ -27,6 +27,7 @@ type RunInfo struct {
 	SHA           string
 	URL           string
 	Branch        string
+	Sender        string
 	CheckRunID    *int64
 	WebConsoleURL string
 }
@@ -34,7 +35,7 @@ type RunInfo struct {
 func (r RunInfo) Check() error {
 	if r.SHA != "" && r.Branch != "" &&
 		r.Repository != "" && r.DefaultBranch != "" &&
-		r.Owner != "" && r.URL != "" {
+		r.Owner != "" && r.URL != "" && r.Sender != "" {
 		return nil
 	}
 	return fmt.Errorf("missing values in runInfo")
@@ -75,28 +76,32 @@ func payloadFix(payload string) string {
 }
 
 func (v GithubVCS) ParsePayload(log *zap.SugaredLogger, payload string) (*RunInfo, error) {
+	runinfo := &RunInfo{}
 	payload = payloadFix(payload)
 	prMap := &github.PullRequestEvent{}
 	err := json.Unmarshal([]byte(payload), prMap)
 	if err != nil {
-		return &RunInfo{}, err
+		return runinfo, err
 	}
 
 	checkrunEvent := new(github.CheckRunEvent)
+
 	err = json.Unmarshal([]byte(payload), &checkrunEvent)
 	if err != nil {
-		return &RunInfo{}, err
+		return runinfo, err
 	}
 
 	// If the user  has requested a recheck then fetch the pr link and use that.
 	if checkrunEvent.GetAction() == "rerequested" {
-		owner := checkrunEvent.GetRepo().Owner.GetLogin()
-		repo := checkrunEvent.GetRepo().GetName()
+		runinfo.Owner = checkrunEvent.GetRepo().Owner.GetLogin()
+		runinfo.Sender = checkrunEvent.GetSender().GetLogin()
+		runinfo.Repository = checkrunEvent.GetRepo().GetName()
 		prNumber := checkrunEvent.GetCheckRun().GetCheckSuite().PullRequests[0].GetNumber()
-		log.Info("Recheck of PR %s/%s#%s has been requested", owner, repo, prNumber)
+
+		log.Info("Recheck of PR %s/%s#%s has been requested", runinfo.Owner, runinfo.Repository, prNumber)
 		// There should be only one pull_request, I am not quite sure how a
 		// checksuite with multiple PR can be done 🤔
-		pr, _, err := v.Client.PullRequests.Get(context.Background(), owner, repo, prNumber)
+		pr, _, err := v.Client.PullRequests.Get(context.Background(), runinfo.Owner, runinfo.Repository, prNumber)
 		if err != nil {
 			return &RunInfo{}, err
 		}
@@ -107,15 +112,42 @@ func (v GithubVCS) ParsePayload(log *zap.SugaredLogger, payload string) (*RunInf
 		return &RunInfo{}, errors.New("cannot parse payload as PR")
 	}
 
-	return &RunInfo{
-		Owner:         prMap.GetRepo().Owner.GetLogin(),
-		Repository:    prMap.GetRepo().GetName(),
-		URL:           prMap.GetRepo().GetHTMLURL(),
-		DefaultBranch: prMap.GetRepo().GetDefaultBranch(),
-		// TODO: this is going to be different on merge
-		SHA:    prMap.PullRequest.Head.GetSHA(),
-		Branch: prMap.PullRequest.Base.GetRef(),
-	}, nil
+	// This should already been set from the rerequest event
+	if runinfo.Sender == "" {
+		runinfo.Sender = prMap.GetPullRequest().GetUser().GetLogin()
+	}
+	if runinfo.Repository == "" {
+		runinfo.Repository = prMap.GetRepo().GetName()
+	}
+	if runinfo.Owner == "" {
+		runinfo.Owner = prMap.GetRepo().Owner.GetLogin()
+	}
+
+	runinfo.DefaultBranch = prMap.GetRepo().GetDefaultBranch()
+	runinfo.URL = prMap.GetRepo().GetHTMLURL()
+	runinfo.SHA = prMap.PullRequest.Head.GetSHA()
+	runinfo.Branch = prMap.PullRequest.Base.GetRef()
+
+	return runinfo, nil
+}
+
+// CheckSenderOrgMembership Get sender user's organization. We can
+// only get the one that the user sets as public 🤷
+func (v GithubVCS) CheckSenderOrgMembership(runinfo *RunInfo) (bool, error) {
+	users, _, err := v.Client.Organizations.ListMembers(v.Context, runinfo.Owner,
+		&github.ListMembersOptions{
+			PublicOnly: true, // We can't list private member in a org
+		})
+	if err != nil {
+		return false, err
+	}
+	for _, v := range users {
+		if v.GetLogin() == runinfo.Sender {
+			return true, nil
+		}
+	}
+
+	return false, nil
 }
 
 func (v GithubVCS) GetTektonDir(path string, runinfo *RunInfo) ([]*github.RepositoryContent, error) {


Test patch:
diff --git a/pkg/cmd/pipelineascode/pipelineascode_test.go b/pkg/cmd/pipelineascode/pipelineascode_test.go
index b1dccff4..1199d780 100644
--- a/pkg/cmd/pipelineascode/pipelineascode_test.go
+++ b/pkg/cmd/pipelineascode/pipelineascode_test.go
@@ -30,6 +30,7 @@ func TestGetInfo(t *testing.T) {
 		SHA:           "d0d0",
 		URL:           "http://chmouel.com",
 		Branch:        "goodRuninfoBranch",
+		Sender:        "ElSender",
 	}
 
 	b, err := ioutil.ReadFile("testdata/pull_request.json")
diff --git a/pkg/pipelineascode/acl_test.go b/pkg/pipelineascode/acl_test.go
new file mode 100644
index 00000000..024ffdfa
--- /dev/null
+++ b/pkg/pipelineascode/acl_test.go
@@ -0,0 +1,97 @@
+package pipelineascode
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"testing"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/cli"
+	testhelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/webvcs"
+)
+
+func TestAclCheck(t *testing.T) {
+	fakeclient, mux, _, teardown := testhelper.SetupGH()
+	defer teardown()
+
+	orgallowed := "allowed"
+	orgdenied := "denied"
+	errit := "err"
+
+	mux.HandleFunc("/orgs/"+orgallowed+"/public_members", func(rw http.ResponseWriter, r *http.Request) {
+		fmt.Fprintf(rw, `[{"login": "login_%s"}]`, orgallowed)
+	})
+
+	mux.HandleFunc("/orgs/"+orgdenied+"/public_members", func(rw http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(rw, `[]`)
+	})
+	mux.HandleFunc("/orgs/"+errit+"/public_members", func(rw http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(rw, `x1x`)
+	})
+
+	gvcs := webvcs.GithubVCS{
+		Client:  fakeclient,
+		Context: context.Background(),
+	}
+
+	tests := []struct {
+		name    string
+		runinfo *webvcs.RunInfo
+		allowed bool
+		wantErr bool
+	}{
+		{
+			name: "sender allowed in org",
+			runinfo: &webvcs.RunInfo{
+				Owner:  orgallowed,
+				Sender: "login_allowed",
+			},
+			allowed: true,
+			wantErr: false,
+		},
+		{
+			name: "owner is sender is allowed",
+			runinfo: &webvcs.RunInfo{
+				Owner:  orgallowed,
+				Sender: "allowed",
+			},
+			allowed: true,
+			wantErr: false,
+		},
+		{
+			name: "sender not allowed in org",
+			runinfo: &webvcs.RunInfo{
+				Owner:  orgdenied,
+				Sender: "notallowed",
+			},
+			allowed: false,
+			wantErr: false,
+		},
+		{
+			name: "err it",
+			runinfo: &webvcs.RunInfo{
+				Owner:  errit,
+				Sender: "error",
+			},
+			allowed: false,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cs := cli.Clients{
+				GithubClient: gvcs,
+			}
+
+			got, err := aclCheck(&cs, tt.runinfo)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("aclCheck() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.allowed {
+				t.Errorf("aclCheck() = %v, want %v", got, tt.allowed)
+			}
+		})
+	}
+}
diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go
index ab6c1682..8720d572 100644
--- a/pkg/pipelineascode/pipelineascode_test.go
+++ b/pkg/pipelineascode/pipelineascode_test.go
@@ -168,11 +168,12 @@ func TestRunDeniedFromForcedNamespace(t *testing.T) {
 	installedNamespace := "nomespace"
 	runinfo := &webvcs.RunInfo{
 		SHA:           "principale",
-		Owner:         "chmouel",
+		Owner:         "organizatione",
 		Repository:    "repo",
 		URL:           "https://service/documentation",
 		Branch:        "press",
 		DefaultBranch: defaultBranch,
+		Sender:        "carlos_valderama",
 	}
 	replyString(mux,
 		fmt.Sprintf("/repos/%s/%s/check-runs", runinfo.Owner, runinfo.Repository),
@@ -193,6 +194,10 @@ func TestRunDeniedFromForcedNamespace(t *testing.T) {
 			fmt.Fprintf(w, `{"content": "%s"}`, forcedNamespaceContent)
 		})
 
+	mux.HandleFunc("/orgs/"+runinfo.Owner+"/public_members", func(rw http.ResponseWriter, r *http.Request) {
+		fmt.Fprintf(rw, `[{"login": "%s"}]`, runinfo.Sender)
+	})
+
 	gcvs := webvcs.GithubVCS{
 		Client:  fakeclient,
 		Context: ctx,
@@ -222,10 +227,11 @@ func TestRun(t *testing.T) {
 	defer teardown()
 	runinfo := &webvcs.RunInfo{
 		SHA:        "principale",
-		Owner:      "gaston",
+		Owner:      "organizationes",
 		Repository: "lagaffe",
 		URL:        "https://service/documentation",
 		Branch:     "press",
+		Sender:     "fantasio",
 	}
 
 	replyString(mux,
@@ -257,6 +263,10 @@ func TestRun(t *testing.T) {
 		fmt.Sprintf("/repos/%s/%s/contents/internal/task", runinfo.Owner, runinfo.Repository),
 		`{"sha": "internaltasksha"}`)
 
+	mux.HandleFunc("/orgs/"+runinfo.Owner+"/public_members", func(rw http.ResponseWriter, r *http.Request) {
+		fmt.Fprintf(rw, `[{"login": "%s"}]`, runinfo.Sender)
+	})
+
 	// Internal task referenced in tekton.yaml
 	taskB, err := ioutil.ReadFile("testdata/task.yaml")
 	assert.NilError(t, err)
diff --git a/pkg/webvcs/github_test.go b/pkg/webvcs/github_test.go
index 29a56666..02b76a57 100644
--- a/pkg/webvcs/github_test.go
+++ b/pkg/webvcs/github_test.go
@@ -36,12 +36,15 @@ func TestPayLoadFix(t *testing.T) {
 }
 
 func TestParsePayloadRerequest(t *testing.T) {
+	repoSender := "jean-pierre"
 	repoOwner := "openshift"
 	repoName := "pipelines"
 	prNumber := "123"
 	checkrunEvent := fmt.Sprintf(`{"action": "rerequested", 
+	"sender": {"login": "%s"},
 	"check_run": {"check_suite": {"pull_requests": [{"number": %s}]}}, 
-	"repository": {"name": "%s", "owner": {"login": "%s"}}}`, prNumber, repoName, repoOwner)
+	"repository": {"name": "%s", "owner": {"login": "%s"}}}`,
+		repoSender, prNumber, repoName, repoOwner)
 	fakeclient, mux, _, teardown := testhelper.SetupGH()
 	defer teardown()
 	mux.HandleFunc("/repos/"+repoOwner+"/"+repoName+"/pulls/"+prNumber, func(rw http.ResponseWriter, r *http.Request) {
@@ -54,9 +57,10 @@ func TestParsePayloadRerequest(t *testing.T) {
 	logger, observer := getLogger()
 	runinfo, err := gvcs.ParsePayload(logger, checkrunEvent)
 	assert.NilError(t, err)
+
 	assert.Equal(t, repoOwner, runinfo.Owner)
 	assert.Equal(t, repoName, runinfo.Repository)
-	assert.Equal(t, repoOwner, runinfo.Owner)
+	assert.Equal(t, repoSender, runinfo.Sender)
 	assert.Assert(t, strings.Contains(observer.TakeAll()[0].Message, "Recheck of PR"))
 }
 
@@ -373,6 +377,7 @@ func TestRunInfoCheck(t *testing.T) {
 		SHA           string
 		URL           string
 		Branch        string
+		Sender        string
 		CheckRunID    *int64
 	}
 	tests := []struct {
@@ -394,6 +399,7 @@ func TestRunInfoCheck(t *testing.T) {
 				SHA:           "1d1",
 				URL:           "https://anywhere",
 				Branch:        "main",
+				Sender:        "beeboo",
 			},
 			wantErr: false,
 		},
@@ -408,6 +414,7 @@ func TestRunInfoCheck(t *testing.T) {
 				URL:           tt.fields.URL,
 				Branch:        tt.fields.Branch,
 				CheckRunID:    tt.fields.CheckRunID,
+				Sender:        tt.fields.Sender,
 			}
 			if err := r.Check(); (err != nil) != tt.wantErr {
 				t.Errorf("RunInfo.Check() error = %v, wantErr %v", err, tt.wantErr)