diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 3718375b3c4..37dcc4b324d 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -23,9 +23,14 @@ import ( "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util/errutil" "github.com/grafana/grafana/pkg/web" ) +var ( + errDisabledIdentity = errutil.NewBase(errutil.StatusUnauthorized, "identity.disabled") +) + // make sure service implements authn.Service interface var _ authn.Service = new(Service) @@ -129,6 +134,10 @@ func (s *Service) Authenticate(ctx context.Context, client string, r *authn.Requ } } + if identity.IsDisabled { + return nil, true, errDisabledIdentity.Errorf("identity is disabled") + } + return identity, true, nil } diff --git a/pkg/services/authn/authnimpl/service_test.go b/pkg/services/authn/authnimpl/service_test.go index 51d84481374..af7d04e9c37 100644 --- a/pkg/services/authn/authnimpl/service_test.go +++ b/pkg/services/authn/authnimpl/service_test.go @@ -22,17 +22,22 @@ import ( func TestService_Authenticate(t *testing.T) { type TestCase struct { - desc string - clientName string - expectedOK bool - expectedErr error + desc string + clientName string + clientErr error + clientIdentity *authn.Identity + expectedOK bool + expectedErr error } + var clientErr = errors.New("some err") + tests := []TestCase{ { - desc: "should succeed with authentication for configured client", - clientName: "fake", - expectedOK: true, + desc: "should succeed with authentication for configured client", + clientIdentity: &authn.Identity{}, + clientName: "fake", + expectedOK: true, }, { desc: "should return false when client is not configured", @@ -43,7 +48,15 @@ func TestService_Authenticate(t *testing.T) { desc: "should return true and error when client could be used but failed to authenticate", clientName: "fake", expectedOK: true, - expectedErr: errors.New("some error"), + clientErr: clientErr, + expectedErr: clientErr, + }, + { + desc: "should return error if identity is disabled", + clientName: "fake", + clientIdentity: &authn.Identity{IsDisabled: true}, + expectedOK: true, + expectedErr: errDisabledIdentity, }, } @@ -51,16 +64,15 @@ func TestService_Authenticate(t *testing.T) { t.Run(tt.desc, func(t *testing.T) { svc := setupTests(t, func(svc *Service) { svc.clients["fake"] = &authntest.FakeClient{ - ExpectedErr: tt.expectedErr, - ExpectedTest: tt.expectedOK, + ExpectedIdentity: tt.clientIdentity, + ExpectedErr: tt.clientErr, + ExpectedTest: tt.expectedOK, } }) _, ok, err := svc.Authenticate(context.Background(), tt.clientName, &authn.Request{}) assert.Equal(t, tt.expectedOK, ok) - if tt.expectedErr != nil { - assert.Error(t, err) - } + assert.ErrorIs(t, err, tt.expectedErr) }) } } @@ -114,7 +126,7 @@ func TestService_AuthenticateOrgID(t *testing.T) { svc.clients["fake"] = authntest.MockClient{ AuthenticateFunc: func(ctx context.Context, r *authn.Request) (*authn.Identity, error) { calledWith = r.OrgID - return nil, nil + return &authn.Identity{}, nil }, TestFunc: func(ctx context.Context, r *authn.Request) bool { return true diff --git a/pkg/services/authn/clients/api_key.go b/pkg/services/authn/clients/api_key.go index 0b375d4b525..ab684a5672d 100644 --- a/pkg/services/authn/clients/api_key.go +++ b/pkg/services/authn/clients/api_key.go @@ -18,10 +18,9 @@ import ( ) var ( - ErrAPIKeyInvalid = errutil.NewBase(errutil.StatusUnauthorized, "api-key.invalid", errutil.WithPublicMessage("Invalid API key")) - ErrAPIKeyExpired = errutil.NewBase(errutil.StatusUnauthorized, "api-key.expired", errutil.WithPublicMessage("Expired API key")) - ErrAPIKeyRevoked = errutil.NewBase(errutil.StatusUnauthorized, "api-key.revoked", errutil.WithPublicMessage("Revoked API key")) - ErrServiceAccountDisabled = errutil.NewBase(errutil.StatusUnauthorized, "service-account.disabled", errutil.WithPublicMessage("Disabled service account")) + errAPIKeyInvalid = errutil.NewBase(errutil.StatusUnauthorized, "api-key.invalid", errutil.WithPublicMessage("Invalid API key")) + errAPIKeyExpired = errutil.NewBase(errutil.StatusUnauthorized, "api-key.expired", errutil.WithPublicMessage("Expired API key")) + errAPIKeyRevoked = errutil.NewBase(errutil.StatusUnauthorized, "api-key.revoked", errutil.WithPublicMessage("Revoked API key")) ) var _ authn.Client = new(APIKey) @@ -44,17 +43,17 @@ func (s *APIKey) Authenticate(ctx context.Context, r *authn.Request) (*authn.Ide apiKey, err := s.getAPIKey(ctx, getTokenFromRequest(r)) if err != nil { if errors.Is(err, apikeygen.ErrInvalidApiKey) { - return nil, ErrAPIKeyInvalid.Errorf("API key is invalid") + return nil, errAPIKeyInvalid.Errorf("API key is invalid") } return nil, err } if apiKey.Expires != nil && *apiKey.Expires <= time.Now().Unix() { - return nil, ErrAPIKeyExpired.Errorf("API key has expired") + return nil, errAPIKeyExpired.Errorf("API key has expired") } if apiKey.IsRevoked != nil && *apiKey.IsRevoked { - return nil, ErrAPIKeyRevoked.Errorf("Api key is revoked") + return nil, errAPIKeyRevoked.Errorf("Api key is revoked") } go func(id int64) { @@ -86,10 +85,6 @@ func (s *APIKey) Authenticate(ctx context.Context, r *authn.Request) (*authn.Ide return nil, err } - if usr.IsDisabled { - return nil, ErrServiceAccountDisabled.Errorf("Disabled service account") - } - return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceServiceAccount, usr.UserID), usr, authn.ClientParams{}), nil } diff --git a/pkg/services/authn/clients/api_key_test.go b/pkg/services/authn/clients/api_key_test.go index cb4e4bf394c..205c0ec43ee 100644 --- a/pkg/services/authn/clients/api_key_test.go +++ b/pkg/services/authn/clients/api_key_test.go @@ -7,6 +7,8 @@ import ( "net/http" "testing" + "github.com/stretchr/testify/assert" + "github.com/grafana/grafana/pkg/components/apikeygen" apikeygenprefix "github.com/grafana/grafana/pkg/components/apikeygenprefixed" "github.com/grafana/grafana/pkg/services/apikey" @@ -15,7 +17,6 @@ import ( "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/services/user/usertest" - "github.com/stretchr/testify/assert" ) var ( @@ -90,7 +91,7 @@ func TestAPIKey_Authenticate(t *testing.T) { Key: hash, Expires: intPtr(0), }, - expectedErr: ErrAPIKeyExpired, + expectedErr: errAPIKeyExpired, }, { desc: "should fail for revoked api key", @@ -99,17 +100,7 @@ func TestAPIKey_Authenticate(t *testing.T) { Key: hash, IsRevoked: &revoked, }, - expectedErr: ErrAPIKeyRevoked, - }, - { - desc: "should fail if service account is disabled", - req: &authn.Request{HTTPRequest: &http.Request{Header: map[string][]string{"Authorization": {"Bearer " + secret}}}}, - expectedKey: &apikey.APIKey{ - Key: hash, - ServiceAccountId: intPtr(1), - }, - expectedUser: &user.SignedInUser{IsDisabled: true}, - expectedErr: ErrServiceAccountDisabled, + expectedErr: errAPIKeyRevoked, }, }