From 549b2d8f1949b702bf998272867cdae91c2ce59e Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 31 Oct 2024 13:12:48 +0100 Subject: [PATCH] Ensure token is valid before proceeding (#1803) Make sure the token is valid before proceeding with the rest of the code. Signed-off-by: Chmouel Boudjnah --- pkg/provider/bitbucketserver/acl_test.go | 3 +- .../bitbucketserver/bitbucketserver.go | 18 +++-- .../bitbucketserver/bitbucketserver_test.go | 65 +++++++++++++++---- pkg/provider/bitbucketserver/test/test.go | 4 +- 4 files changed, 71 insertions(+), 19 deletions(-) diff --git a/pkg/provider/bitbucketserver/acl_test.go b/pkg/provider/bitbucketserver/acl_test.go index 2fc10474e..b27e8aef0 100644 --- a/pkg/provider/bitbucketserver/acl_test.go +++ b/pkg/provider/bitbucketserver/acl_test.go @@ -203,7 +203,7 @@ func TestIsAllowed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) - bbclient, mux, tearDown := bbv1test.SetupBBServerClient(ctx) + bbclient, mux, tearDown, tURL := bbv1test.SetupBBServerClient(ctx) defer tearDown() bbv1test.MuxProjectMemberShip(t, mux, tt.event, tt.fields.projectMembers) bbv1test.MuxRepoMemberShip(t, mux, tt.event, tt.fields.repoMembers) @@ -211,6 +211,7 @@ func TestIsAllowed(t *testing.T) { bbv1test.MuxFiles(t, mux, tt.event, tt.fields.defaultBranchLatestCommit, "", tt.fields.filescontents) v := &Provider{ + baseURL: tURL, Client: bbclient, defaultBranchLatestCommit: tt.fields.defaultBranchLatestCommit, pullRequestNumber: tt.fields.pullRequestNumber, diff --git a/pkg/provider/bitbucketserver/bitbucketserver.go b/pkg/provider/bitbucketserver/bitbucketserver.go index b1bcbd7d5..8efee911b 100644 --- a/pkg/provider/bitbucketserver/bitbucketserver.go +++ b/pkg/provider/bitbucketserver/bitbucketserver.go @@ -3,6 +3,7 @@ package bitbucketserver import ( "context" "fmt" + "net/http" "path/filepath" "strings" @@ -215,13 +216,13 @@ func (v *Provider) GetFileInsideRepo(_ context.Context, event *info.Event, path, func (v *Provider) SetClient(ctx context.Context, run *params.Run, event *info.Event, _ *v1alpha1.Repository, _ *events.EventEmitter) error { if event.Provider.User == "" { - return fmt.Errorf("no provider.user has been set in the repo crd") + return fmt.Errorf("no spec.git_provider.user has been set in the repo crd") } if event.Provider.Token == "" { - return fmt.Errorf("no provider.secret has been set in the repo crd") + return fmt.Errorf("no spec.git_provider.secret has been set in the repo crd") } if event.Provider.URL == "" { - return fmt.Errorf("no provider.url has been set in the repo crd") + return fmt.Errorf("no spec.git_provider.url has been set in the repo crd") } // make sure we have /rest at the end of the url @@ -237,8 +238,17 @@ func (v *Provider) SetClient(ctx context.Context, run *params.Run, event *info.E ctx = context.WithValue(ctx, bbv1.ContextBasicAuth, basicAuth) cfg := bbv1.NewConfiguration(event.Provider.URL) - v.Client = bbv1.NewAPIClient(ctx, cfg) + if v.Client == nil { + v.Client = bbv1.NewAPIClient(ctx, cfg) + } v.run = run + resp, err := v.Client.DefaultApi.GetUser(event.Provider.User) + if resp.Response != nil && resp.StatusCode == http.StatusUnauthorized { + return fmt.Errorf("cannot get user %s with token: %w", event.Provider.User, err) + } + if err != nil { + return fmt.Errorf("cannot get user %s: %w", event.Provider.User, err) + } return nil } diff --git a/pkg/provider/bitbucketserver/bitbucketserver_test.go b/pkg/provider/bitbucketserver/bitbucketserver_test.go index 1aba49d79..2df468747 100644 --- a/pkg/provider/bitbucketserver/bitbucketserver_test.go +++ b/pkg/provider/bitbucketserver/bitbucketserver_test.go @@ -55,9 +55,9 @@ func TestGetTektonDir(t *testing.T) { observer, _ := zapobserver.New(zap.InfoLevel) logger := zap.New(observer).Sugar() ctx, _ := rtesting.SetupFakeContext(t) - client, mux, tearDown := bbtest.SetupBBServerClient(ctx) + client, mux, tearDown, tURL := bbtest.SetupBBServerClient(ctx) defer tearDown() - v := &Provider{Logger: logger, Client: client, projectKey: tt.event.Organization} + v := &Provider{Logger: logger, baseURL: tURL, Client: client, projectKey: tt.event.Organization} bbtest.MuxDirContent(t, mux, tt.event, tt.testDirPath, tt.path) content, err := v.GetTektonDir(ctx, tt.event, tt.path, "") if tt.wantErr { @@ -168,7 +168,7 @@ func TestCreateStatus(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) - client, mux, tearDown := bbtest.SetupBBServerClient(ctx) + client, mux, tearDown, tURL := bbtest.SetupBBServerClient(ctx) defer tearDown() if tt.nilClient { client = nil @@ -177,6 +177,7 @@ func TestCreateStatus(t *testing.T) { event.EventType = "pull_request" event.Provider.Token = "token" v := &Provider{ + baseURL: tURL, Client: client, pullRequestNumber: pullRequestNumber, projectKey: event.Organization, @@ -229,9 +230,9 @@ func TestGetFileInsideRepo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) - client, mux, tearDown := bbtest.SetupBBServerClient(ctx) + client, mux, tearDown, tURL := bbtest.SetupBBServerClient(ctx) defer tearDown() - v := &Provider{Client: client, defaultBranchLatestCommit: "1234", projectKey: tt.event.Organization} + v := &Provider{Client: client, baseURL: tURL, defaultBranchLatestCommit: "1234", projectKey: tt.event.Organization} bbtest.MuxFiles(t, mux, tt.event, tt.targetbranch, filepath.Dir(tt.path), tt.filescontents) fc, err := v.GetFileInsideRepo(ctx, tt.event, tt.path, tt.targetbranch) assert.NilError(t, err) @@ -246,11 +247,12 @@ func TestSetClient(t *testing.T) { apiURL string opts *info.Event wantErrSubstr string + muxUser func(w http.ResponseWriter, r *http.Request) }{ { name: "bad/no username", opts: info.NewEvent(), - wantErrSubstr: "no provider.user", + wantErrSubstr: "no spec.git_provider.user", }, { name: "bad/no secret", @@ -259,7 +261,7 @@ func TestSetClient(t *testing.T) { User: "foo", }, }, - wantErrSubstr: "no provider.secret", + wantErrSubstr: "no spec.git_provider.secret", }, { name: "bad/no url", @@ -269,7 +271,39 @@ func TestSetClient(t *testing.T) { Token: "bar", }, }, - wantErrSubstr: "no provider.url", + wantErrSubstr: "no spec.git_provider.url", + }, + { + name: "bad/invalid user", + opts: &info.Event{ + Provider: &info.Provider{ + User: "foo", + Token: "bar", + URL: "https://foo.bar", + }, + }, + muxUser: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"errors": [{"message": "Unauthorized"}]}`)) + }, + apiURL: "https://foo.bar/rest", + wantErrSubstr: "cannot get user foo with token", + }, + { + name: "bad/unknown error", + opts: &info.Event{ + Provider: &info.Provider{ + User: "foo", + Token: "bar", + URL: "https://foo.bar", + }, + }, + muxUser: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"errors": [{"message": "Internal Server Error"}]}`)) + }, + apiURL: "https://foo.bar/rest", + wantErrSubstr: "cannot get user foo: Status: 500", }, { name: "good/url append /rest", @@ -280,13 +314,21 @@ func TestSetClient(t *testing.T) { URL: "https://foo.bar", }, }, + muxUser: func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, `{"name": "foo"}`) + }, apiURL: "https://foo.bar/rest", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) - v := &Provider{} + bbclient, mux, tearDown, tURL := bbtest.SetupBBServerClient(ctx) + defer tearDown() + if tt.muxUser != nil { + mux.HandleFunc("/users/foo", tt.muxUser) + } + v := &Provider{Client: bbclient, baseURL: tURL} err := v.SetClient(ctx, nil, tt.opts, nil, nil) if tt.wantErrSubstr != "" { assert.ErrorContains(t, err, tt.wantErrSubstr) @@ -299,7 +341,6 @@ func TestSetClient(t *testing.T) { } func TestGetCommitInfo(t *testing.T) { - defaultBaseURL := "https://base" tests := []struct { name string event *info.Event @@ -325,11 +366,11 @@ func TestGetCommitInfo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) - bbclient, mux, tearDown := bbtest.SetupBBServerClient(ctx) + bbclient, mux, tearDown, tURL := bbtest.SetupBBServerClient(ctx) bbtest.MuxCommitInfo(t, mux, tt.event, tt.commit) bbtest.MuxDefaultBranch(t, mux, tt.event, tt.defaultBranch, tt.latestCommit) defer tearDown() - v := &Provider{Client: bbclient, baseURL: defaultBaseURL, projectKey: tt.event.Organization} + v := &Provider{Client: bbclient, baseURL: tURL, projectKey: tt.event.Organization} err := v.GetCommitInfo(ctx, tt.event) assert.NilError(t, err) assert.Equal(t, tt.defaultBranch, tt.event.DefaultBranch) diff --git a/pkg/provider/bitbucketserver/test/test.go b/pkg/provider/bitbucketserver/test/test.go index 1486e4ec6..d3d395c9f 100644 --- a/pkg/provider/bitbucketserver/test/test.go +++ b/pkg/provider/bitbucketserver/test/test.go @@ -25,7 +25,7 @@ var ( buildAPIURL = "/build-status/1.0" ) -func SetupBBServerClient(ctx context.Context) (*bbv1.APIClient, *http.ServeMux, func()) { +func SetupBBServerClient(ctx context.Context) (*bbv1.APIClient, *http.ServeMux, func(), string) { mux := http.NewServeMux() apiHandler := http.NewServeMux() apiHandler.Handle(defaultAPIURL+"/", http.StripPrefix(defaultAPIURL, mux)) @@ -49,7 +49,7 @@ func SetupBBServerClient(ctx context.Context) (*bbv1.APIClient, *http.ServeMux, cfg := bbv1.NewConfiguration(server.URL) cfg.HTTPClient = server.Client() client := bbv1.NewAPIClient(ctx, cfg) - return client, mux, tearDown + return client, mux, tearDown, server.URL } func MuxCreateComment(t *testing.T, mux *http.ServeMux, event *info.Event, expectedCommentSubstr string, prID int) {