Skip to content

Commit

Permalink
Do not construct provider when validating user ID
Browse files Browse the repository at this point in the history
Relates to #2845

When creating a provider, the code previously inserted the provider into
the database and then instantiated it in order to validate that the user
ID is correct. Alter the code so that it just instantiates the client
and some supporting structures to validate the user ID.

This client deliberately does not use the client cache in order to
simplify the code. Since this is the first usage of the client for that
provider, we will never find a suitable client in the cache anyway.
  • Loading branch information
dmjb committed May 1, 2024
1 parent b9daa61 commit 64aa299
Show file tree
Hide file tree
Showing 23 changed files with 464 additions and 236 deletions.
2 changes: 1 addition & 1 deletion cmd/cli/app/quickstart/quickstart.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import (
minderprov "github.com/stacklok/minder/cmd/cli/app/provider"
"github.com/stacklok/minder/cmd/cli/app/repo"
"github.com/stacklok/minder/internal/engine"
ghclient "github.com/stacklok/minder/internal/providers/github/oauth"
ghclient "github.com/stacklok/minder/internal/providers/github/clients"
"github.com/stacklok/minder/internal/util/cli"
minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
)
Expand Down
1 change: 0 additions & 1 deletion cmd/server/app/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ var serveCmd = &cobra.Command{
controlplane.WithProviderMetrics(providerMetrics),
controlplane.WithAuthzClient(authzc),
controlplane.WithIdentityClient(idClient),
controlplane.WithRestClientCache(restClientCache),
}

tsmdw := logger.NewTelemetryStoreWMMiddleware(l)
Expand Down
4 changes: 2 additions & 2 deletions internal/controlplane/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ import (

"google.golang.org/grpc/codes"

"github.com/stacklok/minder/internal/providers/github/oauth"
"github.com/stacklok/minder/internal/providers/github/clients"
"github.com/stacklok/minder/internal/util"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
)

const (
defaultProvider = oauth.Github
defaultProvider = clients.Github
githubURL = "https://github.com"
)

Expand Down
10 changes: 5 additions & 5 deletions internal/controlplane/handlers_githubwebhooks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (s *UnitTestSuite) TestHandleWebHookPing() {
defer ctrl.Finish()

mockStore := mockdb.NewMockStore(ctrl)
srv, evt := newDefaultServer(t, mockStore)
srv, evt := newDefaultServer(t, mockStore, nil)
srv.cfg.WebhookConfig.WebhookSecretFile = whSecretFile.Name()
defer evt.Close()

Expand Down Expand Up @@ -149,7 +149,7 @@ func (s *UnitTestSuite) TestHandleWebHookUnexistentRepository() {
defer ctrl.Finish()

mockStore := mockdb.NewMockStore(ctrl)
srv, evt := newDefaultServer(t, mockStore)
srv, evt := newDefaultServer(t, mockStore, nil)
defer evt.Close()

pq := testqueue.NewPassthroughQueue(t)
Expand Down Expand Up @@ -221,7 +221,7 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
defer os.Remove(prevCredsFile.Name())

mockStore := mockdb.NewMockStore(ctrl)
srv, evt := newDefaultServer(t, mockStore)
srv, evt := newDefaultServer(t, mockStore, nil)
srv.cfg.WebhookConfig.WebhookSecret = "not-our-secret"
srv.cfg.WebhookConfig.PreviousWebhookSecretFile = prevCredsFile.Name()
defer evt.Close()
Expand Down Expand Up @@ -329,7 +329,7 @@ func (s *UnitTestSuite) TestHandleWebHookUnexistentRepoPackage() {
defer ctrl.Finish()

mockStore := mockdb.NewMockStore(ctrl)
srv, evt := newDefaultServer(t, mockStore)
srv, evt := newDefaultServer(t, mockStore, nil)
defer evt.Close()

pq := testqueue.NewPassthroughQueue(t)
Expand Down Expand Up @@ -397,7 +397,7 @@ func (s *UnitTestSuite) TestNoopWebhookHandler() {
defer ctrl.Finish()

mockStore := mockdb.NewMockStore(ctrl)
srv, evt := newDefaultServer(t, mockStore)
srv, evt := newDefaultServer(t, mockStore, nil)
defer evt.Close()

go func() {
Expand Down
21 changes: 11 additions & 10 deletions internal/controlplane/handlers_oauth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
"github.com/stacklok/minder/internal/engine"
"github.com/stacklok/minder/internal/events"
"github.com/stacklok/minder/internal/providers"
mockclients "github.com/stacklok/minder/internal/providers/github/clients/mock"
mockgh "github.com/stacklok/minder/internal/providers/github/mock"
ghService "github.com/stacklok/minder/internal/providers/github/service"
mockprovsvc "github.com/stacklok/minder/internal/providers/github/service/mock"
Expand Down Expand Up @@ -405,19 +406,19 @@ func TestProviderCallback(t *testing.T) {
gh := mockgh.NewMockGitHub(ctrl)
gh.EXPECT().GetUserId(gomock.Any()).Return(int64(31337), nil).AnyTimes()

var opts []ServerOption
var clientFactory *mockclients.MockGitHubClientFactory
if tc.remoteUser.String != "" {
// TODO: verfifyProviderTokenIdentity
cancelable, cancel := context.WithCancel(context.Background())
defer cancel()
clientCache := ratecache.NewRestClientCache(cancelable)
clientCache.Set("", "anAccessToken", db.ProviderTypeGithub, gh)
opts = []ServerOption{
WithRestClientCache(clientCache),
}
delegate := mockgh.NewMockDelegate(ctrl)
delegate.EXPECT().
GetUserId(gomock.Any()).
Return(int64(31337), nil)
clientFactory = mockclients.NewMockGitHubClientFactory(ctrl)
clientFactory.EXPECT().
BuildOAuthClient(gomock.Any(), gomock.Any(), gomock.Any()).
Return(nil, delegate, nil)
}

s, _ := newDefaultServer(t, store, opts...)
s, _ := newDefaultServer(t, store, clientFactory)

var err error
encryptedUrlString, err := s.cryptoEngine.EncryptString(tc.redirectUrl)
Expand Down
2 changes: 1 addition & 1 deletion internal/controlplane/handlers_repositories_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ import (
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine"
"github.com/stacklok/minder/internal/providers"
ghprovider "github.com/stacklok/minder/internal/providers/github/clients"
mockgh "github.com/stacklok/minder/internal/providers/github/mock"
ghprovider "github.com/stacklok/minder/internal/providers/github/oauth"
"github.com/stacklok/minder/internal/providers/ratecache"
ghrepo "github.com/stacklok/minder/internal/repositories/github"
mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock"
Expand Down
1 change: 1 addition & 0 deletions internal/controlplane/handlers_user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,7 @@ func TestDeleteUser_gRPC(t *testing.T) {
GoChannel: serverconfig.GoChannelEventConfig{},
})
require.NoError(t, err, "failed to setup eventer")

server, err := NewServer(
mockStore,
evt,
Expand Down
72 changes: 45 additions & 27 deletions internal/controlplane/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,12 @@ func WithAuthzClient(c authz.Client) ServerOption {
}
}

// WithRestClientCache sets the rest client cache for the server
func WithRestClientCache(c ratecache.RestClientCache) ServerOption {
// WithGitHubProviderService sets the rest client cache for the server
// This is used for unit testing, and it may be possible to remove this in future
// with some refactoring.
func WithGitHubProviderService(g service.GitHubProviderService) ServerOption {
return func(s *Server) {
s.restClientCache = c
s.ghProviders = g
}
}

Expand All @@ -164,7 +166,7 @@ func NewServer(
evt events.Publisher,
cfg *serverconfig.Config,
vldtr auth.JwtValidator,
restCacheClient ratecache.RestClientCache,
restClientCache ratecache.RestClientCache,
providerStore providers.ProviderStore,
opts ...ServerOption,
) (*Server, error) {
Expand All @@ -185,6 +187,37 @@ func NewServer(
}
fallbackTokenClient := ghprov.NewFallbackTokenClient(cfg.Provider)

// See the comment on this struct. Ideally we would detach
// `s.makeProjectForGitHubApp` from the Server struct and pass it to
// NewGitHubProviderService directly. However, `s.makeProjectForGitHubApp`
// depends on the authz client, which gets set in `opts`, and we want to
// allow this to be overridden in the server options. Fixing this issue
// will require further refactoring.
projectFactoryWrapper := &service.ProjectFactoryWrapper{}
ghProviders := service.NewGithubProviderService(
store,
eng,
mt,
&cfg.Provider,
projectFactoryWrapper,
ghClientFactory,
)

githubProviderManager := ghmanager.NewGitHubProviderClassManager(
restClientCache,
ghClientFactory,
&cfg.Provider,
fallbackTokenClient,
eng,
whManager,
store,
ghProviders,
)
providerManager, err := manager.NewProviderManager(providerStore, githubProviderManager)
if err != nil {
return nil, fmt.Errorf("failed to create provider manager: %w", err)
}

s := &Server{
store: store,
cfg: cfg,
Expand All @@ -201,7 +234,10 @@ func NewServer(
featureFlags: openfeature.NewClient(cfg.Flags.AppName),
ghClient: &ghprov.ClientServiceImplementation{},
fallbackTokenClient: fallbackTokenClient,
restClientCache: restCacheClient,
ghProviders: ghProviders,
providerManager: providerManager,
repos: github.NewRepositoryService(whManager, store, evt, providerManager),
restClientCache: restClientCache,
// TODO: this currently always returns authorized as a transitionary measure.
// When OpenFGA is fully rolled out, we may want to make this a hard error or set to false.
authzClient: &mock.NoopClient{Authorized: true},
Expand All @@ -215,28 +251,10 @@ func NewServer(
opt(s)
}

// TODO: Now that RestCacheClient is passed around directly, this will be
// cleaned up in a future PR.
// Moved here because we have a dependency on s.restClientCache
s.ghProviders = service.NewGithubProviderService(
store, eng, mt, provMt, &cfg.Provider, s.makeProjectForGitHubApp, s.restClientCache, s.fallbackTokenClient)

githubProviderManager := ghmanager.NewGitHubProviderClassManager(
s.restClientCache,
ghClientFactory,
&cfg.Provider,
fallbackTokenClient,
eng,
whManager,
store,
s.ghProviders,
)
providerManager, err := manager.NewProviderManager(providerStore, githubProviderManager)
if err != nil {
return nil, fmt.Errorf("failed to create provider manager: %w", err)
}
s.providerManager = providerManager
s.repos = github.NewRepositoryService(whManager, store, evt, providerManager)
// Finally set the ProjectFactory
// TODO: Get rid more of the override functions (specifically for the
// authz client) so that we can get rid of this hack.
projectFactoryWrapper.Factory = s.makeProjectForGitHubApp

return s, nil
}
Expand Down
26 changes: 24 additions & 2 deletions internal/controlplane/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,12 @@ import (
mockdb "github.com/stacklok/minder/database/mock"
mockjwt "github.com/stacklok/minder/internal/auth/mock"
serverconfig "github.com/stacklok/minder/internal/config/server"
"github.com/stacklok/minder/internal/controlplane/metrics"
"github.com/stacklok/minder/internal/crypto"
"github.com/stacklok/minder/internal/events"
"github.com/stacklok/minder/internal/providers"
"github.com/stacklok/minder/internal/providers/github/clients"
ghService "github.com/stacklok/minder/internal/providers/github/service"
"github.com/stacklok/minder/internal/providers/ratecache"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
)
Expand Down Expand Up @@ -65,7 +69,11 @@ func init() {
// It would be nice if we could Close() the httpServer, but we leak it in the test instead
}

func newDefaultServer(t *testing.T, mockStore *mockdb.MockStore, opts ...ServerOption) (*Server, events.Interface) {
func newDefaultServer(
t *testing.T,
mockStore *mockdb.MockStore,
clientFactory clients.GitHubClientFactory,
) (*Server, events.Interface) {
t.Helper()

evt, err := events.Setup(context.Background(), &serverconfig.EventConfig{
Expand All @@ -85,14 +93,28 @@ func newDefaultServer(t *testing.T, mockStore *mockdb.MockStore, opts ...ServerO
defer ctrl.Finish()
mockJwt := mockjwt.NewMockJwtValidator(ctrl)

// Ugly, but needed to keep these tests working as-is.
// In future, beef up unit test coverage in the dependencies
// of this code, and refactor these tests to use stubs.
eng, err := crypto.EngineFromAuthConfig(&c.Auth)
require.NoError(t, err)
ghClientService := ghService.NewGithubProviderService(
mockStore,
eng,
metrics.NewNoopMetrics(),
nil,
nil,
clientFactory,
)

server, err := NewServer(
mockStore,
evt,
c,
mockJwt,
&ratecache.NoopRestClientCache{},
providers.NewProviderStore(mockStore),
opts...,
WithGitHubProviderService(ghClientService),
)
require.NoError(t, err, "failed to create server")
return server, evt
Expand Down
4 changes: 2 additions & 2 deletions internal/engine/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func NewExecutor(
provCfg *serverconfig.ProviderConfig,
evt events.Publisher,
providerStore providers.ProviderStore,
restCacheClient ratecache.RestClientCache,
restClientCache ratecache.RestClientCache,
opts ...ExecutorOption,
) (*Executor, error) {
crypteng, err := crypto.EngineFromAuthConfig(authCfg)
Expand All @@ -115,7 +115,7 @@ func NewExecutor(
provCfg: provCfg,
providerStore: providerStore,
fallbackTokenClient: fallbackTokenClient,
restClientCache: restCacheClient,
restClientCache: restClientCache,
}

for _, opt := range opts {
Expand Down
6 changes: 3 additions & 3 deletions internal/projects/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
"github.com/stacklok/minder/internal/config/server"
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/marketplaces"
github "github.com/stacklok/minder/internal/providers/github/oauth"
github "github.com/stacklok/minder/internal/providers/github/clients"
"github.com/stacklok/minder/pkg/mindpak"
)

Expand Down Expand Up @@ -61,9 +61,9 @@ func ProvisionSelfEnrolledOAuthProject(
Name: github.Github,
ProjectID: project.ID,
Class: db.ProviderClassGithub,
Implements: github.Implements,
Implements: github.OAuthImplements,
Definition: json.RawMessage(`{"github": {}}`),
AuthFlows: github.AuthorizationFlows,
AuthFlows: github.OAuthAuthorizationFlows,
})
if err != nil {
return nil, fmt.Errorf("failed to create provider: %v", err)
Expand Down
Loading

0 comments on commit 64aa299

Please sign in to comment.