diff --git a/docs/sources/administration/roles-and-permissions/access-control/custom-role-actions-scopes/index.md b/docs/sources/administration/roles-and-permissions/access-control/custom-role-actions-scopes/index.md index 31aeaa1a99a..466e0aa99b7 100644 --- a/docs/sources/administration/roles-and-permissions/access-control/custom-role-actions-scopes/index.md +++ b/docs/sources/administration/roles-and-permissions/access-control/custom-role-actions-scopes/index.md @@ -69,8 +69,6 @@ The following list contains role-based access control actions. | `annotations:delete` | | Delete annotations. | | `annotations:read` | | Read annotations and annotation tags. | | `annotations:write` | | Update annotations. | -| `apikeys:read` | | Read API keys. | -| `apikeys:delete` | | Delete API keys. | | `banners:write` | None | Create [announcement banners](/docs/grafana-cloud/whats-new/2024-09-10-announcement-banner/). | | `dashboards:create` | | Create dashboards in one or more folders and their subfolders. | | `dashboards:delete` | | Delete one or more dashboards. | @@ -268,7 +266,6 @@ The following list contains role-based access control scopes. | Scopes | Descriptions | | -------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | | Restrict an action to a set of annotations. For example, `annotations:*` matches any annotation, `annotations:type:dashboard` matches annotations associated with dashboards and `annotations:type:organization` matches organization annotations. | -| | Restrict an action to a set of API keys. For example, `apikeys:*` matches any API key, `apikey:id:1` matches the API key whose id is `1`. | | | Restrict an action to a set of dashboards. For example, `dashboards:*` matches any dashboard, and `dashboards:uid:1` matches the dashboard whose UID is `1`. | | | Restrict an action to a set of data sources. For example, `datasources:*` matches any data source, and `datasources:uid:1` matches the data source whose UID is `1`. | | | Restrict an action to a set of folders. For example, `folders:*` matches any folder, and `folders:uid:1` matches the folder whose UID is `1`. Note that permissions granted to a folder cascade down to subfolders located under it. | diff --git a/docs/sources/administration/roles-and-permissions/access-control/rbac-fixed-basic-role-definitions/index.md b/docs/sources/administration/roles-and-permissions/access-control/rbac-fixed-basic-role-definitions/index.md index e6c7b1fd51c..1495886a032 100644 --- a/docs/sources/administration/roles-and-permissions/access-control/rbac-fixed-basic-role-definitions/index.md +++ b/docs/sources/administration/roles-and-permissions/access-control/rbac-fixed-basic-role-definitions/index.md @@ -54,14 +54,14 @@ The following tables list permissions associated with basic and fixed roles. Thi ## Basic role assignments -| Basic role | UID | Associated fixed roles | Description | -| --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- | +| Basic role | UID | Associated fixed roles | Description | +| --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- | | Grafana Admin | `basic_grafana_admin` | | `fixed:authentication.config:writer`
`fixed:general.auth.config:writer`
`fixed:ldap:writer`
`fixed:licensing:writer`
`fixed:migrationassistant:migrator`
`fixed:org.users:writer`
`fixed:organization:maintainer`
`fixed:plugins:maintainer`
`fixed:provisioning:writer`
`fixed:roles:writer`
`fixed:settings:reader`
`fixed:settings:writer`
`fixed:stats:reader`
`fixed:support.bundles:writer`
`fixed:usagestats:reader`
`fixed:users:writer` | Default [Grafana server administrator](/docs/grafana//administration/roles-and-permissions/#grafana-server-administrators) assignments. | -| Admin | `basic_admin` | All roles assigned to Editor and `fixed:reports:writer`
`fixed:datasources:writer`
`fixed:organization:writer`
`fixed:datasources.permissions:writer`
`fixed:teams:writer`
`fixed:dashboards:writer`
`fixed:dashboards.permissions:writer`
`fixed:dashboards.public:writer`
`fixed:folders:writer`
`fixed:folders.permissions:writer`
`fixed:alerting:writer`
`fixed:apikeys:writer`
`fixed:alerting.provisioning.secrets:reader`
`fixed:alerting.provisioning:writer`
`fixed:datasources.caching:writer`
`fixed:plugins:writer`
`fixed:library.panels:writer` | Default [Grafana organization administrator](ref:rbac-basic-roles) assignments. | -| Editor | `basic_editor` | All roles assigned to Viewer and `fixed:datasources:explorer`
`fixed:dashboards:creator`
`fixed:folders:creator`
`fixed:annotations:writer`
`fixed:alerting:writer`
`fixed:library.panels:creator`
`fixed:library.panels:general.writer`
`fixed:alerting.provisioning.status:writer` | Default [Editor](ref:rbac-basic-roles) assignments. | -| Viewer | `basic_viewer` | `fixed:datasources.id:reader`
`fixed:organization:reader`
`fixed:annotations:reader`
`fixed:annotations.dashboard:writer`
`fixed:alerting:reader`
`fixed:plugins.app:reader`
`fixed:dashboards.insights:reader`
`fixed:datasources.insights:reader`
`fixed:library.panels:general.reader`
`fixed:folders.general:reader`
`fixed:datasources.builtin:reader` | Default [Viewer](ref:rbac-basic-roles) assignments. | -| No Basic Role | n/a | | Default [No Basic Role](ref:rbac-basic-roles) | +| Admin | `basic_admin` | All roles assigned to Editor and `fixed:reports:writer`
`fixed:datasources:writer`
`fixed:organization:writer`
`fixed:datasources.permissions:writer`
`fixed:teams:writer`
`fixed:dashboards:writer`
`fixed:dashboards.permissions:writer`
`fixed:dashboards.public:writer`
`fixed:folders:writer`
`fixed:folders.permissions:writer`
`fixed:alerting:writer`
`fixed:alerting.provisioning.secrets:reader`
`fixed:alerting.provisioning:writer`
`fixed:datasources.caching:writer`
`fixed:plugins:writer`
`fixed:library.panels:writer` | Default [Grafana organization administrator](ref:rbac-basic-roles) assignments. | +| Editor | `basic_editor` | All roles assigned to Viewer and `fixed:datasources:explorer`
`fixed:dashboards:creator`
`fixed:folders:creator`
`fixed:annotations:writer`
`fixed:alerting:writer`
`fixed:library.panels:creator`
`fixed:library.panels:general.writer`
`fixed:alerting.provisioning.status:writer` | Default [Editor](ref:rbac-basic-roles) assignments. | +| Viewer | `basic_viewer` | `fixed:datasources.id:reader`
`fixed:organization:reader`
`fixed:annotations:reader`
`fixed:annotations.dashboard:writer`
`fixed:alerting:reader`
`fixed:plugins.app:reader`
`fixed:dashboards.insights:reader`
`fixed:datasources.insights:reader`
`fixed:library.panels:general.reader`
`fixed:folders.general:reader`
`fixed:datasources.builtin:reader` | Default [Viewer](ref:rbac-basic-roles) assignments. | +| No Basic Role | n/a | | Default [No Basic Role](ref:rbac-basic-roles) | ## Fixed role definitions @@ -90,8 +90,6 @@ To learn how to use the roles API to determine the role UUIDs, refer to [Manage | `fixed:annotations:reader` | `fixed_hpZnoizrfAJsrceNcNQqWYV-xNU` | `annotations:read` for scopes `annotations:type:*` | Read all annotations and annotation tags. | | `fixed:annotations:writer` | `fixed_ZVW-Aa9Tzle6J4s2aUFcq1StKWE` | All permissions from `fixed:annotations:reader`
`annotations:write`
`annotations.create`
`annotations:delete` for scope `annotations:type:*` | Read, create, update and delete all annotations and annotation tags. | | `fixed:annotations.dashboard:writer` | `fixed_8A775xenXeKaJk4Cr7bchP9yXOA` | `annotations:write`
`annotations.create`
`annotations:delete` for scope `annotations:type:dashboard` | Create, update and delete dashboard annotations and annotation tags. | -| `fixed:apikeys:reader` | `fixed_kYZ7UEkwEvGmCCjTrq07cFAVFws` | `apikeys:read` for scope `apikeys:*` | Read all api keys. | -| `fixed:apikeys:writer` | `fixed_anTrcpRkm21NBO1Q2CsX8y0fiCQ` | All permissions from `fixed:apikeys:reader` and
`apikeys:create`
`apikeys:delete` for scope `apikeys:*` | Read, create, delete all api keys. | | `fixed:authentication.config:writer` | `fixed_0rYhZ2Qnzs8AdB1nX7gexk3fHDw` | `settings:read` for scope `settings:auth.saml:*`
`settings:write` for scope `settings:auth.saml:*` | Read and update authentication and SAML settings. | | `fixed:general.auth.config:writer` | `fixed_QFxIT_FGtBqbIVJIwx1bLgI5z6c` | `settings:read` for scope `settings:auth:oauth_allow_insecure_email_lookup`
`settings:write` for scope `settings:auth:oauth_allow_insecure_email_lookup` | Read and update the Grafana instance's general authentication configuration settings. | | `fixed:dashboards:creator` | `fixed_ZorKUcEPCM01A1fPakEzGBUyU64` | `dashboards:create`
`folders:read` | Create dashboards. | diff --git a/pkg/api/accesscontrol.go b/pkg/api/accesscontrol.go index de783d3309e..1bddc9490cc 100644 --- a/pkg/api/accesscontrol.go +++ b/pkg/api/accesscontrol.go @@ -176,41 +176,6 @@ func (hs *HTTPServer) declareFixedRoles() error { Grants: []string{string(org.RoleViewer)}, } - apikeyReaderRole := ac.RoleRegistration{ - Role: ac.RoleDTO{ - Name: "fixed:apikeys:reader", - DisplayName: "Reader", - Description: "Gives access to read api keys.", - Group: "API Keys", - Permissions: []ac.Permission{ - { - Action: ac.ActionAPIKeyRead, - Scope: ac.ScopeAPIKeysAll, - }, - }, - }, - Grants: []string{string(org.RoleAdmin)}, - } - - apikeyWriterRole := ac.RoleRegistration{ - Role: ac.RoleDTO{ - Name: "fixed:apikeys:writer", - DisplayName: "Writer", - Description: "Gives access to add and delete api keys.", - Group: "API Keys", - Permissions: ac.ConcatPermissions(apikeyReaderRole.Role.Permissions, []ac.Permission{ - { - Action: ac.ActionAPIKeyCreate, - }, - { - Action: ac.ActionAPIKeyDelete, - Scope: ac.ScopeAPIKeysAll, - }, - }), - }, - Grants: []string{string(org.RoleAdmin)}, - } - orgReaderRole := ac.RoleRegistration{ Role: ac.RoleDTO{ Name: "fixed:organization:reader", @@ -649,7 +614,7 @@ func (hs *HTTPServer) declareFixedRoles() error { orgMaintainerRole, teamsCreatorRole, teamsWriterRole, teamsReaderRole, datasourcesExplorerRole, annotationsReaderRole, dashboardAnnotationsWriterRole, annotationsWriterRole, dashboardsCreatorRole, dashboardsReaderRole, dashboardsWriterRole, - foldersCreatorRole, foldersReaderRole, generalFolderReaderRole, foldersWriterRole, apikeyReaderRole, apikeyWriterRole, + foldersCreatorRole, foldersReaderRole, generalFolderReaderRole, foldersWriterRole, publicDashboardsWriterRole, featuremgmtReaderRole, featuremgmtWriterRole, libraryPanelsCreatorRole, libraryPanelsReaderRole, libraryPanelsWriterRole, libraryPanelsGeneralReaderRole, libraryPanelsGeneralWriterRole, snapshotsCreatorRole, snapshotsDeleterRole, snapshotsReaderRole} diff --git a/pkg/api/api.go b/pkg/api/api.go index d9689f92eb4..ec6c3c3cbe3 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -107,7 +107,6 @@ func (hs *HTTPServer) registerRoutes() { r.Get("/org/teams/new", authorize(ac.EvalPermission(ac.ActionTeamsCreate)), hs.Index) r.Get("/org/serviceaccounts", authorize(ac.EvalPermission(serviceaccounts.ActionRead)), hs.Index) r.Get("/org/serviceaccounts/:serviceAccountId", authorize(ac.EvalPermission(serviceaccounts.ActionRead)), hs.Index) - r.Get("/org/apikeys/", authorize(ac.EvalPermission(ac.ActionAPIKeyRead)), hs.Index) r.Get("/dashboard/import/", reqSignedIn, hs.Index) r.Get("/configuration", reqGrafanaAdmin, hs.Index) r.Get("/admin", reqOrgAdmin, hs.Index) diff --git a/pkg/api/dtos/apikey.go b/pkg/api/dtos/apikey.go index 28dd1389810..ec77c18a550 100644 --- a/pkg/api/dtos/apikey.go +++ b/pkg/api/dtos/apikey.go @@ -1,6 +1,5 @@ package dtos -// swagger:model type NewApiKeyResult struct { // example: 1 ID int64 `json:"id"` diff --git a/pkg/components/apikeygen/apikeygen.go b/pkg/components/apikeygen/apikeygen.go index f907573bf95..ba10258519d 100644 --- a/pkg/components/apikeygen/apikeygen.go +++ b/pkg/components/apikeygen/apikeygen.go @@ -3,13 +3,11 @@ package apikeygen import ( "encoding/base64" "encoding/json" - "errors" + "github.com/grafana/grafana/pkg/components/satokengen" "github.com/grafana/grafana/pkg/util" ) -var ErrInvalidApiKey = errors.New("invalid API key") - type KeyGenResult struct { HashedKey string ClientSecret string @@ -50,13 +48,13 @@ func New(orgId int64, name string) (KeyGenResult, error) { func Decode(keyString string) (*ApiKeyJson, error) { jsonString, err := base64.StdEncoding.DecodeString(keyString) if err != nil { - return nil, ErrInvalidApiKey + return nil, satokengen.ErrInvalidApiKey } var keyObj ApiKeyJson err = json.Unmarshal(jsonString, &keyObj) if err != nil { - return nil, ErrInvalidApiKey + return nil, satokengen.ErrInvalidApiKey } return &keyObj, nil diff --git a/pkg/components/satokengen/errors.go b/pkg/components/satokengen/errors.go deleted file mode 100644 index bd6eef8e411..00000000000 --- a/pkg/components/satokengen/errors.go +++ /dev/null @@ -1,14 +0,0 @@ -package satokengen - -import "github.com/grafana/grafana/pkg/components/apikeygen" - -type ErrInvalidApiKey struct { -} - -func (e *ErrInvalidApiKey) Error() string { - return "invalid API key" -} - -func (e *ErrInvalidApiKey) Unwrap() error { - return apikeygen.ErrInvalidApiKey -} diff --git a/pkg/components/satokengen/tokengen.go b/pkg/components/satokengen/tokengen.go index 3e96e9f080e..b2e0f0a0ffa 100644 --- a/pkg/components/satokengen/tokengen.go +++ b/pkg/components/satokengen/tokengen.go @@ -2,6 +2,7 @@ package satokengen import ( "encoding/hex" + "errors" "hash/crc32" "strings" @@ -10,6 +11,8 @@ import ( const GrafanaPrefix = "gl" +var ErrInvalidApiKey = errors.New("invalid API key") + type KeyGenResult struct { HashedKey string ClientSecret string @@ -72,12 +75,12 @@ func New(serviceID string) (KeyGenResult, error) { func Decode(keyString string) (*PrefixedKey, error) { if !strings.HasPrefix(keyString, GrafanaPrefix) { - return nil, &ErrInvalidApiKey{} + return nil, ErrInvalidApiKey } parts := strings.Split(keyString, "_") if len(parts) != 3 { - return nil, &ErrInvalidApiKey{} + return nil, ErrInvalidApiKey } key := &PrefixedKey{ @@ -86,7 +89,7 @@ func Decode(keyString string) (*PrefixedKey, error) { Checksum: parts[2], } if key.CalculateChecksum() != key.Checksum { - return nil, &ErrInvalidApiKey{} + return nil, ErrInvalidApiKey } return key, nil diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index 86adbdbfcb0..ecc0210edcd 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -81,6 +81,11 @@ func ProvideService( return nil, err } + // Migrating to remove deprecated permissions from the database + if err := migrator.MigrateRemoveDeprecatedPermissions(db, service.log); err != nil { + return nil, err + } + return service, nil } @@ -699,7 +704,7 @@ func PermissionMatchesSearchOptions(permission accesscontrol.Permission, searchO if searchOptions.Scope != "" { // Permissions including the scope should also match scopes := append(searchOptions.Wildcards(), searchOptions.Scope) - if !slices.Contains[[]string, string](scopes, permission.Scope) { + if !slices.Contains(scopes, permission.Scope) { return false } } diff --git a/pkg/services/accesscontrol/migrator/migrator.go b/pkg/services/accesscontrol/migrator/migrator.go index 8ecc20ad553..1463ffd9eaa 100644 --- a/pkg/services/accesscontrol/migrator/migrator.go +++ b/pkg/services/accesscontrol/migrator/migrator.go @@ -4,8 +4,11 @@ import ( "context" "time" + "go.opentelemetry.io/otel/attribute" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/tracing" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/session" @@ -120,6 +123,90 @@ func batch(count, batchSize int, eachFn func(start, end int) error) error { return nil } +// MigrateRemoveDeprecatedPermissions removes deprecated permissions from the database +func MigrateRemoveDeprecatedPermissions(db db.DB, log log.Logger) error { + ctx := context.Background() + ctx, span := tracing.Start(ctx, "migrator.removeDeprecatedPermissions", + attribute.String("migration.type", "removeDeprecatedPermissions")) + defer span.End() + + t := time.Now() + + // Define the deprecated permissions to remove + deprecatedPermissions := []string{ + "apikeys:", // remove this line in 2026/03, no apikeys:read/write/create should exist by then and downgrade/upgrade scenarios are less likely + } + if len(deprecatedPermissions) == 0 { + span.SetAttributes(attribute.Bool("migration.skipped", true)) + log.Debug("No deprecated permissions to remove", "migration", "removeDeprecatedPermissions") + return nil + } + + span.SetAttributes(attribute.Int("deprecated.patterns.count", len(deprecatedPermissions))) + log.Info("Starting migration to remove deprecated permissions", "migration", "removeDeprecatedPermissions") + + // Find and remove permissions matching the deprecated patterns + var totalRemoved int + for _, permPattern := range deprecatedPermissions { + patternCtx, patternSpan := tracing.Start(ctx, "migrator.removeDeprecatedPermissions.pattern", + attribute.String("pattern", permPattern)) + patternSpan.SetAttributes(attribute.String("migration.type", "removeDeprecatedPermissions")) + + var permissions []ac.Permission + if errFind := db.WithTransactionalDbSession(patternCtx, func(sess *sqlstore.DBSession) error { + return sess.SQL("SELECT id FROM permission WHERE action LIKE ?", permPattern+"%").Find(&permissions) + }); errFind != nil { + log.Error("Could not search for deprecated permissions to remove", "migration", "removeDeprecatedPermissions", "pattern", permPattern, "error", errFind) + patternSpan.RecordError(errFind) + patternSpan.End() + return errFind + } + + patternSpan.SetAttributes(attribute.Int("permissions.found", len(permissions))) + + if len(permissions) == 0 { + log.Debug("No permissions found for pattern", "migration", "removeDeprecatedPermissions", "pattern", permPattern) + patternSpan.End() + continue + } + + // Remove permissions by the exact IDs we found + if errDel := db.GetSqlxSession().WithTransaction(patternCtx, func(tx *session.SessionTx) error { + delQuery := "DELETE FROM permission WHERE id IN (" + delArgs := make([]any, 0, len(permissions)) + for i := range permissions { + delQuery += "?," + delArgs = append(delArgs, permissions[i].ID) + } + // close the IN clause + delQuery = delQuery[:len(delQuery)-1] + ")" + + _, err := tx.Exec(patternCtx, delQuery, delArgs...) + return err + }); errDel != nil { + log.Error("Error deleting deprecated permissions", "migration", "removeDeprecatedPermissions", "pattern", permPattern, "error", errDel) + patternSpan.RecordError(errDel) + patternSpan.End() + return errDel + } + + // We previously fetched matching permissions; count them as removed + totalRemoved += len(permissions) + patternSpan.SetAttributes(attribute.Int("permissions.removed", len(permissions))) + log.Info("Removed deprecated permissions for pattern", "migration", "removeDeprecatedPermissions", "pattern", permPattern, "count", len(permissions)) + + patternSpan.End() + } + + span.SetAttributes( + attribute.Int("permissions.total.removed", totalRemoved), + attribute.Int("migration.duration.ms", int(time.Since(t).Milliseconds())), + ) + + log.Info("Completed migration to remove deprecated permissions", "migration", "removeDeprecatedPermissions", "totalRemoved", totalRemoved, "duration", time.Since(t)) + return nil +} + func trimToMaxLen(s string, maxLen int) string { if len(s) > maxLen { return s[:maxLen] diff --git a/pkg/services/accesscontrol/migrator/migrator_test.go b/pkg/services/accesscontrol/migrator/migrator_test.go index a9714d9677c..5595a251c81 100644 --- a/pkg/services/accesscontrol/migrator/migrator_test.go +++ b/pkg/services/accesscontrol/migrator/migrator_test.go @@ -88,3 +88,216 @@ func TestIntegrationMigrateScopeSplitTruncation(t *testing.T) { } } } + +// batchInsertTestPermissions inserts test permissions for migration testing +func batchInsertTestPermissions(cnt int, sqlStore db.DB, actionPrefix string) error { + now := time.Now() + suffixes := []string{"read", "write", "delete"} + + return batch(cnt, batchSize, func(start, end int) error { + n := end - start + permissions := make([]ac.Permission, 0, n) + for i := start; i < end; i++ { + suffix := suffixes[i%len(suffixes)] + permissions = append(permissions, ac.Permission{ + RoleID: 1, + Action: fmt.Sprintf("%s:%s", actionPrefix, suffix), + Scope: fmt.Sprintf("%s:uid:%v", actionPrefix, i+1), + Created: now, + Updated: now, + }) + } + return sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + _, err := sess.Insert(permissions) + return err + }) + }) +} + +// TestIntegrationMigrateRemoveDeprecatedPermissions tests the deprecated permissions removal migration +func TestIntegrationMigrateRemoveDeprecatedPermissions(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + sqlStore := db.InitTestDB(t) + logger := log.New("accesscontrol.migrator.test") + + // Test 1: Basic functionality - remove deprecated permissions + t.Run("removes deprecated permissions", func(t *testing.T) { + // Insert deprecated permissions (apikeys: pattern) + require.NoError(t, batchInsertTestPermissions(5, sqlStore, "apikeys"), "could not insert deprecated permissions") + + // Insert non-deprecated permissions + require.NoError(t, batchInsertTestPermissions(3, sqlStore, "dashboards"), "could not insert non-deprecated permissions") + + // Count permissions before migration + var permissionsBefore []ac.Permission + err := sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsBefore) + }) + require.NoError(t, err, "could not count permissions before migration") + assert.Equal(t, 8, len(permissionsBefore), "expected 8 permissions before migration") + + // Run migration + require.NoError(t, MigrateRemoveDeprecatedPermissions(sqlStore, logger)) + + // Count permissions after migration + var permissionsAfter []ac.Permission + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsAfter) + }) + require.NoError(t, err, "could not count permissions after migration") + assert.Equal(t, 3, len(permissionsAfter), "expected 3 permissions after migration") + + // Verify only non-deprecated permissions remain + for _, perm := range permissionsAfter { + assert.NotContains(t, perm.Action, "apikeys:", "deprecated permission should have been removed") + } + }) +} + +// TestIntegrationMigrateRemoveDeprecatedPermissionsEmptyDB tests migration with empty database +func TestIntegrationMigrateRemoveDeprecatedPermissionsEmptyDB(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + sqlStore := db.InitTestDB(t) + logger := log.New("accesscontrol.migrator.test") + + // Run migration on empty database + require.NoError(t, MigrateRemoveDeprecatedPermissions(sqlStore, logger)) + + // Verify no permissions exist + var permissions []ac.Permission + err := sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissions) + }) + require.NoError(t, err, "could not query permissions") + assert.Empty(t, permissions, "expected no permissions in empty database") +} + +// TestIntegrationMigrateRemoveDeprecatedPermissionsBatchProcessing tests batch processing with large dataset +func TestIntegrationMigrateRemoveDeprecatedPermissionsBatchProcessing(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + sqlStore := db.InitTestDB(t) + logger := log.New("accesscontrol.migrator.test") + + // Set small batch size for testing + originalBatchSize := batchSize + batchSize = 3 + defer func() { batchSize = originalBatchSize }() + + // Insert more deprecated permissions than batch size + require.NoError(t, batchInsertTestPermissions(10, sqlStore, "apikeys"), "could not insert deprecated permissions") + + // Insert some non-deprecated permissions + require.NoError(t, batchInsertTestPermissions(2, sqlStore, "folders"), "could not insert non-deprecated permissions") + + // Count permissions before migration + var permissionsBefore []ac.Permission + err := sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsBefore) + }) + require.NoError(t, err, "could not count permissions before migration") + assert.Equal(t, 12, len(permissionsBefore), "expected 12 permissions before migration") + + // Run migration + require.NoError(t, MigrateRemoveDeprecatedPermissions(sqlStore, logger)) + + // Count permissions after migration + var permissionsAfter []ac.Permission + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsAfter) + }) + require.NoError(t, err, "could not count permissions after migration") + assert.Equal(t, 2, len(permissionsAfter), "expected 2 permissions after migration") + + // Verify only non-deprecated permissions remain + for _, perm := range permissionsAfter { + assert.NotContains(t, perm.Action, "apikeys:", "deprecated permission should have been removed") + assert.Contains(t, perm.Action, "folders:", "non-deprecated permission should remain") + } +} + +// TestIntegrationMigrateRemoveDeprecatedPermissionsNoDeprecated tests when no deprecated permissions exist +func TestIntegrationMigrateRemoveDeprecatedPermissionsNoDeprecated(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + sqlStore := db.InitTestDB(t) + logger := log.New("accesscontrol.migrator.test") + + // Insert only non-deprecated permissions + require.NoError(t, batchInsertTestPermissions(5, sqlStore, "users"), "could not insert non-deprecated permissions") + + // Count permissions before migration + var permissionsBefore []ac.Permission + err := sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsBefore) + }) + require.NoError(t, err, "could not count permissions before migration") + assert.Equal(t, 5, len(permissionsBefore), "expected 5 permissions before migration") + + // Run migration + require.NoError(t, MigrateRemoveDeprecatedPermissions(sqlStore, logger)) + + // Count permissions after migration + var permissionsAfter []ac.Permission + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsAfter) + }) + require.NoError(t, err, "could not count permissions after migration") + assert.Equal(t, 5, len(permissionsAfter), "expected 5 permissions after migration (none should be removed)") + + // Verify all permissions remain unchanged + for _, perm := range permissionsAfter { + assert.NotContains(t, perm.Action, "apikeys:", "no deprecated permissions should exist") + assert.Contains(t, perm.Action, "users:", "non-deprecated permissions should remain") + } +} + +// TestIntegrationMigrateRemoveDeprecatedPermissionsMixedPatterns tests mixed deprecated and non-deprecated patterns +func TestIntegrationMigrateRemoveDeprecatedPermissionsMixedPatterns(t *testing.T) { + testutil.SkipIntegrationTestInShortMode(t) + + sqlStore := db.InitTestDB(t) + logger := log.New("accesscontrol.migrator.test") + + // Insert deprecated permissions + require.NoError(t, batchInsertTestPermissions(3, sqlStore, "apikeys"), "could not insert deprecated permissions") + + // Insert various non-deprecated permissions + require.NoError(t, batchInsertTestPermissions(2, sqlStore, "dashboards"), "could not insert dashboard permissions") + require.NoError(t, batchInsertTestPermissions(2, sqlStore, "folders"), "could not insert folder permissions") + require.NoError(t, batchInsertTestPermissions(2, sqlStore, "datasources"), "could not insert datasource permissions") + + // Count permissions before migration + var permissionsBefore []ac.Permission + err := sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsBefore) + }) + require.NoError(t, err, "could not count permissions before migration") + assert.Equal(t, 9, len(permissionsBefore), "expected 9 permissions before migration") + + // Run migration + require.NoError(t, MigrateRemoveDeprecatedPermissions(sqlStore, logger)) + + // Count permissions after migration + var permissionsAfter []ac.Permission + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + return sess.Find(&permissionsAfter) + }) + require.NoError(t, err, "could not count permissions after migration") + assert.Equal(t, 6, len(permissionsAfter), "expected 6 permissions after migration") + + // Verify deprecated permissions are removed and others remain + deprecatedCount := 0 + validCount := 0 + for _, perm := range permissionsAfter { + if strings.HasPrefix(perm.Action, "apikeys:") { + deprecatedCount++ + } else { + validCount++ + } + } + assert.Equal(t, 0, deprecatedCount, "no deprecated permissions should remain") + assert.Equal(t, 6, validCount, "expected 6 valid permissions to remain") +} diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index dc12171eaa5..8cfb9d5121b 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -328,12 +328,6 @@ const ( K6FolderUID = "k6-app" RoleGrafanaAdmin = "Grafana Admin" - // Permission actions - - ActionAPIKeyRead = "apikeys:read" - ActionAPIKeyCreate = "apikeys:create" - ActionAPIKeyDelete = "apikeys:delete" - // Users actions ActionUsersRead = "users:read" ActionUsersWrite = "users:write" @@ -391,9 +385,6 @@ const ( // Global Scopes ScopeGlobalUsersAll = "global.users:*" - // APIKeys scope - ScopeAPIKeysAll = "apikeys:*" - // Users scope ScopeUsersAll = "users:*" ScopeUsersPrefix = "users:id:" @@ -587,9 +578,6 @@ var OrgsCreateAccessEvaluator = EvalAll( EvalPermission(ActionOrgsCreate), ) -// ApiKeyAccessEvaluator is used to protect the "Configuration > API keys" page access -var ApiKeyAccessEvaluator = EvalPermission(ActionAPIKeyRead) - type QueryWithOrg struct { OrgId *int64 `json:"orgId"` Global bool `json:"global"` diff --git a/pkg/services/accesscontrol/permreg/permreg.go b/pkg/services/accesscontrol/permreg/permreg.go index 1d46d749b38..5d025a1258b 100644 --- a/pkg/services/accesscontrol/permreg/permreg.go +++ b/pkg/services/accesscontrol/permreg/permreg.go @@ -82,7 +82,6 @@ func newPermissionRegistry() *permissionRegistry { "dashboards": "dashboards:uid:", "folders": "folders:uid:", "annotations": "annotations:type:", - "apikeys": "apikeys:id:", "orgs": "orgs:id:", "plugins": "plugins:id:", "provisioners": "provisioners:", diff --git a/pkg/services/apikey/apikeyimpl/store_test.go b/pkg/services/apikey/apikeyimpl/store_test.go index ac2cfb06088..dda62c750bd 100644 --- a/pkg/services/apikey/apikeyimpl/store_test.go +++ b/pkg/services/apikey/apikeyimpl/store_test.go @@ -56,6 +56,9 @@ func seedApiKeys(t *testing.T, store store, num int) { } func testIntegrationApiKeyDataAccess(t *testing.T, fn getStore) { + if testing.Short() { + t.Skip("skipping integration test") + } t.Helper() mockTimeNow() @@ -188,24 +191,18 @@ func testIntegrationApiKeyDataAccess(t *testing.T, fn getStore) { t.Run("Testing Get API keys", func(t *testing.T) { tests := []getApiKeysTestCase{ { - desc: "expect all keys for wildcard scope", - user: &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{ - 1: {"apikeys:read": {"apikeys:*"}}, - }}, + desc: "expect all keys for wildcard scope", + user: &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{}}, expectedAllNumKeys: 10, }, { - desc: "expect only api keys that user have scopes for", - user: &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{ - 1: {"apikeys:read": {"apikeys:id:1", "apikeys:id:3"}}, - }}, + desc: "expect only api keys that user have scopes for", + user: &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{}}, expectedAllNumKeys: 10, }, { - desc: "expect no keys when user have no scopes", - user: &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{ - 1: {"apikeys:read": {}}, - }}, + desc: "expect no keys when user have no scopes", + user: &user.SignedInUser{OrgID: 1, Permissions: map[int64]map[string][]string{}}, expectedAllNumKeys: 10, }, } diff --git a/pkg/services/apikey/model.go b/pkg/services/apikey/model.go index 7e1bcf71446..7a2118590d4 100644 --- a/pkg/services/apikey/model.go +++ b/pkg/services/apikey/model.go @@ -31,7 +31,6 @@ type APIKey struct { func (k APIKey) TableName() string { return "api_key" } -// swagger:model AddAPIKeyCommand type AddCommand struct { Name string `json:"name" binding:"Required"` Role org.RoleType `json:"role" binding:"Required"` diff --git a/pkg/services/authn/clients/api_key.go b/pkg/services/authn/clients/api_key.go index 2023cb4e418..d2357f10f1f 100644 --- a/pkg/services/authn/clients/api_key.go +++ b/pkg/services/authn/clients/api_key.go @@ -60,7 +60,7 @@ func (s *APIKey) Authenticate(ctx context.Context, r *authn.Request) (*authn.Ide defer span.End() key, err := s.getAPIKey(ctx, getTokenFromRequest(r)) if err != nil { - if errors.Is(err, apikeygen.ErrInvalidApiKey) { + if errors.Is(err, satokengen.ErrInvalidApiKey) { return nil, errAPIKeyInvalid.Errorf("API key is invalid") } return nil, err @@ -141,7 +141,7 @@ func (s *APIKey) getFromTokenLegacy(ctx context.Context, token string) (*apikey. return nil, err } if !isValid { - return nil, apikeygen.ErrInvalidApiKey + return nil, satokengen.ErrInvalidApiKey } return key, nil diff --git a/pkg/services/authn/clients/api_key_test.go b/pkg/services/authn/clients/api_key_test.go index 62fc83bc7f0..5a47b1879db 100644 --- a/pkg/services/authn/clients/api_key_test.go +++ b/pkg/services/authn/clients/api_key_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/assert" claims "github.com/grafana/authlib/types" - "github.com/grafana/grafana/pkg/components/apikeygen" "github.com/grafana/grafana/pkg/components/satokengen" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/apikey" @@ -22,7 +21,7 @@ import ( var ( revoked = true - secret, hash = genApiKey(false) + secret, hash = genApiKey() ) func TestAPIKey_Authenticate(t *testing.T) { @@ -188,11 +187,7 @@ func boolPtr(b bool) *bool { return &b } -func genApiKey(legacy bool) (string, string) { - if legacy { - res, _ := apikeygen.New(1, "test") - return res.ClientSecret, res.HashedKey - } +func genApiKey() (string, string) { res, _ := satokengen.New("test") return res.ClientSecret, res.HashedKey } diff --git a/pkg/services/grpcserver/interceptors/auth_test.go b/pkg/services/grpcserver/interceptors/auth_test.go index 5f989ddd79a..389459311bf 100644 --- a/pkg/services/grpcserver/interceptors/auth_test.go +++ b/pkg/services/grpcserver/interceptors/auth_test.go @@ -102,8 +102,8 @@ func TestAuthenticator_Authenticate(t *testing.T) { }, nil) permissions := []accesscontrol.Permission{ { - Action: accesscontrol.ActionAPIKeyRead, - Scope: accesscontrol.ScopeAPIKeysAll, + Action: accesscontrol.ActionUsersWrite, + Scope: accesscontrol.ScopeUsersAll, }, } ac := accesscontrolmock.New().WithPermissions(permissions) @@ -114,7 +114,7 @@ func TestAuthenticator_Authenticate(t *testing.T) { require.NoError(t, err) signedInUser := grpccontext.FromContext(ctx).SignedInUser require.Equal(t, serviceAccountId, signedInUser.UserID) - require.Equal(t, []string{accesscontrol.ScopeAPIKeysAll}, signedInUser.Permissions[1][accesscontrol.ActionAPIKeyRead]) + require.Equal(t, []string{accesscontrol.ScopeUsersAll}, signedInUser.Permissions[1][accesscontrol.ActionUsersWrite]) }) } diff --git a/pkg/services/serviceaccounts/database/token_store_test.go b/pkg/services/serviceaccounts/database/token_store_test.go index bef863abd11..45767c349b9 100644 --- a/pkg/services/serviceaccounts/database/token_store_test.go +++ b/pkg/services/serviceaccounts/database/token_store_test.go @@ -6,7 +6,7 @@ import ( "github.com/stretchr/testify/require" - "github.com/grafana/grafana/pkg/components/apikeygen" + "github.com/grafana/grafana/pkg/components/satokengen" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" "github.com/grafana/grafana/pkg/util/testutil" @@ -29,7 +29,7 @@ func TestIntegration_Store_AddServiceAccountToken(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { keyName := t.Name() - key, err := apikeygen.New(user.OrgID, keyName) + key, err := satokengen.New(keyName) require.NoError(t, err) cmd := serviceaccounts.AddServiceAccountTokenCommand{ @@ -84,7 +84,7 @@ func TestIntegration_Store_AddServiceAccountToken_WrongServiceAccount(t *testing sa := tests.SetupUserServiceAccount(t, db, store.cfg, saToCreate) keyName := t.Name() - key, err := apikeygen.New(sa.OrgID, keyName) + key, err := satokengen.New(keyName) require.NoError(t, err) cmd := serviceaccounts.AddServiceAccountTokenCommand{ @@ -106,7 +106,7 @@ func TestIntegration_Store_RevokeServiceAccountToken(t *testing.T) { sa := tests.SetupUserServiceAccount(t, db, store.cfg, userToCreate) keyName := t.Name() - key, err := apikeygen.New(sa.OrgID, keyName) + key, err := satokengen.New(keyName) require.NoError(t, err) cmd := serviceaccounts.AddServiceAccountTokenCommand{ @@ -148,7 +148,7 @@ func TestIntegration_Store_DeleteServiceAccountToken(t *testing.T) { sa := tests.SetupUserServiceAccount(t, db, store.cfg, userToCreate) keyName := t.Name() - key, err := apikeygen.New(sa.OrgID, keyName) + key, err := satokengen.New(keyName) require.NoError(t, err) cmd := serviceaccounts.AddServiceAccountTokenCommand{ diff --git a/pkg/services/serviceaccounts/extsvcaccounts/service.go b/pkg/services/serviceaccounts/extsvcaccounts/service.go index d51df9964ca..c27b8aadb85 100644 --- a/pkg/services/serviceaccounts/extsvcaccounts/service.go +++ b/pkg/services/serviceaccounts/extsvcaccounts/service.go @@ -350,7 +350,7 @@ func (esa *ExtSvcAccountsService) getExtSvcAccountToken(ctx context.Context, org // Get credentials from store credentials, err := esa.GetExtSvcCredentials(ctx, orgID, extSvcSlug) if err != nil && !errors.Is(err, ErrCredentialsNotFound) { - if !errors.Is(err, &satokengen.ErrInvalidApiKey{}) { + if !errors.Is(err, satokengen.ErrInvalidApiKey) { return "", err } ctxLogger.Warn("Invalid token found in store, recovering...", "service", extSvcSlug, "orgID", orgID) diff --git a/public/api-enterprise-spec.json b/public/api-enterprise-spec.json index 9fa1d213c0a..ea2046dedd7 100644 --- a/public/api-enterprise-spec.json +++ b/public/api-enterprise-spec.json @@ -2475,27 +2475,6 @@ } } }, - "AddAPIKeyCommand": { - "type": "object", - "properties": { - "name": { - "type": "string" - }, - "role": { - "type": "string", - "enum": [ - "None", - "Viewer", - "Editor", - "Admin" - ] - }, - "secondsToLive": { - "type": "integer", - "format": "int64" - } - } - }, "AddDataSourceCommand": { "description": "Also acts as api DTO", "type": "object", diff --git a/public/api-merged.json b/public/api-merged.json index 5b0d1291c82..830f4f4c309 100644 --- a/public/api-merged.json +++ b/public/api-merged.json @@ -12622,27 +12622,6 @@ } } }, - "AddAPIKeyCommand": { - "type": "object", - "properties": { - "name": { - "type": "string" - }, - "role": { - "type": "string", - "enum": [ - "None", - "Viewer", - "Editor", - "Admin" - ] - }, - "secondsToLive": { - "type": "integer", - "format": "int64" - } - } - }, "AddDataSourceCommand": { "description": "Also acts as api DTO", "type": "object", diff --git a/public/app/core/reducers/navModel.test.ts b/public/app/core/reducers/navModel.test.ts index 0b6b00ebede..3655efdb400 100644 --- a/public/app/core/reducers/navModel.test.ts +++ b/public/app/core/reducers/navModel.test.ts @@ -49,31 +49,28 @@ describe('navModelReducer', () => { const teams = { id: 'teams', text: 'Teams' }; const plugins = { id: 'plugins', text: 'Plugins' }; const orgsettings = { id: 'org-settings', text: 'Preferences' }; - const apikeys = { id: 'apikeys', text: 'API Keys' }; const initialState = { - cfg: { ...originalCfg, children: [datasources, users, teams, plugins, orgsettings, apikeys] }, + cfg: { ...originalCfg, children: [datasources, users, teams, plugins, orgsettings] }, datasources: { ...datasources, parentItem: originalCfg }, correlations: { ...correlations, parentItem: originalCfg }, users: { ...users, parentItem: originalCfg }, teams: { ...teams, parentItem: originalCfg }, plugins: { ...plugins, parentItem: originalCfg }, 'org-settings': { ...orgsettings, parentItem: originalCfg }, - apikeys: { ...apikeys, parentItem: originalCfg }, }; const newOrgName = 'Org 2'; const subTitle = `Organization: ${newOrgName}`; const newCfg = { ...originalCfg, subTitle }; const expectedState = { - cfg: { ...newCfg, children: [datasources, users, teams, plugins, orgsettings, apikeys] }, + cfg: { ...newCfg, children: [datasources, users, teams, plugins, orgsettings] }, datasources: { ...datasources, parentItem: newCfg }, correlations: { ...correlations, parentItem: newCfg }, users: { ...users, parentItem: newCfg }, teams: { ...teams, parentItem: newCfg }, plugins: { ...plugins, parentItem: newCfg }, 'org-settings': { ...orgsettings, parentItem: newCfg }, - apikeys: { ...apikeys, parentItem: newCfg }, }; reducerTester() diff --git a/public/app/core/reducers/navModel.ts b/public/app/core/reducers/navModel.ts index 6717c66b350..ed6b798bb0a 100644 --- a/public/app/core/reducers/navModel.ts +++ b/public/app/core/reducers/navModel.ts @@ -122,7 +122,6 @@ export const navIndexReducer = (state: NavIndex = initialState, action: AnyActio teams: getItemWithNewSubTitle(state.teams, subTitle), plugins: getItemWithNewSubTitle(state.plugins, subTitle), 'org-settings': getItemWithNewSubTitle(state['org-settings'], subTitle), - apikeys: getItemWithNewSubTitle(state.apikeys, subTitle), }; } else if (removeNavIndex.match(action)) { delete state[action.payload]; diff --git a/public/app/types/accessControl.ts b/public/app/types/accessControl.ts index 7ef86382edc..abbf8969b05 100644 --- a/public/app/types/accessControl.ts +++ b/public/app/types/accessControl.ts @@ -155,10 +155,6 @@ export enum AccessControlAction { AlertingTemplatesWrite = 'alert.notifications.templates:write', AlertingTemplatesDelete = 'alert.notifications.templates:delete', - ActionAPIKeysRead = 'apikeys:read', - ActionAPIKeysCreate = 'apikeys:create', - ActionAPIKeysDelete = 'apikeys:delete', - PluginsInstall = 'plugins:install', PluginsWrite = 'plugins:write', diff --git a/public/openapi3.json b/public/openapi3.json index d786cb53437..9140c87244d 100644 --- a/public/openapi3.json +++ b/public/openapi3.json @@ -2150,27 +2150,6 @@ }, "type": "object" }, - "AddAPIKeyCommand": { - "properties": { - "name": { - "type": "string" - }, - "role": { - "enum": [ - "None", - "Viewer", - "Editor", - "Admin" - ], - "type": "string" - }, - "secondsToLive": { - "format": "int64", - "type": "integer" - } - }, - "type": "object" - }, "AddDataSourceCommand": { "description": "Also acts as api DTO", "properties": {