From 51da96d94e616ebf3f980e3bd46c7fd474a28977 Mon Sep 17 00:00:00 2001 From: linoman <2051016+linoman@users.noreply.github.com> Date: Mon, 15 Apr 2024 02:54:50 -0600 Subject: [PATCH] Auth: Add `IsClientEnabled` and `IsEnabled` for the `authn.Service` and `authn.Client` interfaces (#86034) * Add `Service. IsClientEnabled` and `Client.IsEnabled` functions * Implement `IsEnabled` function for authn clients * Implement `IsClientEnabled` function for authn services --- pkg/api/frontendsettings_test.go | 8 +++-- pkg/api/login.go | 3 +- pkg/api/login_test.go | 2 ++ pkg/services/anonymous/anonimpl/client.go | 4 +++ pkg/services/authn/authn.go | 15 ++++++++ pkg/services/authn/authnimpl/service.go | 9 +++++ pkg/services/authn/authntest/fake.go | 8 +++++ pkg/services/authn/authntest/mock.go | 8 +++++ pkg/services/authn/clients/api_key.go | 4 +++ pkg/services/authn/clients/basic.go | 4 +++ pkg/services/authn/clients/ext_jwt.go | 4 +++ pkg/services/authn/clients/form.go | 4 +++ pkg/services/authn/clients/identity.go | 4 +++ pkg/services/authn/clients/jwt.go | 4 +++ pkg/services/authn/clients/oauth.go | 9 +++++ pkg/services/authn/clients/oauth_test.go | 43 +++++++++++++++++++++++ pkg/services/authn/clients/proxy.go | 4 +++ pkg/services/authn/clients/render.go | 4 +++ pkg/services/authn/clients/session.go | 4 +++ 19 files changed, 140 insertions(+), 5 deletions(-) diff --git a/pkg/api/frontendsettings_test.go b/pkg/api/frontendsettings_test.go index d0fbcb0c147..e9bdd97af78 100644 --- a/pkg/api/frontendsettings_test.go +++ b/pkg/api/frontendsettings_test.go @@ -20,6 +20,7 @@ import ( "github.com/grafana/grafana/pkg/plugins/pluginscdn" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" + "github.com/grafana/grafana/pkg/services/authn/authntest" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/licensing" "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings" @@ -58,9 +59,10 @@ func setupTestEnvironment(t *testing.T, cfg *setting.Cfg, features featuremgmt.F } hs := &HTTPServer{ - Cfg: cfg, - Features: features, - License: &licensing.OSSLicensingService{Cfg: cfg}, + authnService: &authntest.FakeService{}, + Cfg: cfg, + Features: features, + License: &licensing.OSSLicensingService{Cfg: cfg}, RenderService: &rendering.RenderingService{ Cfg: cfg, RendererPluginManager: &fakeRendererPluginManager{}, diff --git a/pkg/api/login.go b/pkg/api/login.go index c1638600678..dcea398d4a9 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -12,7 +12,6 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/infra/network" - "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/middleware/cookies" "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/auth/identity" @@ -329,7 +328,7 @@ func (hs *HTTPServer) redirectURLWithErrorCookie(c *contextmodel.ReqContext, err } func (hs *HTTPServer) samlEnabled() bool { - return hs.SettingsProvider.KeyValue("auth.saml", "enabled").MustBool(false) && hs.License.FeatureEnabled(social.SAMLProviderName) + return hs.authnService.IsClientEnabled(authn.ClientSAML) } func (hs *HTTPServer) samlName() string { diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index 4c8dcd77742..1d4c725f23c 100644 --- a/pkg/api/login_test.go +++ b/pkg/api/login_test.go @@ -491,6 +491,7 @@ func TestLoginOAuthRedirect(t *testing.T) { oAuthInfos: oAuthInfos, } hs := &HTTPServer{ + authnService: &authntest.FakeService{}, Cfg: cfg, SettingsProvider: &setting.OSSImpl{Cfg: cfg}, License: &licensing.OSSLicensingService{}, @@ -657,6 +658,7 @@ func TestLogoutSaml(t *testing.T) { license.On("FeatureEnabled", "saml").Return(true) hs := &HTTPServer{ + authnService: &authntest.FakeService{}, Cfg: sc.cfg, SettingsProvider: &setting.OSSImpl{Cfg: sc.cfg}, License: license, diff --git a/pkg/services/anonymous/anonimpl/client.go b/pkg/services/anonymous/anonimpl/client.go index a74c9367cdd..c93d4d40567 100644 --- a/pkg/services/anonymous/anonimpl/client.go +++ b/pkg/services/anonymous/anonimpl/client.go @@ -58,6 +58,10 @@ func (a *Anonymous) Authenticate(ctx context.Context, r *authn.Request) (*authn. }, nil } +func (a *Anonymous) IsEnabled() bool { + return a.cfg.AnonymousEnabled +} + func (a *Anonymous) Test(ctx context.Context, r *authn.Request) bool { // If anonymous client is register it can always be used for authentication return true diff --git a/pkg/services/authn/authn.go b/pkg/services/authn/authn.go index d50627721cd..c2ec983b72a 100644 --- a/pkg/services/authn/authn.go +++ b/pkg/services/authn/authn.go @@ -94,6 +94,19 @@ type Service interface { // RegisterClient will register a new authn.Client that can be used for authentication RegisterClient(c Client) + + // IsClientEnabled returns true if the client is enabled. + // + // The client lookup follows the same formats used by the `authn` package + // constants. + // + // For OAuth clients, use the `authn.ClientWithPrefix(name)` to get the provider + // name. Append the prefix `auth.client.{providerName}`. + // + // Example: + // - "saml" = "auth.client.saml" + // - "github" = "auth.client.github" + IsClientEnabled(client string) bool } type IdentitySynchronizer interface { @@ -105,6 +118,8 @@ type Client interface { Name() string // Authenticate performs the authentication for the request Authenticate(ctx context.Context, r *Request) (*Identity, error) + // IsEnabled returns the enabled status of the client + IsEnabled() bool } // ContextAwareClient is an optional interface that auth client can implement. diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 2e40100ebe8..d6718f11cf4 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -320,6 +320,15 @@ func (s *Service) RegisterClient(c authn.Client) { } } +func (s *Service) IsClientEnabled(name string) bool { + client, ok := s.clients[name] + if !ok { + return false + } + + return client.IsEnabled() +} + func (s *Service) SyncIdentity(ctx context.Context, identity *authn.Identity) error { r := &authn.Request{OrgID: identity.OrgID} // hack to not update last seen on external syncs diff --git a/pkg/services/authn/authntest/fake.go b/pkg/services/authn/authntest/fake.go index 02bc1f11944..c65b2d7f7fb 100644 --- a/pkg/services/authn/authntest/fake.go +++ b/pkg/services/authn/authntest/fake.go @@ -40,6 +40,10 @@ func (f *FakeService) Authenticate(ctx context.Context, r *authn.Request) (*auth return f.ExpectedIdentity, f.ExpectedErr } +func (f *FakeService) IsClientEnabled(name string) bool { + return true +} + func (f *FakeService) RegisterPostAuthHook(hook authn.PostAuthHookFn, priority uint) {} func (f *FakeService) Login(ctx context.Context, client string, r *authn.Request) (*authn.Identity, error) { @@ -119,6 +123,8 @@ func (f *FakeClient) Authenticate(ctx context.Context, r *authn.Request) (*authn return f.ExpectedIdentity, f.ExpectedErr } +func (f FakeClient) IsEnabled() bool { return true } + func (f *FakeClient) Test(ctx context.Context, r *authn.Request) bool { return f.ExpectedTest } @@ -161,6 +167,8 @@ func (f FakeRedirectClient) Authenticate(ctx context.Context, r *authn.Request) return f.ExpectedIdentity, f.ExpectedErr } +func (f FakeRedirectClient) IsEnabled() bool { return true } + func (f FakeRedirectClient) RedirectURL(ctx context.Context, r *authn.Request) (*authn.Redirect, error) { return f.ExpectedRedirect, f.ExpectedErr } diff --git a/pkg/services/authn/authntest/mock.go b/pkg/services/authn/authntest/mock.go index 5568bb0f8da..f2d7d77d9b1 100644 --- a/pkg/services/authn/authntest/mock.go +++ b/pkg/services/authn/authntest/mock.go @@ -20,6 +20,10 @@ func (m *MockService) Authenticate(ctx context.Context, r *authn.Request) (*auth panic("unimplemented") } +func (m *MockService) IsClientEnabled(name string) bool { + panic("unimplemented") +} + func (m *MockService) Login(ctx context.Context, client string, r *authn.Request) (*authn.Identity, error) { panic("unimplemented") } @@ -87,6 +91,10 @@ func (m MockClient) Authenticate(ctx context.Context, r *authn.Request) (*authn. return nil, nil } +func (m MockClient) IsEnabled() bool { + return true +} + func (m MockClient) Test(ctx context.Context, r *authn.Request) bool { if m.TestFunc != nil { return m.TestFunc(ctx, r) diff --git a/pkg/services/authn/clients/api_key.go b/pkg/services/authn/clients/api_key.go index c8582d02fda..c2679470fc7 100644 --- a/pkg/services/authn/clients/api_key.go +++ b/pkg/services/authn/clients/api_key.go @@ -70,6 +70,10 @@ func (s *APIKey) Authenticate(ctx context.Context, r *authn.Request) (*authn.Ide return newServiceAccountIdentity(key), nil } +func (s *APIKey) IsEnabled() bool { + return true +} + func (s *APIKey) getAPIKey(ctx context.Context, token string) (*apikey.APIKey, error) { fn := s.getFromToken if !strings.HasPrefix(token, satokengen.GrafanaPrefix) { diff --git a/pkg/services/authn/clients/basic.go b/pkg/services/authn/clients/basic.go index d0c7050f5bf..e714cc03765 100644 --- a/pkg/services/authn/clients/basic.go +++ b/pkg/services/authn/clients/basic.go @@ -38,6 +38,10 @@ func (c *Basic) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden return c.client.AuthenticatePassword(ctx, r, username, password) } +func (c *Basic) IsEnabled() bool { + return true +} + func (c *Basic) Test(ctx context.Context, r *authn.Request) bool { if r.HTTPRequest == nil { return false diff --git a/pkg/services/authn/clients/ext_jwt.go b/pkg/services/authn/clients/ext_jwt.go index 58533d0d0d2..303b14d9cad 100644 --- a/pkg/services/authn/clients/ext_jwt.go +++ b/pkg/services/authn/clients/ext_jwt.go @@ -91,6 +91,10 @@ func (s *ExtendedJWT) Authenticate(ctx context.Context, r *authn.Request) (*auth return s.authenticateAsService(claims) } +func (s *ExtendedJWT) IsEnabled() bool { + return s.cfg.ExtJWTAuth.Enabled +} + func (s *ExtendedJWT) authenticateAsUser(idTokenClaims, accessTokenClaims *ExtendedJWTClaims) (*authn.Identity, error) { // Only allow access policies to impersonate diff --git a/pkg/services/authn/clients/form.go b/pkg/services/authn/clients/form.go index a16a884a35c..b64e2b06ef7 100644 --- a/pkg/services/authn/clients/form.go +++ b/pkg/services/authn/clients/form.go @@ -38,3 +38,7 @@ func (c *Form) Authenticate(ctx context.Context, r *authn.Request) (*authn.Ident } return c.client.AuthenticatePassword(ctx, r, form.Username, form.Password) } + +func (c *Form) IsEnabled() bool { + return true +} diff --git a/pkg/services/authn/clients/identity.go b/pkg/services/authn/clients/identity.go index 3a6fa745bc0..faf47907334 100644 --- a/pkg/services/authn/clients/identity.go +++ b/pkg/services/authn/clients/identity.go @@ -20,6 +20,10 @@ func (i *IdentityClient) Name() string { return "identity" } +func (i *IdentityClient) IsEnabled() bool { + return true +} + func (i *IdentityClient) Authenticate(ctx context.Context, r *authn.Request) (*authn.Identity, error) { return i.identity, nil } diff --git a/pkg/services/authn/clients/jwt.go b/pkg/services/authn/clients/jwt.go index 4c741fd2ab0..3e49d31d379 100644 --- a/pkg/services/authn/clients/jwt.go +++ b/pkg/services/authn/clients/jwt.go @@ -139,6 +139,10 @@ func (s *JWT) Authenticate(ctx context.Context, r *authn.Request) (*authn.Identi return id, nil } +func (s *JWT) IsEnabled() bool { + return s.cfg.JWTAuth.Enabled +} + // remove sensitive query param // avoid JWT URL login passing auth_token in URL func (s *JWT) stripSensitiveParam(httpRequest *http.Request) { diff --git a/pkg/services/authn/clients/oauth.go b/pkg/services/authn/clients/oauth.go index e9184f41e37..0a446102780 100644 --- a/pkg/services/authn/clients/oauth.go +++ b/pkg/services/authn/clients/oauth.go @@ -199,6 +199,15 @@ func (c *OAuth) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden }, nil } +func (c *OAuth) IsEnabled() bool { + provider := c.socialService.GetOAuthInfoProvider(c.providerName) + if provider == nil { + return false + } + + return provider.Enabled +} + func (c *OAuth) RedirectURL(ctx context.Context, r *authn.Request) (*authn.Redirect, error) { var opts []oauth2.AuthCodeOption diff --git a/pkg/services/authn/clients/oauth_test.go b/pkg/services/authn/clients/oauth_test.go index a746c2d4633..65916f3b435 100644 --- a/pkg/services/authn/clients/oauth_test.go +++ b/pkg/services/authn/clients/oauth_test.go @@ -507,6 +507,49 @@ func TestGenPKCECodeVerifier(t *testing.T) { assert.Len(t, verifier, 128) } +func TestIsEnabled(t *testing.T) { + type testCase struct { + desc string + oauthCfg *social.OAuthInfo + expected bool + } + + tests := []testCase{ + { + desc: "should return false when client is not enabled", + oauthCfg: &social.OAuthInfo{Enabled: false}, + expected: false, + }, + { + desc: "should return false when client doesnt exists", + oauthCfg: nil, + expected: false, + }, + { + desc: "should return true when client is enabled", + oauthCfg: &social.OAuthInfo{Enabled: true}, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + fakeSocialSvc := &socialtest.FakeSocialService{ + ExpectedAuthInfoProvider: tt.oauthCfg, + } + cfg := setting.NewCfg() + c := ProvideOAuth( + social.GitHubProviderName, + cfg, + nil, + fakeSocialSvc, + &setting.OSSImpl{Cfg: cfg}, + featuremgmt.WithFeatures()) + assert.Equal(t, tt.expected, c.IsEnabled()) + }) + } +} + type mockConnector struct { AuthCodeURLFunc func(state string, opts ...oauth2.AuthCodeOption) string social.SocialConnector diff --git a/pkg/services/authn/clients/proxy.go b/pkg/services/authn/clients/proxy.go index cd6a8907615..157af7ee0af 100644 --- a/pkg/services/authn/clients/proxy.go +++ b/pkg/services/authn/clients/proxy.go @@ -107,6 +107,10 @@ func (c *Proxy) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden return nil, clientErr } +func (c *Proxy) IsEnabled() bool { + return c.cfg.AuthProxy.Enabled +} + // See if we have cached the user id, in that case we can fetch the signed-in user and skip sync. // Error here means that we could not find anything in cache, so we can proceed as usual func (c *Proxy) retrieveIDFromCache(ctx context.Context, cacheKey string, r *authn.Request) (*authn.Identity, error) { diff --git a/pkg/services/authn/clients/render.go b/pkg/services/authn/clients/render.go index e5a1b6970a4..0a7b692029a 100644 --- a/pkg/services/authn/clients/render.go +++ b/pkg/services/authn/clients/render.go @@ -59,6 +59,10 @@ func (c *Render) Authenticate(ctx context.Context, r *authn.Request) (*authn.Ide }, nil } +func (c *Render) IsEnabled() bool { + return true +} + func (c *Render) Test(ctx context.Context, r *authn.Request) bool { if r.HTTPRequest == nil { return false diff --git a/pkg/services/authn/clients/session.go b/pkg/services/authn/clients/session.go index 014192c27d1..02ec093b74e 100644 --- a/pkg/services/authn/clients/session.go +++ b/pkg/services/authn/clients/session.go @@ -78,6 +78,10 @@ func (s *Session) Authenticate(ctx context.Context, r *authn.Request) (*authn.Id return ident, nil } +func (s *Session) IsEnabled() bool { + return true +} + func (s *Session) Test(ctx context.Context, r *authn.Request) bool { if s.cfg.LoginCookieName == "" { return false