Authz: Remove legacy API Key permissions (#110860)

* remove API key roles

* remove API key gen

* remove frontend and doc mentions

* restore legacy keygen

* restore codeowners

* prettier

* update swagger

* remove permissions including apikeys

* add migrator for removing deprecated permissions

* add tracing

* update openapi3

* simplify migrator for now

* accesscontrol/migrator: remove batching for deprecated permissions deletion
This commit is contained in:
Jo
2025-09-12 13:59:37 +02:00
committed by GitHub
parent 1f7afc6b6a
commit edcd113054
26 changed files with 346 additions and 189 deletions
+6 -1
View File
@@ -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
}
}
@@ -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]
@@ -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")
}
-12
View File
@@ -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"`
@@ -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:",
+9 -12
View File
@@ -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,
},
}
-1
View File
@@ -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"`
+2 -2
View File
@@ -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
+2 -7
View File
@@ -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
}
@@ -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])
})
}
@@ -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{
@@ -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)