Add owner flag validation during provider enrollment (#4107)

Fix #2723

config/server-config.yaml.example

@@ -110,6 +110,7 @@ authz:
 #    namespace: stacklok
 #    name: healthcheck
 
+# Set key_dir path to /app/.ssh for docker compose and .ssh for running minder outside of docker compose
 crypto:
     keystore:
         type: local

internal/controlplane/handlers_oauth.go

@@ -291,6 +291,14 @@ func (s *Server) processOAuthCallback(ctx context.Context, w http.ResponseWriter
 		return fmt.Errorf("error encoding token: %w", err)
 	}
 
+	// Verify if the token user is a member of the ownerFilter (Organization)
+	if stateData.OwnerFilter.Valid {
+		err := s.validateOwnerFilter(ctx, token, stateData.OwnerFilter.String)
+		if err != nil {
+			return err
+		}
+	}
+
 	var errConfig providers.ErrProviderInvalidConfig
 
 	p, err := s.sessionService.CreateProviderFromSessionState(ctx, db.ProviderClass(provider), &encryptedToken, state)
@@ -324,6 +332,20 @@ func (s *Server) processOAuthCallback(ctx context.Context, w http.ResponseWriter
 	return nil
 }
 
+func (s *Server) validateOwnerFilter(ctx context.Context, token *oauth2.Token, ownerFilter string) error {
+	member, err := s.ghProviders.ValidateOrgMembershipForToken(ctx, token, ownerFilter)
+	if err != nil {
+		return fmt.Errorf("error checking organization membership: %w", err)
+	}
+
+	if !member {
+		return newHttpError(http.StatusForbidden, "Invalid OwnerFilter Provided").SetContents(
+			"You do not have access to the organization %s, specified in the owner filter."+
+				" Please ensure that you are a member of the GitHub organization and try again.", ownerFilter)
+	}
+	return nil
+}
+
 func (s *Server) processAppCallback(ctx context.Context, w http.ResponseWriter, r *http.Request,
 	pathParams map[string]string) error {
 	state := r.URL.Query().Get("state")

internal/controlplane/handlers_oauth_test.go

@@ -462,6 +462,18 @@ func TestProviderCallback(t *testing.T) {
 			Return([]db.Provider{}, nil)
 	}
 
+	withValidOrgMemberships := func(service *mockprovsvc.MockGitHubProviderService) {
+		service.EXPECT().ValidateOrgMembershipForToken(gomock.Any(), gomock.Any(), gomock.Any()).Return(true, nil)
+	}
+
+	withInvalidOrgMemberships := func(service *mockprovsvc.MockGitHubProviderService) {
+		service.EXPECT().ValidateOrgMembershipForToken(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
+	}
+
+	orgMembershipWithFailure := func(service *mockprovsvc.MockGitHubProviderService) {
+		service.EXPECT().ValidateOrgMembershipForToken(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, errors.New("failed to validate org membership"))
+	}
+
 	testCases := []struct {
 		name                       string
 		redirectUrl                string
@@ -471,14 +483,16 @@ func TestProviderCallback(t *testing.T) {
 		projectIDBySessionNumCalls int
 		err                        string
 		config                     []byte
+		buildStubs                 func(service *mockprovsvc.MockGitHubProviderService)
 	}{{
 		name:                       "Success",
 		redirectUrl:                "http://localhost:8080",
 		projectIDBySessionNumCalls: 2,
 		storeMockSetup: func(store *mockdb.MockStore) {
 			withProviderSearch(store)
 		},
-		code: 307,
+		code:       307,
+		buildStubs: withValidOrgMemberships,
 	}, {
 		name:                       "Success with remote user",
 		redirectUrl:                "http://localhost:8080",
@@ -487,7 +501,8 @@ func TestProviderCallback(t *testing.T) {
 		storeMockSetup: func(store *mockdb.MockStore) {
 			withProviderSearch(store)
 		},
-		code: 307,
+		code:       307,
+		buildStubs: withValidOrgMemberships,
 	}, {
 		name:                       "Wrong remote userid",
 		remoteUser:                 sql.NullString{Valid: true, String: "1234"},
@@ -497,6 +512,9 @@ func TestProviderCallback(t *testing.T) {
 		},
 		code: 403,
 		err:  "The provided login token was associated with a different user.\n",
+		buildStubs: func(_ *mockprovsvc.MockGitHubProviderService) {
+			// this codepath fails before the ghProviderService is called
+		},
 	}, {
 		name:        "No existing provider",
 		redirectUrl: "http://localhost:8080",
@@ -508,6 +526,7 @@ func TestProviderCallback(t *testing.T) {
 		storeMockSetup: func(store *mockdb.MockStore) {
 			withProviderCreate(store)
 		},
+		buildStubs: withValidOrgMemberships,
 	}, {
 		name:                       "Config does not validate",
 		redirectUrl:                "http://localhost:8080",
@@ -516,8 +535,28 @@ func TestProviderCallback(t *testing.T) {
 		storeMockSetup: func(store *mockdb.MockStore) {
 			withProviderNotFound(store)
 		},
-		config: []byte(`{`),
-		code:   http.StatusBadRequest,
+		config:     []byte(`{`),
+		code:       http.StatusBadRequest,
+		buildStubs: withValidOrgMemberships,
+	}, {
+		name:                       "Invalid ownerfilter returns useful error",
+		redirectUrl:                "http://localhost:8080",
+		projectIDBySessionNumCalls: 1,
+		storeMockSetup: func(_ *mockdb.MockStore) {
+			// this codepath fails before the store is called
+		},
+		code:       403,
+		err:        "You do not have access to the organization TestOwner, specified in the owner filter. Please ensure that you are a member of the GitHub organization and try again.\n",
+		buildStubs: withInvalidOrgMemberships,
+	}, {
+		name:                       "Other failures during ownerfilter returns generic error",
+		redirectUrl:                "http://localhost:8080",
+		projectIDBySessionNumCalls: 1,
+		storeMockSetup: func(_ *mockdb.MockStore) {
+			// this codepath fails before the store is called
+		},
+		code:       500,
+		buildStubs: orgMembershipWithFailure,
 	}}
 
 	for _, tt := range testCases {
@@ -572,7 +611,10 @@ func TestProviderCallback(t *testing.T) {
 			}
 			tc.storeMockSetup(store)
 
+			mockGhProviderService := mockprovsvc.NewMockGitHubProviderService(ctrl)
+			tc.buildStubs(mockGhProviderService)
 			s, _ := newDefaultServer(t, store, clientFactory)
+			s.ghProviders = mockGhProviderService
 			s.cfg.Provider = serverconfig.ProviderConfig{
 				GitHub: &serverconfig.GitHubConfig{
 					OAuthClientConfig: serverconfig.OAuthClientConfig{
@@ -658,6 +700,7 @@ func TestProviderCallback(t *testing.T) {
 					},
 					RemoteUser:     tc.remoteUser,
 					ProviderConfig: tc.config,
+					OwnerFilter:    sql.NullString{String: "TestOwner", Valid: true},
 				}, nil).Times(tc.projectIDBySessionNumCalls)
 
 			if tc.code < http.StatusBadRequest {

internal/providers/github/common.go

@@ -94,6 +94,7 @@ type ClientService interface {
 	GetUserIdFromToken(ctx context.Context, token *oauth2.Token) (*int64, error)
 	ListUserInstallations(ctx context.Context, token *oauth2.Token) ([]*github.Installation, error)
 	DeleteInstallation(ctx context.Context, id int64, jwt string) (*github.Response, error)
+	GetOrgMembership(ctx context.Context, token *oauth2.Token, org string) (*github.Membership, *github.Response, error)
 }
 
 var _ ClientService = (*ClientServiceImplementation)(nil)
@@ -139,6 +140,14 @@ func (ClientServiceImplementation) DeleteInstallation(ctx context.Context, id in
 	return ghClient.Apps.DeleteInstallation(ctx, id)
 }
 
+// GetOrgMembership is a wrapper for the GitHub API to get users' organization membership
+func (ClientServiceImplementation) GetOrgMembership(
+	ctx context.Context, token *oauth2.Token, org string,
+) (*github.Membership, *github.Response, error) {
+	ghClient := github.NewClient(nil).WithAuthToken(token.AccessToken)
+	return ghClient.Organizations.GetOrgMembership(ctx, "", org)
+}
+
 // Delegate is the interface that contains operations that differ between different GitHub actors (user vs app)
 type Delegate interface {
 	GetCredential() provifv1.GitHubCredential

internal/providers/github/service/service.go

@@ -63,6 +63,8 @@ type GitHubProviderService interface {
 	// DeleteInstallation deletes the installation from GitHub, if the provider has an associated installation
 	DeleteInstallation(ctx context.Context, providerID uuid.UUID) error
 	VerifyProviderTokenIdentity(ctx context.Context, remoteUser string, accessToken string) error
+	// ValidateOrgMembershipForToken checks if the token user is a member of the organization
+	ValidateOrgMembershipForToken(ctx context.Context, token *oauth2.Token, org string) (bool, error)
 }
 
 // TypeGitHubOrganization is the type returned from the GitHub API when the owner is an organization
@@ -427,3 +429,19 @@ func (p *ghProviderService) getInstallationOwner(ctx context.Context, installati
 	}
 	return installation.GetAccount(), nil
 }
+
+// Check if the user is an actve member of the Github organization
+func (p *ghProviderService) ValidateOrgMembershipForToken(ctx context.Context, token *oauth2.Token, org string) (bool, error) {
+	membership, ghResponse, err := p.ghClientService.GetOrgMembership(ctx, token, org)
+	if err != nil {
+		if ghResponse.StatusCode == http.StatusNotFound {
+			return false, nil
+		}
+		return false, fmt.Errorf("error getting user membership: %w", err)
+	}
+	if membership.GetState() == "active" {
+		return true, nil
+	}
+	// If the user is not active member of the organization
+	return false, nil
+}

internal/providers/github/service/service_test.go

@@ -25,6 +25,7 @@ import (
 	"encoding/json"
 	"encoding/pem"
 	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -546,3 +547,59 @@ func TestProviderService_DeleteInstallation(t *testing.T) {
 	)
 	require.NoError(t, err)
 }
+
+func TestProviderService_ValidateOrgMembershipForToken(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	pvtKeyFile := testCreatePrivateKeyFile(t)
+	defer os.Remove(pvtKeyFile.Name())
+	cfg := &server.ProviderConfig{
+		GitHubApp: &server.GitHubAppConfig{
+			PrivateKey: pvtKeyFile.Name(),
+		},
+	}
+
+	provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, nil, nil)
+
+	// Active member of the GitHub organization
+	memberResp := github.Membership{State: github.String("active")}
+	mocks.svcMock.EXPECT().GetOrgMembership(gomock.Any(), gomock.Any(), gomock.Any()).Return(&memberResp, &github.Response{}, nil)
+	member, err := provSvc.ValidateOrgMembershipForToken(context.Background(), &oauth2.Token{}, "test-org")
+	require.NoError(t, err)
+	require.True(t, member)
+
+	// Non Active member of the GitHub organization
+	memberResp = github.Membership{State: github.String("inactive")}
+	mocks.svcMock.EXPECT().GetOrgMembership(gomock.Any(), gomock.Any(), gomock.Any()).Return(&memberResp, &github.Response{}, nil)
+	member, err = provSvc.ValidateOrgMembershipForToken(context.Background(), &oauth2.Token{}, "test-org")
+	require.NoError(t, err)
+	require.False(t, member)
+
+	// Member not found in the GitHub organization
+	httpResponse := &http.Response{
+		StatusCode: http.StatusNotFound,
+	}
+	ghResponse := &github.Response{
+		Response: httpResponse,
+	}
+	mocks.svcMock.EXPECT().GetOrgMembership(gomock.Any(), gomock.Any(), gomock.Any()).Return(&memberResp, ghResponse, fmt.Errorf("test error"))
+	member, err = provSvc.ValidateOrgMembershipForToken(context.Background(), &oauth2.Token{}, "test-org")
+	require.NoError(t, err)
+	require.False(t, member)
+
+	// Error while fetching member from the GitHub organization
+	httpResponse = &http.Response{
+		StatusCode: http.StatusInternalServerError,
+	}
+	ghResponse = &github.Response{
+		Response: httpResponse,
+	}
+	mocks.svcMock.EXPECT().GetOrgMembership(gomock.Any(), gomock.Any(), gomock.Any()).Return(&memberResp, ghResponse, fmt.Errorf("test error"))
+	member, err = provSvc.ValidateOrgMembershipForToken(context.Background(), &oauth2.Token{}, "test-org")
+	require.Error(t, err)
+	require.False(t, member)
+
+}