From b5d4dd7c638f8090f422e92a88e7641aad9850a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E1=B4=9C=C9=B4=E1=B4=8B=C9=B4=E1=B4=A1=E1=B4=8F=C9=B4?= Date: Wed, 14 Oct 2020 22:42:32 +0800 Subject: [PATCH] authz: remove read/write paths of permissions in binary format (#14660) --- CHANGELOG.md | 1 + enterprise/cmd/repo-updater/main.go | 8 +- enterprise/internal/db/integration_test.go | 2 - enterprise/internal/db/perms_store.go | 262 ++-------------- enterprise/internal/db/perms_store_test.go | 291 +----------------- internal/db/schema.md | 8 +- ...dd_permissions_object_ids_default.down.sql | 8 + ..._add_permissions_object_ids_default.up.sql | 8 + migrations/frontend/bindata.go | 46 +++ 9 files changed, 115 insertions(+), 519 deletions(-) create mode 100644 migrations/frontend/1528395733_add_permissions_object_ids_default.down.sql create mode 100644 migrations/frontend/1528395733_add_permissions_object_ids_default.up.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 844df70f967..b4182f8c470 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ All notable changes to Sourcegraph are documented in this file. - Kubernetes admins mounting [configuration files](https://docs.sourcegraph.com/admin/config/advanced_config_file#kubernetes-configmap) are encouraged to change how the ConfigMap is mounted. See the new documentation. Previously our documentation suggested using subPath. However, this lead to Kubernetes not automatically updating the files on configuration change. [#14297](https://github.com/sourcegraph/sourcegraph/pull/14297) - The precise code intel bundle manager will now expire any converted LSIF data that is older than `PRECISE_CODE_INTEL_MAX_DATA_AGE` (30 days by default) that is also not visible from the tip of the default branch. - `SRC_LOG_LEVEL=warn` is now the default in Docker Compose and Kubernetes deployments, reducing the amount of uninformative log spam. [#14458](https://github.com/sourcegraph/sourcegraph/pull/14458) +- Permissions data that were stored in deprecated binary format are abandoned. Downgrade from 3.21 to 3.20 is OK, but to 3.19 or prior versions might experience missing/incomplete state of permissions for a short period of time. [#13740](https://github.com/sourcegraph/sourcegraph/issues/13740) ### Fixed diff --git a/enterprise/cmd/repo-updater/main.go b/enterprise/cmd/repo-updater/main.go index c6a82508f5a..961d219fdca 100644 --- a/enterprise/cmd/repo-updater/main.go +++ b/enterprise/cmd/repo-updater/main.go @@ -78,13 +78,7 @@ func enterpriseInit( dbconn.Global = db permsStore := edb.NewPermsStore(db, clock) permsSyncer := authz.NewPermsSyncer(repoStore, permsStore, clock, ratelimit.DefaultRegistry) - go func() { - if err := permsStore.MigrateBinaryToIntarray(ctx, 1000); err != nil { - log15.Error("MigrateBinaryToIntarray", "error", err) - } - - startBackgroundPermsSync(ctx, permsSyncer) - }() + go startBackgroundPermsSync(ctx, permsSyncer) debugDumpers = append(debugDumpers, permsSyncer) if server != nil { server.PermsSyncer = permsSyncer diff --git a/enterprise/internal/db/integration_test.go b/enterprise/internal/db/integration_test.go index 0acf08d4b1b..c524adc2b0a 100644 --- a/enterprise/internal/db/integration_test.go +++ b/enterprise/internal/db/integration_test.go @@ -34,8 +34,6 @@ func TestIntegration_PermsStore(t *testing.T) { {"DeleteAllUserPermissions", testPermsStore_DeleteAllUserPermissions(db)}, {"DeleteAllUserPendingPermissions", testPermsStore_DeleteAllUserPendingPermissions(db)}, {"DatabaseDeadlocks", testPermsStore_DatabaseDeadlocks(db)}, - {"FallbackToOldFormat", testPermsStore_FallbackToOldFormat(db)}, - {"MigrateBinaryToIntarray", testPermsStore_MigrateBinaryToIntarray(db)}, {"ListExternalAccounts", testPermsStore_ListExternalAccounts(db)}, {"GetUserIDsByExternalAccounts", testPermsStore_GetUserIDsByExternalAccounts(db)}, diff --git a/enterprise/internal/db/perms_store.go b/enterprise/internal/db/perms_store.go index 6761d550c51..75e07ff3425 100644 --- a/enterprise/internal/db/perms_store.go +++ b/enterprise/internal/db/perms_store.go @@ -65,7 +65,7 @@ func (s *PermsStore) LoadUserPermissions(ctx context.Context, p *authz.UserPermi func loadUserPermissionsQuery(p *authz.UserPermissions, lock string) *sqlf.Query { const format = ` -- source: enterprise/internal/db/perms_store.go:loadUserPermissionsQuery -SELECT user_id, object_ids, object_ids_ints, updated_at, synced_at +SELECT user_id, object_ids_ints, updated_at, synced_at FROM user_permissions WHERE user_id = %s AND permission = %s @@ -103,7 +103,7 @@ func (s *PermsStore) LoadRepoPermissions(ctx context.Context, p *authz.RepoPermi func loadRepoPermissionsQuery(p *authz.RepoPermissions, lock string) *sqlf.Query { const format = ` -- source: enterprise/internal/db/perms_store.go:loadRepoPermissionsQuery -SELECT repo_id, user_ids, user_ids_ints, updated_at, synced_at +SELECT repo_id, user_ids_ints, updated_at, synced_at FROM repo_permissions WHERE repo_id = %s AND permission = %s @@ -238,24 +238,18 @@ func upsertUserPermissionsQuery(p *authz.UserPermissions) (*sqlf.Query, error) { const format = ` -- source: enterprise/internal/db/perms_store.go:upsertUserPermissionsQuery INSERT INTO user_permissions - (user_id, permission, object_type, object_ids, object_ids_ints, updated_at, synced_at) + (user_id, permission, object_type, object_ids_ints, updated_at, synced_at) VALUES - (%s, %s, %s, %s, %s, %s, %s) + (%s, %s, %s, %s, %s, %s) ON CONFLICT ON CONSTRAINT user_permissions_perm_object_unique DO UPDATE SET - object_ids = excluded.object_ids, object_ids_ints = excluded.object_ids_ints, updated_at = excluded.updated_at, synced_at = excluded.synced_at ` p.IDs.RunOptimize() - ids, err := p.IDs.ToBytes() - if err != nil { - return nil, err - } - if p.UpdatedAt.IsZero() { return nil, ErrPermsUpdatedAtNotSet } else if p.SyncedAt.IsZero() { @@ -267,7 +261,6 @@ DO UPDATE SET p.UserID, p.Perm.String(), p.Type, - ids, pq.Array(p.IDs.ToArray()), p.UpdatedAt.UTC(), p.SyncedAt.UTC(), @@ -403,7 +396,7 @@ func loadUserPermissionsBatchQuery( ) *sqlf.Query { const format = ` -- source: enterprise/internal/db/perms_store.go:loadUserPermissionsBatchQuery -SELECT user_id, object_ids, object_ids_ints +SELECT user_id, object_ids_ints FROM user_permissions WHERE user_id IN (%s) AND permission = %s @@ -426,13 +419,12 @@ func upsertUserPermissionsBatchQuery(ps ...*authz.UserPermissions) (*sqlf.Query, const format = ` -- source: enterprise/internal/db/perms_store.go:upsertUserPermissionsBatchQuery INSERT INTO user_permissions - (user_id, permission, object_type, object_ids, object_ids_ints, updated_at) + (user_id, permission, object_type, object_ids_ints, updated_at) VALUES %s ON CONFLICT ON CONSTRAINT user_permissions_perm_object_unique DO UPDATE SET - object_ids = excluded.object_ids, object_ids_ints = excluded.object_ids_ints, updated_at = excluded.updated_at ` @@ -440,20 +432,14 @@ DO UPDATE SET items := make([]*sqlf.Query, len(ps)) for i := range ps { ps[i].IDs.RunOptimize() - ids, err := ps[i].IDs.ToBytes() - if err != nil { - return nil, err - } - if ps[i].UpdatedAt.IsZero() { return nil, ErrPermsUpdatedAtNotSet } - items[i] = sqlf.Sprintf("(%s, %s, %s, %s, %s, %s)", + items[i] = sqlf.Sprintf("(%s, %s, %s, %s, %s)", ps[i].UserID, ps[i].Perm.String(), ps[i].Type, - ids, pq.Array(ps[i].IDs.ToArray()), ps[i].UpdatedAt.UTC(), ) @@ -472,24 +458,18 @@ func upsertRepoPermissionsQuery(p *authz.RepoPermissions) (*sqlf.Query, error) { const format = ` -- source: enterprise/internal/db/perms_store.go:upsertRepoPermissionsQuery INSERT INTO repo_permissions - (repo_id, permission, user_ids, user_ids_ints, updated_at, synced_at) + (repo_id, permission, user_ids_ints, updated_at, synced_at) VALUES - (%s, %s, %s, %s, %s, %s) + (%s, %s, %s, %s, %s) ON CONFLICT ON CONSTRAINT repo_permissions_perm_unique DO UPDATE SET - user_ids = excluded.user_ids, user_ids_ints = excluded.user_ids_ints, updated_at = excluded.updated_at, synced_at = excluded.synced_at ` p.UserIDs.RunOptimize() - ids, err := p.UserIDs.ToBytes() - if err != nil { - return nil, err - } - if p.UpdatedAt.IsZero() { return nil, ErrPermsUpdatedAtNotSet } else if p.SyncedAt.IsZero() { @@ -500,7 +480,6 @@ DO UPDATE SET format, p.RepoID, p.Perm.String(), - ids, pq.Array(p.UserIDs.ToArray()), p.UpdatedAt.UTC(), p.SyncedAt.UTC(), @@ -530,7 +509,7 @@ func (s *PermsStore) LoadUserPendingPermissions(ctx context.Context, p *authz.Us func loadUserPendingPermissionsQuery(p *authz.UserPendingPermissions, lock string) *sqlf.Query { const format = ` -- source: enterprise/internal/db/perms_store.go:loadUserPendingPermissionsQuery -SELECT id, object_ids, object_ids_ints, updated_at, NULL +SELECT id, object_ids_ints, updated_at, NULL FROM user_pending_permissions WHERE service_type = %s AND service_id = %s @@ -730,7 +709,7 @@ func insertUserPendingPermissionsBatchQuery( const format = ` -- source: enterprise/internal/db/perms_store.go:insertUserPendingPermissionsBatchQuery INSERT INTO user_pending_permissions - (service_type, service_id, bind_id, permission, object_type, object_ids, updated_at) + (service_type, service_id, bind_id, permission, object_type, updated_at) VALUES %s ON CONFLICT ON CONSTRAINT @@ -744,21 +723,14 @@ RETURNING id return nil, ErrPermsUpdatedAtNotSet } - // Create an empty roaring bitmap - ids, err := roaring.NewBitmap().ToBytes() - if err != nil { - return nil, err - } - items := make([]*sqlf.Query, len(accounts.AccountIDs)) for i := range accounts.AccountIDs { - items[i] = sqlf.Sprintf("(%s, %s, %s, %s, %s, %s, %s)", + items[i] = sqlf.Sprintf("(%s, %s, %s, %s, %s, %s)", accounts.ServiceType, accounts.ServiceID, accounts.AccountIDs[i], p.Perm.String(), authz.PermRepos, - ids, p.UpdatedAt.UTC(), ) } @@ -772,7 +744,7 @@ RETURNING id func loadRepoPendingPermissionsQuery(p *authz.RepoPermissions, lock string) *sqlf.Query { const format = ` -- source: enterprise/internal/db/perms_store.go:loadRepoPendingPermissionsQuery -SELECT repo_id, user_ids, user_ids_ints, updated_at, NULL +SELECT repo_id, user_ids_ints, updated_at, NULL FROM repo_pending_permissions WHERE repo_id = %s AND permission = %s @@ -787,7 +759,7 @@ AND permission = %s func loadUserPendingPermissionsByIDBatchQuery(ids []uint32, perm authz.Perms, typ authz.PermType, lock string) *sqlf.Query { const format = ` -- source: enterprise/internal/db/perms_store.go:loadUserPendingPermissionsByIDBatchQuery -SELECT id, object_ids, object_ids_ints +SELECT id, object_ids_ints FROM user_pending_permissions WHERE id IN (%s) AND permission = %s @@ -811,28 +783,21 @@ func updateUserPendingPermissionsBatchQuery(ps ...*authz.UserPendingPermissions) -- source: enterprise/internal/db/perms_store.go:updateUserPendingPermissionsBatchQuery UPDATE user_pending_permissions SET - object_ids = update.object_ids, object_ids_ints = update.object_ids_ints, updated_at = update.updated_at -FROM (VALUES %s) AS update (id, object_ids, object_ids_ints, updated_at) +FROM (VALUES %s) AS update (id, object_ids_ints, updated_at) WHERE user_pending_permissions.id = update.id ` items := make([]*sqlf.Query, len(ps)) for i := range ps { ps[i].IDs.RunOptimize() - ids, err := ps[i].IDs.ToBytes() - if err != nil { - return nil, err - } - if ps[i].UpdatedAt.IsZero() { return nil, ErrPermsUpdatedAtNotSet } - items[i] = sqlf.Sprintf("(%s::INT, %s::BYTEA, %s::INT[], %s::TIMESTAMP WITH TIME ZONE)", + items[i] = sqlf.Sprintf("(%s::INT, %s::INT[], %s::TIMESTAMP WITH TIME ZONE)", ps[i].ID, - ids, pq.Array(ps[i].IDs.ToArray()), ps[i].UpdatedAt.UTC(), ) @@ -848,13 +813,12 @@ func upsertRepoPendingPermissionsBatchQuery(ps ...*authz.RepoPermissions) (*sqlf const format = ` -- source: enterprise/internal/db/perms_store.go:upsertRepoPendingPermissionsBatchQuery INSERT INTO repo_pending_permissions - (repo_id, permission, user_ids, user_ids_ints, updated_at) + (repo_id, permission, user_ids_ints, updated_at) VALUES %s ON CONFLICT ON CONSTRAINT repo_pending_permissions_perm_unique DO UPDATE SET - user_ids = excluded.user_ids, user_ids_ints = excluded.user_ids_ints, updated_at = excluded.updated_at ` @@ -862,19 +826,13 @@ DO UPDATE SET items := make([]*sqlf.Query, len(ps)) for i := range ps { ps[i].UserIDs.RunOptimize() - ids, err := ps[i].UserIDs.ToBytes() - if err != nil { - return nil, err - } - if ps[i].UpdatedAt.IsZero() { return nil, ErrPermsUpdatedAtNotSet } - items[i] = sqlf.Sprintf("(%s, %s, %s, %s, %s)", + items[i] = sqlf.Sprintf("(%s, %s, %s, %s)", ps[i].RepoID, ps[i].Perm.String(), - ids, pq.Array(ps[i].UserIDs.ToArray()), ps[i].UpdatedAt.UTC(), ) @@ -1008,7 +966,7 @@ func (s *PermsStore) GrantPendingPermissions(ctx context.Context, userID int32, func loadRepoPermissionsBatchQuery(repoIDs []uint32, perm authz.Perms, lock string) *sqlf.Query { const format = ` -- source: enterprise/internal/db/perms_store.go:loadRepoPermissionsBatchQuery -SELECT repo_id, user_ids, user_ids_ints +SELECT repo_id, user_ids_ints FROM repo_permissions WHERE repo_id IN (%s) AND permission = %s @@ -1029,13 +987,12 @@ func upsertRepoPermissionsBatchQuery(ps ...*authz.RepoPermissions) (*sqlf.Query, const format = ` -- source: enterprise/internal/db/perms_store.go:upsertRepoPermissionsBatchQuery INSERT INTO repo_permissions - (repo_id, permission, user_ids, user_ids_ints, updated_at) + (repo_id, permission, user_ids_ints, updated_at) VALUES %s ON CONFLICT ON CONSTRAINT repo_permissions_perm_unique DO UPDATE SET - user_ids = excluded.user_ids, user_ids_ints = excluded.user_ids_ints, updated_at = excluded.updated_at ` @@ -1043,19 +1000,13 @@ DO UPDATE SET items := make([]*sqlf.Query, len(ps)) for i := range ps { ps[i].UserIDs.RunOptimize() - ids, err := ps[i].UserIDs.ToBytes() - if err != nil { - return nil, err - } - if ps[i].UpdatedAt.IsZero() { return nil, ErrPermsUpdatedAtNotSet } - items[i] = sqlf.Sprintf("(%s, %s, %s, %s, %s)", + items[i] = sqlf.Sprintf("(%s, %s, %s, %s)", ps[i].RepoID, ps[i].Perm.String(), - ids, pq.Array(ps[i].UserIDs.ToArray()), ps[i].UpdatedAt.UTC(), ) @@ -1099,7 +1050,7 @@ func (s *PermsStore) ListPendingUsers(ctx context.Context, serviceType, serviceI defer save(&err) q := sqlf.Sprintf(` -SELECT bind_id, object_ids, object_ids_ints +SELECT bind_id, object_ids_ints FROM user_pending_permissions WHERE service_type = %s AND service_id = %s @@ -1114,27 +1065,14 @@ AND service_id = %s for rows.Next() { var bindID string - var binary []byte var ids []int64 - if err = rows.Scan(&bindID, &binary, pq.Array(&ids)); err != nil { + if err = rows.Scan(&bindID, pq.Array(&ids)); err != nil { return nil, err } bm := roaring.NewBitmap() - - // Fallback to permissions stored in binary format if the new format is in the initial state. - if len(ids) == 0 { - if len(binary) == 0 { - continue // Ignore malformed data - } - - if err = bm.UnmarshalBinary(binary); err != nil { - return nil, err - } - } else { - for _, id := range ids { - bm.Add(uint32(id)) - } + for _, id := range ids { + bm.Add(uint32(id)) } // This user has no pending permissions, only has an empty record @@ -1256,11 +1194,10 @@ func (s *PermsStore) load(ctx context.Context, q *sqlf.Query) (*permsLoadValues, } var id int32 - var binary []byte var ids []int64 var updatedAt time.Time var syncedAt time.Time - if err = rows.Scan(&id, &binary, pq.Array(&ids), &updatedAt, &dbutil.NullTime{Time: &syncedAt}); err != nil { + if err = rows.Scan(&id, pq.Array(&ids), &updatedAt, &dbutil.NullTime{Time: &syncedAt}); err != nil { return nil, err } @@ -1274,20 +1211,8 @@ func (s *PermsStore) load(ctx context.Context, q *sqlf.Query) (*permsLoadValues, syncedAt: syncedAt, updatedAt: updatedAt, } - - // Fallback to permissions stored in binary format if the new format is in the initial state. - if len(ids) == 0 { - if len(binary) == 0 { - return vals, nil // Ignore malformed data - } - - if err = vals.ids.UnmarshalBinary(binary); err != nil { - return nil, err - } - } else { - for _, id := range ids { - vals.ids.Add(uint32(id)) - } + for _, id := range ids { + vals.ids.Add(uint32(id)) } return vals, nil } @@ -1313,29 +1238,16 @@ func (s *PermsStore) batchLoadIDs(ctx context.Context, q *sqlf.Query) (map[int32 loaded := make(map[int32]*roaring.Bitmap) for rows.Next() { var id int32 - var binary []byte var ids []int64 - if err = rows.Scan(&id, &binary, pq.Array(&ids)); err != nil { + if err = rows.Scan(&id, pq.Array(&ids)); err != nil { return nil, err } bm := roaring.NewBitmap() - loaded[id] = bm - - // Fallback to permissions stored in binary format if the new format is in the initial state. - if len(ids) == 0 { - if len(binary) == 0 { - continue // Ignore malformed data - } - - if err = bm.UnmarshalBinary(binary); err != nil { - return nil, err - } - } else { - for _, id := range ids { - bm.Add(uint32(id)) - } + for _, id := range ids { + bm.Add(uint32(id)) } + loaded[id] = bm } if err = rows.Err(); err != nil { return nil, err @@ -1693,113 +1605,3 @@ func (s *PermsStore) observe(ctx context.Context, family, title string) (context tr.Finish() } } - -// MigrateBinaryToIntarray performs migration on permissions tables which -// replicates data previously stored as binary (`BYTEA`) to intarray (`INT[]`). -func (s *PermsStore) MigrateBinaryToIntarray(ctx context.Context, batchSize int) error { - migrations := []struct { - batchQuery string - batchUpdate string - }{ - { - batchQuery: ` - SELECT user_id, object_ids, object_ids_ints - FROM user_permissions - WHERE user_id > %s AND ICOUNT(object_ids_ints) = 0 - ORDER BY user_id ASC - LIMIT %s - `, - batchUpdate: ` - UPDATE user_permissions - SET object_ids_ints = update.ints - FROM (VALUES %s) AS update (id, ints) - WHERE user_permissions.user_id = update.id - `, - }, - { - batchQuery: ` - SELECT repo_id, user_ids, user_ids_ints - FROM repo_permissions - WHERE repo_id > %s AND ICOUNT(user_ids_ints) = 0 - ORDER BY repo_id ASC - LIMIT %s - `, - batchUpdate: ` - UPDATE repo_permissions - SET user_ids_ints = update.ints - FROM (VALUES %s) AS update (id, ints) - WHERE repo_permissions.repo_id = update.id - `, - }, - { - batchQuery: ` - SELECT id, object_ids, object_ids_ints - FROM user_pending_permissions - WHERE id > %s AND ICOUNT(object_ids_ints) = 0 - ORDER BY id ASC - LIMIT %s - `, - batchUpdate: ` - UPDATE user_pending_permissions - SET object_ids_ints = update.ints - FROM (VALUES %s) AS update (id, ints) - WHERE user_pending_permissions.id = update.id - `, - }, - { - batchQuery: ` - SELECT repo_id, user_ids, user_ids_ints - FROM repo_pending_permissions - WHERE repo_id > %s AND ICOUNT(user_ids_ints) = 0 - ORDER BY repo_id ASC - LIMIT %s - `, - batchUpdate: ` - UPDATE repo_pending_permissions - SET user_ids_ints = update.ints - FROM (VALUES %s) AS update (id, ints) - WHERE repo_pending_permissions.repo_id = update.id - `, - }, - } - for _, m := range migrations { - var cursor int32 - for { - q := sqlf.Sprintf(m.batchQuery, cursor, batchSize) - loaded, err := s.batchLoadIDs(ctx, q) - if err != nil { - return errors.Wrap(err, "batch load") - } - - if len(loaded) == 0 { - break // Exit if no more rows need to be migrated - } - - items := make([]*sqlf.Query, 0, len(loaded)) - for id, bm := range loaded { - items = append(items, sqlf.Sprintf("(%s::INT, %s::INT[])", - id, - pq.Array(bm.ToArray()), - )) - - if id > cursor { - cursor = id - } - } - - q = sqlf.Sprintf( - m.batchUpdate, - sqlf.Join(items, ","), - ) - if err = s.execute(ctx, q); err != nil { - return errors.Wrap(err, "batch update") - } - - if len(loaded) < batchSize { - break // Exit when less results than expected, this is our last batch - } - } - } - - return nil -} diff --git a/enterprise/internal/db/perms_store_test.go b/enterprise/internal/db/perms_store_test.go index 1484c0e26b1..7dca9799666 100644 --- a/enterprise/internal/db/perms_store_test.go +++ b/enterprise/internal/db/perms_store_test.go @@ -259,18 +259,11 @@ func checkRegularPermsTable(s *PermsStore, sql string, expects map[int32][]uint3 for rows.Next() { var id int32 - var binary []byte var ids []int64 - if err = rows.Scan(&id, &binary, pq.Array(&ids)); err != nil { + if err = rows.Scan(&id, pq.Array(&ids)); err != nil { return err } - bm := roaring.NewBitmap() - if err = bm.UnmarshalBinary(binary); err != nil { - return err - } - binaryIDs := bm.ToArray() - intIDs := make([]uint32, 0, len(ids)) for _, id := range ids { intIDs = append(intIDs, uint32(id)) @@ -281,12 +274,7 @@ func checkRegularPermsTable(s *PermsStore, sql string, expects map[int32][]uint3 } want := fmt.Sprintf("%v", expects[id]) - have := fmt.Sprintf("%v", binaryIDs) - if have != want { - return fmt.Errorf("binaryIDs - key %v: want %q but got %q", id, want, have) - } - - have = fmt.Sprintf("%v", intIDs) + have := fmt.Sprintf("%v", intIDs) if have != want { return fmt.Errorf("intIDs - key %v: want %q but got %q", id, want, have) } @@ -468,12 +456,12 @@ func testPermsStore_SetUserPermissions(db *sql.DB) func(*testing.T) { } } - err := checkRegularPermsTable(s, `SELECT user_id, object_ids, object_ids_ints FROM user_permissions`, test.expectUserPerms) + err := checkRegularPermsTable(s, `SELECT user_id, object_ids_ints FROM user_permissions`, test.expectUserPerms) if err != nil { t.Fatal("user_permissions:", err) } - err = checkRegularPermsTable(s, `SELECT repo_id, user_ids, user_ids_ints FROM repo_permissions`, test.expectRepoPerms) + err = checkRegularPermsTable(s, `SELECT repo_id, user_ids_ints FROM repo_permissions`, test.expectRepoPerms) if err != nil { t.Fatal("repo_permissions:", err) } @@ -644,12 +632,12 @@ func testPermsStore_SetRepoPermissions(db *sql.DB) func(*testing.T) { } } - err := checkRegularPermsTable(s, `SELECT user_id, object_ids, object_ids_ints FROM user_permissions`, test.expectUserPerms) + err := checkRegularPermsTable(s, `SELECT user_id, object_ids_ints FROM user_permissions`, test.expectUserPerms) if err != nil { t.Fatal("user_permissions:", err) } - err = checkRegularPermsTable(s, `SELECT repo_id, user_ids, user_ids_ints FROM repo_permissions`, test.expectRepoPerms) + err = checkRegularPermsTable(s, `SELECT repo_id, user_ids_ints FROM repo_permissions`, test.expectRepoPerms) if err != nil { t.Fatal("repo_permissions:", err) } @@ -836,7 +824,7 @@ func checkUserPendingPermsTable( idToSpecs map[int32]extsvc.AccountSpec, err error, ) { - q := `SELECT id, service_type, service_id, bind_id, object_ids, object_ids_ints FROM user_pending_permissions` + q := `SELECT id, service_type, service_id, bind_id, object_ids_ints FROM user_pending_permissions` rows, err := s.db.QueryContext(ctx, q) if err != nil { return nil, err @@ -847,19 +835,12 @@ func checkUserPendingPermsTable( for rows.Next() { var id int32 var spec extsvc.AccountSpec - var binary []byte var ids []int64 - if err := rows.Scan(&id, &spec.ServiceType, &spec.ServiceID, &spec.AccountID, &binary, pq.Array(&ids)); err != nil { + if err := rows.Scan(&id, &spec.ServiceType, &spec.ServiceID, &spec.AccountID, pq.Array(&ids)); err != nil { return nil, err } idToSpecs[id] = spec - bm := roaring.NewBitmap() - if err = bm.UnmarshalBinary(binary); err != nil { - return nil, err - } - binaryIDs := bm.ToArray() - intIDs := make([]uint32, 0, len(ids)) for _, id := range ids { intIDs = append(intIDs, uint32(id)) @@ -870,12 +851,7 @@ func checkUserPendingPermsTable( } want := fmt.Sprintf("%v", expects[spec]) - have := fmt.Sprintf("%v", binaryIDs) - if have != want { - return nil, fmt.Errorf("binaryIDs - spec %q: want %q but got %q", spec, want, have) - } - - have = fmt.Sprintf("%v", intIDs) + have := fmt.Sprintf("%v", intIDs) if have != want { return nil, fmt.Errorf("intIDs - spec %q: want %q but got %q", spec, want, have) } @@ -899,25 +875,18 @@ func checkRepoPendingPermsTable( idToSpecs map[int32]extsvc.AccountSpec, expects map[int32][]extsvc.AccountSpec, ) error { - rows, err := s.db.QueryContext(ctx, `SELECT repo_id, user_ids, user_ids_ints FROM repo_pending_permissions`) + rows, err := s.db.QueryContext(ctx, `SELECT repo_id, user_ids_ints FROM repo_pending_permissions`) if err != nil { return err } for rows.Next() { var id int32 - var binary []byte var ids []int64 - if err := rows.Scan(&id, &binary, pq.Array(&ids)); err != nil { + if err := rows.Scan(&id, pq.Array(&ids)); err != nil { return err } - bm := roaring.NewBitmap() - if err = bm.UnmarshalBinary(binary); err != nil { - return err - } - binaryIDs := bm.ToArray() - intIDs := make([]int, 0, len(ids)) for _, id := range ids { intIDs = append(intIDs, int(id)) @@ -928,22 +897,7 @@ func checkRepoPendingPermsTable( } want := fmt.Sprintf("%v", expects[id]) - haveSpecs := make([]extsvc.AccountSpec, 0, len(binaryIDs)) - for _, userID := range binaryIDs { - spec, ok := idToSpecs[int32(userID)] - if !ok { - continue - } - - haveSpecs = append(haveSpecs, spec) - } - - have := fmt.Sprintf("%v", haveSpecs) - if have != want { - return fmt.Errorf("binaryIDs - id %d: want %q but got %q", id, want, have) - } - - haveSpecs = make([]extsvc.AccountSpec, 0, len(intIDs)) + haveSpecs := make([]extsvc.AccountSpec, 0, len(intIDs)) for _, userID := range intIDs { spec, ok := idToSpecs[int32(userID)] if !ok { @@ -953,7 +907,7 @@ func checkRepoPendingPermsTable( haveSpecs = append(haveSpecs, spec) } - have = fmt.Sprintf("%v", haveSpecs) + have := fmt.Sprintf("%v", haveSpecs) if have != want { return fmt.Errorf("intIDs - id %d: want %q but got %q", id, want, have) } @@ -1620,12 +1574,12 @@ func testPermsStore_GrantPendingPermissions(db *sql.DB) func(*testing.T) { } } - err := checkRegularPermsTable(s, `SELECT user_id, object_ids, object_ids_ints FROM user_permissions`, test.expectUserPerms) + err := checkRegularPermsTable(s, `SELECT user_id, object_ids_ints FROM user_permissions`, test.expectUserPerms) if err != nil { t.Fatal("user_permissions:", err) } - err = checkRegularPermsTable(s, `SELECT repo_id, user_ids, user_ids_ints FROM repo_permissions`, test.expectRepoPerms) + err = checkRegularPermsTable(s, `SELECT repo_id, user_ids_ints FROM repo_permissions`, test.expectRepoPerms) if err != nil { t.Fatal("repo_permissions:", err) } @@ -1911,221 +1865,6 @@ func testPermsStore_DatabaseDeadlocks(db *sql.DB) func(*testing.T) { } } -func testPermsStore_FallbackToOldFormat(db *sql.DB) func(t *testing.T) { - return func(t *testing.T) { - s := NewPermsStore(db, time.Now) - ctx := context.Background() - - t.Run("batchLoadUserPendingPermissions", func(t *testing.T) { - defer cleanupPermsTables(t, s) - - accounts := &extsvc.Accounts{ - ServiceType: authz.SourcegraphServiceType, - ServiceID: authz.SourcegraphServiceID, - AccountIDs: []string{"alice"}, - } - rp := &authz.RepoPermissions{ - RepoID: 1, - Perm: authz.Read, - } - if err := s.SetRepoPendingPermissions(ctx, accounts, rp); err != nil { - t.Fatal(err) - } - - // Reset "object_ids_ints" columns - q := sqlf.Sprintf(`UPDATE user_pending_permissions SET object_ids_ints = '{}'`) - if err := s.execute(ctx, q); err != nil { - t.Fatal(err) - } - - alice := &authz.UserPendingPermissions{ - ServiceType: authz.SourcegraphServiceType, - ServiceID: authz.SourcegraphServiceID, - BindID: "alice", - Perm: authz.Read, - Type: authz.PermRepos, - } - if err := s.LoadUserPendingPermissions(ctx, alice); err != nil { - t.Fatal(err) - } - equal(t, "IDs", []int{1}, bitmapToArray(alice.IDs)) - - q = loadUserPendingPermissionsByIDBatchQuery([]uint32{uint32(alice.ID)}, alice.Perm, alice.Type, "") - loaded, err := s.batchLoadIDs(ctx, q) - if err != nil { - t.Fatal(err) - } - equal(t, "loaded", []int{1}, bitmapToArray(loaded[alice.ID])) - }) - - t.Run("batchLoadIDs", func(t *testing.T) { - defer cleanupPermsTables(t, s) - - rp := &authz.RepoPermissions{ - RepoID: 1, - Perm: authz.Read, - UserIDs: toBitmap(2), - } - if err := s.SetRepoPermissions(ctx, rp); err != nil { - t.Fatal(err) - } - - // Reset "object_ids_ints" columns - q := sqlf.Sprintf(`UPDATE user_permissions SET object_ids_ints = '{}'`) - if err := s.execute(ctx, q); err != nil { - t.Fatal(err) - } - - q = loadUserPermissionsBatchQuery([]uint32{uint32(2)}, rp.Perm, authz.PermRepos, "") - loaded, err := s.batchLoadIDs(ctx, q) - if err != nil { - t.Fatal(err) - } - equal(t, "loaded", []int{1}, bitmapToArray(loaded[2])) - }) - - t.Run("ListPendingUsers", func(t *testing.T) { - defer cleanupPermsTables(t, s) - - accounts := &extsvc.Accounts{ - ServiceType: authz.SourcegraphServiceType, - ServiceID: authz.SourcegraphServiceID, - AccountIDs: []string{"alice"}, - } - rp := &authz.RepoPermissions{ - RepoID: 1, - Perm: authz.Read, - } - if err := s.SetRepoPendingPermissions(ctx, accounts, rp); err != nil { - t.Fatal(err) - } - - // Reset "object_ids_ints" columns - q := sqlf.Sprintf(`UPDATE user_pending_permissions SET object_ids_ints = '{}'`) - if err := s.execute(ctx, q); err != nil { - t.Fatal(err) - } - - bindIDs, err := s.ListPendingUsers(ctx, accounts.ServiceType, accounts.ServiceID) - if err != nil { - t.Fatal(err) - } - equal(t, "bindIDs", []string{"alice"}, bindIDs) - }) - } -} - -func testPermsStore_MigrateBinaryToIntarray(db *sql.DB) func(t *testing.T) { - return func(t *testing.T) { - s := NewPermsStore(db, time.Now) - ctx := context.Background() - - defer cleanupPermsTables(t, s) - - // Set up test permissions - rps := []*authz.RepoPermissions{ - { - RepoID: 1, - Perm: authz.Read, - UserIDs: toBitmap(11, 22), - }, - { - RepoID: 2, - Perm: authz.Read, - UserIDs: toBitmap(11, 33), - }, - { - RepoID: 3, - Perm: authz.Read, - UserIDs: toBitmap(22, 44), - }, - } - for _, rp := range rps { - if err := s.SetRepoPermissions(ctx, rp); err != nil { - t.Fatal(err) - } - } - - accounts := &extsvc.Accounts{ - ServiceType: authz.SourcegraphServiceType, - ServiceID: authz.SourcegraphServiceID, - AccountIDs: []string{"alice", "bob"}, - } - rp := &authz.RepoPermissions{ - RepoID: 1, - Perm: authz.Read, - } - if err := s.SetRepoPendingPermissions(ctx, accounts, rp); err != nil { - t.Fatal(err) - } - - // Reset "*_ints" columns - q := sqlf.Sprintf(` -UPDATE user_permissions SET object_ids_ints = '{}'; -UPDATE repo_permissions SET user_ids_ints = '{}'; -UPDATE user_pending_permissions SET object_ids_ints = '{}'; -UPDATE repo_pending_permissions SET user_ids_ints = '{}'; -`) - if err := s.execute(ctx, q); err != nil { - t.Fatal(err) - } - - if err := s.MigrateBinaryToIntarray(ctx, 2); err != nil { - t.Fatal(err) - } - - // Query and check rows in permissions tables. - err := checkRegularPermsTable(s, `SELECT user_id, object_ids, object_ids_ints FROM user_permissions`, - map[int32][]uint32{ - 11: {1, 2}, - 22: {1, 3}, - 33: {2}, - 44: {3}, - }, - ) - if err != nil { - t.Fatal("user_permissions:", err) - } - - err = checkRegularPermsTable(s, `SELECT repo_id, user_ids, user_ids_ints FROM repo_permissions`, - map[int32][]uint32{ - 1: {11, 22}, - 2: {11, 33}, - 3: {22, 44}, - }, - ) - if err != nil { - t.Fatal("repo_permissions:", err) - } - - alice := extsvc.AccountSpec{ - ServiceType: authz.SourcegraphServiceType, - ServiceID: authz.SourcegraphServiceID, - AccountID: "alice", - } - bob := extsvc.AccountSpec{ - ServiceType: authz.SourcegraphServiceType, - ServiceID: authz.SourcegraphServiceID, - AccountID: "bob", - } - - idToSpecs, err := checkUserPendingPermsTable(ctx, s, map[extsvc.AccountSpec][]uint32{ - alice: {1}, - bob: {1}, - }) - if err != nil { - t.Fatal("user_pending_permissions:", err) - } - - err = checkRepoPendingPermsTable(ctx, s, idToSpecs, map[int32][]extsvc.AccountSpec{ - 1: {alice, bob}, - }) - if err != nil { - t.Fatal("repo_pending_permissions:", err) - } - } -} - func cleanupUsersTable(t *testing.T, s *PermsStore) { if t.Failed() { return diff --git a/internal/db/schema.md b/internal/db/schema.md index 085c72c4aad..88d993c4e34 100644 --- a/internal/db/schema.md +++ b/internal/db/schema.md @@ -992,7 +992,7 @@ Triggers: ---------------+--------------------------+---------------------------------- repo_id | integer | not null permission | text | not null - user_ids | bytea | not null + user_ids | bytea | not null default '\x'::bytea updated_at | timestamp with time zone | not null user_ids_ints | integer[] | not null default '{}'::integer[] Indexes: @@ -1006,7 +1006,7 @@ Indexes: ---------------+--------------------------+---------------------------------- repo_id | integer | not null permission | text | not null - user_ids | bytea | not null + user_ids | bytea | not null default '\x'::bytea updated_at | timestamp with time zone | not null synced_at | timestamp with time zone | user_ids_ints | integer[] | not null default '{}'::integer[] @@ -1168,7 +1168,7 @@ Foreign-key constraints: bind_id | text | not null permission | text | not null object_type | text | not null - object_ids | bytea | not null + object_ids | bytea | not null default '\x'::bytea updated_at | timestamp with time zone | not null service_type | text | not null service_id | text | not null @@ -1185,7 +1185,7 @@ Indexes: user_id | integer | not null permission | text | not null object_type | text | not null - object_ids | bytea | not null + object_ids | bytea | not null default '\x'::bytea updated_at | timestamp with time zone | not null synced_at | timestamp with time zone | object_ids_ints | integer[] | not null default '{}'::integer[] diff --git a/migrations/frontend/1528395733_add_permissions_object_ids_default.down.sql b/migrations/frontend/1528395733_add_permissions_object_ids_default.down.sql new file mode 100644 index 00000000000..d4151c3302d --- /dev/null +++ b/migrations/frontend/1528395733_add_permissions_object_ids_default.down.sql @@ -0,0 +1,8 @@ +BEGIN; + +ALTER TABLE repo_permissions ALTER COLUMN user_ids DROP DEFAULT; +ALTER TABLE user_permissions ALTER COLUMN object_ids DROP DEFAULT; +ALTER TABLE repo_pending_permissions ALTER COLUMN user_ids DROP DEFAULT; +ALTER TABLE user_pending_permissions ALTER COLUMN object_ids DROP DEFAULT; + +COMMIT; diff --git a/migrations/frontend/1528395733_add_permissions_object_ids_default.up.sql b/migrations/frontend/1528395733_add_permissions_object_ids_default.up.sql new file mode 100644 index 00000000000..70ce6ec9013 --- /dev/null +++ b/migrations/frontend/1528395733_add_permissions_object_ids_default.up.sql @@ -0,0 +1,8 @@ +BEGIN; + +ALTER TABLE repo_permissions ALTER COLUMN user_ids SET DEFAULT '\x'; +ALTER TABLE user_permissions ALTER COLUMN object_ids SET DEFAULT '\x'; +ALTER TABLE repo_pending_permissions ALTER COLUMN user_ids SET DEFAULT '\x'; +ALTER TABLE user_pending_permissions ALTER COLUMN object_ids SET DEFAULT '\x'; + +COMMIT; diff --git a/migrations/frontend/bindata.go b/migrations/frontend/bindata.go index a9b3722c872..d9677263c4e 100644 --- a/migrations/frontend/bindata.go +++ b/migrations/frontend/bindata.go @@ -98,6 +98,8 @@ // 1528395731_add_nearest_upload_direction.up.sql (510B) // 1528395732_add_external_services_sync_jobs_state_index.down.sql (76B) // 1528395732_add_external_services_sync_jobs_state_index.up.sql (120B) +// 1528395733_add_permissions_object_ids_default.down.sql (297B) +// 1528395733_add_permissions_object_ids_default.up.sql (313B) package migrations @@ -2126,6 +2128,46 @@ func _1528395732_add_external_services_sync_jobs_state_indexUpSql() (*asset, err return a, nil } +var __1528395733_add_permissions_object_ids_defaultDownSql = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x72\x72\x75\xf7\xf4\xb3\xe6\xe2\x72\xf4\x09\x71\x0d\x52\x08\x71\x74\xf2\x71\x55\x28\x4a\x2d\xc8\x8f\x2f\x48\x2d\xca\xcd\x2c\x2e\xce\xcc\xcf\x2b\x56\x80\x48\x3a\xfb\xfb\x84\xfa\xfa\x29\x94\x16\xa7\x16\xc5\x67\xa6\x14\x2b\xb8\x04\xf9\x07\x28\xb8\xb8\xba\x39\x86\xfa\x84\x58\xa3\x18\x00\x56\x82\xd3\x80\xfc\xa4\xac\xd4\xe4\x12\x02\x46\x40\xdd\x90\x97\x92\x99\x97\x4e\x0d\xb7\x10\x30\x08\xa7\x9b\xb8\x9c\xfd\x7d\x7d\x3d\x43\xac\xb9\x00\x01\x00\x00\xff\xff\xd7\x70\x8a\xe9\x29\x01\x00\x00") + +func _1528395733_add_permissions_object_ids_defaultDownSqlBytes() ([]byte, error) { + return bindataRead( + __1528395733_add_permissions_object_ids_defaultDownSql, + "1528395733_add_permissions_object_ids_default.down.sql", + ) +} + +func _1528395733_add_permissions_object_ids_defaultDownSql() (*asset, error) { + bytes, err := _1528395733_add_permissions_object_ids_defaultDownSqlBytes() + if err != nil { + return nil, err + } + + info := bindataFileInfo{name: "1528395733_add_permissions_object_ids_default.down.sql", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)} + a := &asset{bytes: bytes, info: info, digest: [32]uint8{0x21, 0x47, 0x84, 0x12, 0xbd, 0x21, 0xc, 0x7, 0x24, 0xd8, 0x73, 0x42, 0x1f, 0x58, 0xbd, 0x17, 0x48, 0xd3, 0x25, 0xeb, 0xab, 0x25, 0xbf, 0x32, 0x32, 0x85, 0xc6, 0xaf, 0x2a, 0xf3, 0x10, 0x30}} + return a, nil +} + +var __1528395733_add_permissions_object_ids_defaultUpSql = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x72\x72\x75\xf7\xf4\xb3\xe6\xe2\x72\xf4\x09\x71\x0d\x52\x08\x71\x74\xf2\x71\x55\x28\x4a\x2d\xc8\x8f\x2f\x48\x2d\xca\xcd\x2c\x2e\xce\xcc\xcf\x2b\x56\x80\x48\x3a\xfb\xfb\x84\xfa\xfa\x29\x94\x16\xa7\x16\xc5\x67\xa6\x14\x2b\x04\xbb\x86\x28\xb8\xb8\xba\x39\x86\xfa\x84\x28\xa8\xc7\x54\xa8\x5b\xa3\x18\x02\x56\x86\xd3\x90\xfc\xa4\xac\xd4\xe4\x12\x22\x8c\x81\xba\x25\x2f\x25\x33\x2f\x9d\x5a\x6e\x22\x60\x18\x5e\xb7\x71\x39\xfb\xfb\xfa\x7a\x86\x58\x73\x01\x02\x00\x00\xff\xff\xa0\x76\x6b\x75\x39\x01\x00\x00") + +func _1528395733_add_permissions_object_ids_defaultUpSqlBytes() ([]byte, error) { + return bindataRead( + __1528395733_add_permissions_object_ids_defaultUpSql, + "1528395733_add_permissions_object_ids_default.up.sql", + ) +} + +func _1528395733_add_permissions_object_ids_defaultUpSql() (*asset, error) { + bytes, err := _1528395733_add_permissions_object_ids_defaultUpSqlBytes() + if err != nil { + return nil, err + } + + info := bindataFileInfo{name: "1528395733_add_permissions_object_ids_default.up.sql", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)} + a := &asset{bytes: bytes, info: info, digest: [32]uint8{0xbc, 0xca, 0x93, 0xad, 0x9e, 0xbc, 0xa0, 0xaa, 0xb4, 0xef, 0xbc, 0x96, 0x3, 0x26, 0x9f, 0x0, 0xe6, 0x56, 0x11, 0xa5, 0x67, 0x0, 0x3a, 0x72, 0x77, 0x2b, 0x2c, 0x87, 0x1d, 0x54, 0x30, 0x7c}} + return a, nil +} + // Asset loads and returns the asset for the given name. // It returns an error if the asset could not be found or // could not be loaded. @@ -2315,6 +2357,8 @@ var _bindata = map[string]func() (*asset, error){ "1528395731_add_nearest_upload_direction.up.sql": _1528395731_add_nearest_upload_directionUpSql, "1528395732_add_external_services_sync_jobs_state_index.down.sql": _1528395732_add_external_services_sync_jobs_state_indexDownSql, "1528395732_add_external_services_sync_jobs_state_index.up.sql": _1528395732_add_external_services_sync_jobs_state_indexUpSql, + "1528395733_add_permissions_object_ids_default.down.sql": _1528395733_add_permissions_object_ids_defaultDownSql, + "1528395733_add_permissions_object_ids_default.up.sql": _1528395733_add_permissions_object_ids_defaultUpSql, } // AssetDebug is true if the assets were built with the debug flag enabled. @@ -2459,6 +2503,8 @@ var _bintree = &bintree{nil, map[string]*bintree{ "1528395731_add_nearest_upload_direction.up.sql": {_1528395731_add_nearest_upload_directionUpSql, map[string]*bintree{}}, "1528395732_add_external_services_sync_jobs_state_index.down.sql": {_1528395732_add_external_services_sync_jobs_state_indexDownSql, map[string]*bintree{}}, "1528395732_add_external_services_sync_jobs_state_index.up.sql": {_1528395732_add_external_services_sync_jobs_state_indexUpSql, map[string]*bintree{}}, + "1528395733_add_permissions_object_ids_default.down.sql": {_1528395733_add_permissions_object_ids_defaultDownSql, map[string]*bintree{}}, + "1528395733_add_permissions_object_ids_default.up.sql": {_1528395733_add_permissions_object_ids_defaultUpSql, map[string]*bintree{}}, }} // RestoreAsset restores an asset under the given directory.