public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

AuthN: Set oauth client grant_types based on plugin state (#77248)

Browse Source 
* Disable plugin service account

* Fix bug seen by linoman 💯

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>

* Account for PR feedback

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>

* Fix test data

* Enable datasource plugins by default

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>

* Update pkg/services/extsvcauth/oauthserver/oasimpl/service.go

* Handle error differently

* Fix service reg

---------

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>
Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
This commit is contained in:
Gabriel MABILLE
2023-10-27 14:45:04 +02:00
committed by GitHub
co-authored by linoman Andres Martinez Gotor
parent 214535c1a9
commit 83e9088314
13 changed files with 226 additions and 77 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/extsvcauth/oauthserver/oasimpl/aggregate_store_test.go
+1 -1
View File
@@ -46,7 +46,7 @@ func TestOAuth2ServiceImpl_GetPublicKeyScopes(t *testing.T) {
{
name: "should error out when GetExternalService returns error",
initTestEnv: func(env *TestEnv) {
env.OAuthStore.On("GetExternalService", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFound("my-ext-service"))
env.OAuthStore.On("GetExternalService", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFoundFn("my-ext-service"))
},
wantErr: true,
},
pkg/services/extsvcauth/oauthserver/oasimpl/service.go
+70 -30
View File
@@ -22,6 +22,7 @@ import (
"golang.org/x/crypto/bcrypt"
"github.com/grafana/grafana/pkg/api/routing"
"github.com/grafana/grafana/pkg/bus"
"github.com/grafana/grafana/pkg/infra/db"
"github.com/grafana/grafana/pkg/infra/localcache"
"github.com/grafana/grafana/pkg/infra/log"
@@ -33,12 +34,12 @@ import (
"github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver/store"
"github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver/utils"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings"
"github.com/grafana/grafana/pkg/services/serviceaccounts"
"github.com/grafana/grafana/pkg/services/signingkeys"
"github.com/grafana/grafana/pkg/services/team"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/util/errutil"
)
const (
@@ -61,7 +62,7 @@ type OAuth2ServiceImpl struct {
publicKey any
}
func ProvideService(router routing.RouteRegister, db db.DB, cfg *setting.Cfg,
func ProvideService(router routing.RouteRegister, bus bus.Bus, db db.DB, cfg *setting.Cfg,
extSvcAccSvc serviceaccounts.ExtSvcAccountsService, accessControl ac.AccessControl, acSvc ac.Service, userSvc user.Service,
teamSvc team.Service, keySvc signingkeys.Service, fmgmt *featuremgmt.FeatureManager) (*OAuth2ServiceImpl, error) {
if !fmgmt.IsEnabled(featuremgmt.FlagExternalServiceAuth) {
@@ -91,6 +92,8 @@ func ProvideService(router routing.RouteRegister, db db.DB, cfg *setting.Cfg,
api := api.NewAPI(router, s)
api.RegisterAPIEndpoints()
bus.AddEventListener(s.handlePluginStateChanged)
s.oauthProvider = newProvider(config, s, keySvc)
return s, nil
@@ -134,44 +137,50 @@ func (s *OAuth2ServiceImpl) GetExternalService(ctx context.Context, id string) (
return nil, err
}
// Handle the case where the external service has no service account
if err := s.setClientUser(ctx, client); err != nil {
return nil, err
}
s.cache.Set(id, *client, cacheExpirationTime)
return client, nil
}
// setClientUser sets the SignedInUser and SelfPermissions fields of the client
func (s *OAuth2ServiceImpl) setClientUser(ctx context.Context, client *oauthserver.OAuthExternalService) error {
if client.ServiceAccountID == oauthserver.NoServiceAccountID {
s.logger.Debug("GetExternalService: service has no service account, hence no permission", "id", id, "name", client.Name)
// Create a signed in user with no role and no permissions
s.logger.Debug("GetExternalService: service has no service account, hence no permission", "client_id", client.ClientID, "name", client.Name)
// Create a signed in user with no role and no permission
client.SignedInUser = &user.SignedInUser{
UserID: oauthserver.NoServiceAccountID,
OrgID: oauthserver.TmpOrgID,
Name: client.Name,
Permissions: map[int64]map[string][]string{oauthserver.TmpOrgID: {}},
}
s.cache.Set(id, *client, cacheExpirationTime)
return client, nil
return nil
}
// Retrieve self permissions and generate a signed in user
s.logger.Debug("GetExternalService: fetch permissions", "client id", id)
s.logger.Debug("GetExternalService: fetch permissions", "client_id", client.ClientID)
sa, err := s.saService.RetrieveExtSvcAccount(ctx, oauthserver.TmpOrgID, client.ServiceAccountID)
if err != nil {
s.logger.Error("GetExternalService: error fetching service account", "id", id, "error", err)
return nil, err
s.logger.Error("GetExternalService: error fetching service account", "id", client.ClientID, "error", err)
return err
}
client.SignedInUser = &user.SignedInUser{
UserID: sa.ID,
OrgID: oauthserver.TmpOrgID,
OrgRole: sa.Role, // Need this to compute the permissions in OSS
OrgRole: sa.Role,
Login: sa.Login,
Name: sa.Name,
Permissions: map[int64]map[string][]string{},
}
client.SelfPermissions, err = s.acService.GetUserPermissions(ctx, client.SignedInUser, ac.Options{})
if err != nil {
s.logger.Error("GetExternalService: error fetching permissions", "id", id, "error", err)
return nil, err
s.logger.Error("GetExternalService: error fetching permissions", "client_id", client.ClientID, "error", err)
return err
}
client.SignedInUser.Permissions[oauthserver.TmpOrgID] = ac.GroupScopesByAction(client.SelfPermissions)
s.cache.Set(id, *client, cacheExpirationTime)
return client, nil
return nil
}
// SaveExternalService creates or updates an external service in the database, it generates client_id and secrets and
@@ -186,14 +195,9 @@ func (s *OAuth2ServiceImpl) SaveExternalService(ctx context.Context, registratio
// Check if the client already exists in store
client, errFetchExtSvc := s.sqlstore.GetExternalServiceByName(ctx, registration.Name)
if errFetchExtSvc != nil {
var srcError errutil.Error
if errors.As(errFetchExtSvc, &srcError) {
if srcError.MessageID != oauthserver.ErrClientNotFoundMessageID {
s.logger.Error("Error fetching service", "external service", registration.Name, "error", errFetchExtSvc)
return nil, errFetchExtSvc
}
}
if errFetchExtSvc != nil && !errors.Is(errFetchExtSvc, oauthserver.ErrClientNotFound) {
s.logger.Error("Error fetching service", "external service", registration.Name, "error", errFetchExtSvc)
return nil, errFetchExtSvc
}
// Otherwise, create a new client
if client == nil {
@@ -386,13 +390,10 @@ func (s *OAuth2ServiceImpl) handleKeyOptions(ctx context.Context, keyOption *ext
// handleRegistrationPermissions parses the registration form to retrieve requested permissions and adds default
// permissions when impersonation is requested
func (*OAuth2ServiceImpl) handleRegistrationPermissions(registration *extsvcauth.ExternalServiceRegistration) ([]ac.Permission, []ac.Permission) {
selfPermissions := []ac.Permission{}
selfPermissions := registration.Self.Permissions
impersonatePermissions := []ac.Permission{}
if registration.Self.Enabled {
selfPermissions = append(selfPermissions, registration.Self.Permissions...)
}
if registration.Impersonation.Enabled {
if len(registration.Impersonation.Permissions) > 0 {
requiredForToken := []ac.Permission{
{Action: ac.ActionUsersRead, Scope: oauthserver.ScopeGlobalUsersSelf},
{Action: ac.ActionUsersPermissionsRead, Scope: oauthserver.ScopeUsersSelf},
@@ -405,3 +406,42 @@ func (*OAuth2ServiceImpl) handleRegistrationPermissions(registration *extsvcauth
}
return selfPermissions, impersonatePermissions
}
// handlePluginStateChanged reset the client authorized grant_types according to the plugin state
func (s *OAuth2ServiceImpl) handlePluginStateChanged(ctx context.Context, event *pluginsettings.PluginStateChangedEvent) error {
s.logger.Info("Plugin state changed", "pluginId", event.PluginId, "enabled", event.Enabled)
// Retrieve client associated to the plugin
slug := slugify.Slugify(event.PluginId)
client, err := s.sqlstore.GetExternalServiceByName(ctx, slug)
if err != nil {
if errors.Is(err, oauthserver.ErrClientNotFound) {
s.logger.Debug("No external service linked to this plugin", "pluginId", event.PluginId)
return nil
}
s.logger.Error("Error fetching service", "pluginId", event.PluginId, "error", err)
return err
}
// Since we will change the grants, clear cache entry
s.cache.Delete(client.ClientID)
if !event.Enabled {
// Plugin is disabled => remove all grant_types
return s.sqlstore.UpdateExternalServiceGrantTypes(ctx, client.ClientID, "")
}
if err := s.setClientUser(ctx, client); err != nil {
return err
}
// The plugin has self permissions (not only impersonate)
canOnlyImpersonate := len(client.SelfPermissions) == 1 && (client.SelfPermissions[0].Action == ac.ActionUsersImpersonate)
selfEnabled := len(client.SelfPermissions) > 0 && !canOnlyImpersonate
// The plugin declared impersonate permissions
impersonateEnabled := len(client.ImpersonatePermissions) > 0
grantTypes := s.computeGrantTypes(selfEnabled, impersonateEnabled)
return s.sqlstore.UpdateExternalServiceGrantTypes(ctx, client.ClientID, strings.Join(grantTypes, ","))
}
pkg/services/extsvcauth/oauthserver/oasimpl/service_test.go
+108 -4
View File
@@ -24,6 +24,8 @@ import (
"github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver"
"github.com/grafana/grafana/pkg/services/extsvcauth/oauthserver/oastest"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings"
sa "github.com/grafana/grafana/pkg/services/serviceaccounts"
saTests "github.com/grafana/grafana/pkg/services/serviceaccounts/tests"
"github.com/grafana/grafana/pkg/services/signingkeys/signingkeystest"
@@ -114,7 +116,7 @@ func TestOAuth2ServiceImpl_SaveExternalService(t *testing.T) {
name: "should create a new client without permissions",
init: func(env *TestEnv) {
// No client at the beginning
env.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFound(serviceName))
env.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFoundFn(serviceName))
env.OAuthStore.On("SaveExternalService", mock.Anything, mock.Anything).Return(nil)
// Return a service account ID
@@ -139,7 +141,7 @@ func TestOAuth2ServiceImpl_SaveExternalService(t *testing.T) {
name: "should allow client credentials grant with correct permissions",
init: func(env *TestEnv) {
// No client at the beginning
env.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFound(serviceName))
env.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFoundFn(serviceName))
env.OAuthStore.On("SaveExternalService", mock.Anything, mock.Anything).Return(nil)
// Return a service account ID
@@ -175,7 +177,7 @@ func TestOAuth2ServiceImpl_SaveExternalService(t *testing.T) {
name: "should allow jwt bearer grant and set default permissions",
init: func(env *TestEnv) {
// No client at the beginning
env.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFound(serviceName))
env.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFoundFn(serviceName))
env.OAuthStore.On("SaveExternalService", mock.Anything, mock.Anything).Return(nil)
// The service account needs to be created with a permission to impersonate users
env.SAService.On("ManageExtSvcAccount", mock.Anything, mock.Anything).Return(int64(10), nil)
@@ -310,7 +312,7 @@ func TestOAuth2ServiceImpl_GetExternalService(t *testing.T) {
{
name: "should return error when the client was not found",
init: func(env *TestEnv) {
env.OAuthStore.On("GetExternalService", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFound(serviceName))
env.OAuthStore.On("GetExternalService", mock.Anything, mock.Anything).Return(nil, oauthserver.ErrClientNotFoundFn(serviceName))
},
wantErr: true,
},
@@ -474,3 +476,105 @@ mgGaC8vUIigFQVsVB+v/HZ4yG1Rcvysig+tyNk1dZQpozpFc2dGmzHlGhw==
require.True(t, result.Generated)
})
}
func TestOAuth2ServiceImpl_handlePluginStateChanged(t *testing.T) {
pluginID := "my-app"
clientID := "RANDOMID"
impersonatePermission := []ac.Permission{{Action: ac.ActionUsersImpersonate, Scope: ac.ScopeUsersAll}}
selfPermission := append(impersonatePermission, ac.Permission{Action: ac.ActionUsersRead, Scope: ac.ScopeUsersAll})
saID := int64(101)
client := &oauthserver.OAuthExternalService{
ID: 11,
Name: pluginID,
ClientID: clientID,
Secret: "SECRET",
ServiceAccountID: saID,
}
clientWithImpersonate := &oauthserver.OAuthExternalService{
ID: 11,
Name: pluginID,
ClientID: clientID,
Secret: "SECRET",
ImpersonatePermissions: []ac.Permission{
{Action: ac.ActionUsersRead, Scope: ac.ScopeUsersAll},
},
ServiceAccountID: saID,
}
extSvcAcc := &sa.ExtSvcAccount{
ID: saID,
Login: "sa-my-app",
Name: pluginID,
OrgID: extsvcauth.TmpOrgID,
IsDisabled: false,
Role: org.RoleNone,
}
tests := []struct {
name string
init func(*TestEnv)
cmd *pluginsettings.PluginStateChangedEvent
}{
{
name: "should do nothing with not found",
init: func(te *TestEnv) {
te.OAuthStore.On("GetExternalServiceByName", mock.Anything, "unknown").Return(nil, oauthserver.ErrClientNotFoundFn("unknown"))
},
cmd: &pluginsettings.PluginStateChangedEvent{PluginId: "unknown", OrgId: 1, Enabled: false},
},
{
name: "should remove grants",
init: func(te *TestEnv) {
te.OAuthStore.On("GetExternalServiceByName", mock.Anything, pluginID).Return(clientWithImpersonate, nil)
te.OAuthStore.On("UpdateExternalServiceGrantTypes", mock.Anything, clientID, "").Return(nil)
},
cmd: &pluginsettings.PluginStateChangedEvent{PluginId: pluginID, OrgId: 1, Enabled: false},
},
{
name: "should set both grants",
init: func(te *TestEnv) {
te.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(clientWithImpersonate, nil)
te.SAService.On("RetrieveExtSvcAccount", mock.Anything, extsvcauth.TmpOrgID, saID).Return(extSvcAcc, nil)
te.AcStore.On("GetUserPermissions", mock.Anything, mock.Anything, mock.Anything).Return(selfPermission, nil)
te.OAuthStore.On("UpdateExternalServiceGrantTypes", mock.Anything, clientID,
string(fosite.GrantTypeClientCredentials)+","+string(fosite.GrantTypeJWTBearer)).Return(nil)
},
cmd: &pluginsettings.PluginStateChangedEvent{PluginId: pluginID, OrgId: 1, Enabled: true},
},
{
name: "should set impersonate grant",
init: func(te *TestEnv) {
te.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(clientWithImpersonate, nil)
te.SAService.On("RetrieveExtSvcAccount", mock.Anything, extsvcauth.TmpOrgID, saID).Return(extSvcAcc, nil)
te.AcStore.On("GetUserPermissions", mock.Anything, mock.Anything, mock.Anything).Return(impersonatePermission, nil)
te.OAuthStore.On("UpdateExternalServiceGrantTypes", mock.Anything, clientID, string(fosite.GrantTypeJWTBearer)).Return(nil)
},
cmd: &pluginsettings.PluginStateChangedEvent{PluginId: pluginID, OrgId: 1, Enabled: true},
},
{
name: "should set client_credentials grant",
init: func(te *TestEnv) {
te.OAuthStore.On("GetExternalServiceByName", mock.Anything, mock.Anything).Return(client, nil)
te.SAService.On("RetrieveExtSvcAccount", mock.Anything, extsvcauth.TmpOrgID, saID).Return(extSvcAcc, nil)
te.AcStore.On("GetUserPermissions", mock.Anything, mock.Anything, mock.Anything).Return(selfPermission, nil)
te.OAuthStore.On("UpdateExternalServiceGrantTypes", mock.Anything, clientID, string(fosite.GrantTypeClientCredentials)).Return(nil)
},
cmd: &pluginsettings.PluginStateChangedEvent{PluginId: pluginID, OrgId: 1, Enabled: true},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
env := setupTestEnv(t)
if tt.init != nil {
tt.init(env)
}
err := env.S.handlePluginStateChanged(context.Background(), tt.cmd)
require.NoError(t, err)
// Check that mocks were called as expected
env.OAuthStore.AssertExpectations(t)
env.SAService.AssertExpectations(t)
env.AcStore.AssertExpectations(t)
})
}
}