From fa39fe5b0f34ad061740e567ff7805e9f5feed5c Mon Sep 17 00:00:00 2001 From: Todd Date: Tue, 11 Oct 2022 16:59:52 -0700 Subject: [PATCH] Delete the base auth account when a concrete auth account is deleted (#2528) --- CHANGELOG.md | 7 +- .../migrations/oss/postgres/56/01_auth.up.sql | 33 ++++ .../migrations/oss/postgres_56_01_test.go | 185 ++++++++++++++++++ internal/db/sqltest/Makefile | 1 + .../db/sqltest/tests/account/oidc/account.sql | 35 ++++ .../tests/account/password/account.sql | 35 ++++ 6 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 internal/db/schema/migrations/oss/postgres/56/01_auth.up.sql create mode 100644 internal/db/schema/migrations/oss/postgres_56_01_test.go create mode 100644 internal/db/sqltest/tests/account/oidc/account.sql create mode 100644 internal/db/sqltest/tests/account/password/account.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 03eef6e2b..6571fece8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,12 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. - ## Next +## Next + +### Bug Fixes + +* accounts: Deleted auth accounts would still show up as being associated with + a User when reading the User ([PR](https://github.com/hashicorp/boundary/pull/2528)) ## 0.11.0 (2022/09/27) diff --git a/internal/db/schema/migrations/oss/postgres/56/01_auth.up.sql b/internal/db/schema/migrations/oss/postgres/56/01_auth.up.sql new file mode 100644 index 000000000..2b601a513 --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/56/01_auth.up.sql @@ -0,0 +1,33 @@ +begin; + +-- This deletes the base type if the concrete types exist anymore. +delete from auth_account +where public_id not in + (select public_id + from auth_oidc_account) +and public_id not in + (select public_id + from auth_password_account); + +-- delete_auth_account_subtype() is an after trigger function for subytypes of +-- auth_account. +-- this function is similar to the delete_auth_method_subtype account created +-- in 2/10_auth.up.sql +create function delete_auth_account_subtype() returns trigger +as $$ +begin + delete from auth_account + where public_id = old.public_id; + return null; -- results are ignore since this is an after trigger. +end; +$$ language plpgsql; +comment on function delete_auth_account_subtype is + 'delete_auth_account_subtype() is an after trigger function for subytypes of auth_account'; + +create trigger delete_auth_account_subtype after delete on auth_oidc_account + for each row execute procedure delete_auth_account_subtype(); + +create trigger delete_auth_account_subtype after delete on auth_password_account + for each row execute procedure delete_auth_account_subtype(); + +commit; \ No newline at end of file diff --git a/internal/db/schema/migrations/oss/postgres_56_01_test.go b/internal/db/schema/migrations/oss/postgres_56_01_test.go new file mode 100644 index 000000000..0d8f3c0cc --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres_56_01_test.go @@ -0,0 +1,185 @@ +package oss_test + +import ( + "context" + "crypto/rand" + "testing" + + "github.com/hashicorp/boundary/internal/auth/oidc" + "github.com/hashicorp/boundary/internal/auth/password" + "github.com/hashicorp/boundary/internal/db" + "github.com/hashicorp/boundary/internal/db/common" + "github.com/hashicorp/boundary/internal/db/schema" + "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/types/scope" + "github.com/hashicorp/boundary/testing/dbtest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMigrations_DeleteOrphanedAccounts(t *testing.T) { + const ( + priorMigration = 55001 + currentMigration = 56001 + ) + + t.Parallel() + ctx := context.Background() + dialect := dbtest.Postgres + + c, u, _, err := dbtest.StartUsingTemplate(dialect, dbtest.WithTemplate(dbtest.Template1)) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, c()) + }) + d, err := common.SqlOpen(dialect, u) + require.NoError(t, err) + + // migration to the prior migration (before the one we want to test) + m, err := schema.NewManager(ctx, schema.Dialect(dialect), d, schema.WithEditions( + schema.TestCreatePartialEditions(schema.Dialect(dialect), schema.PartialEditions{"oss": priorMigration}), + )) + require.NoError(t, err) + + _, err = m.ApplyMigrations(ctx) + require.NoError(t, err) + state, err := m.CurrentState(ctx) + require.NoError(t, err) + want := &schema.State{ + Initialized: true, + Editions: []schema.EditionState{ + { + Name: "oss", + BinarySchemaVersion: priorMigration, + DatabaseSchemaVersion: priorMigration, + DatabaseSchemaState: schema.Equal, + }, + }, + } + require.Equal(t, want, state) + + // Get a connection + dbType, err := db.StringToDbType(dialect) + require.NoError(t, err) + conn, err := db.Open(ctx, dbType, u) + require.NoError(t, err) + + wrapper := db.TestWrapper(t) + rw := db.New(conn) + kmsCache := kms.TestKms(t, conn, wrapper) + err = kmsCache.CreateKeys(context.Background(), scope.Global.String(), kms.WithRandomReader(rand.Reader)) + require.NoError(t, err) + + // Create the user we will associate accounts with + iamRepo := iam.TestRepo(t, conn, wrapper) + usr, err := iam.NewUser(scope.Global.String()) + require.NoError(t, err) + usr, err = iamRepo.CreateUser(ctx, usr) + require.NoError(t, err) + + // Create the accounts we will delete and assert their behavior + pwRepo, err := password.NewRepository(rw, rw, kmsCache) + require.NoError(t, err) + + pwAm1, err := password.NewAuthMethod(scope.Global.String()) + require.NoError(t, err) + pwAm1, err = pwRepo.CreateAuthMethod(ctx, pwAm1) + require.NoError(t, err) + pwAcct1, err := password.NewAccount(pwAm1.GetPublicId(), password.WithLoginName("account1"), password.WithPassword("password")) + require.NoError(t, err) + pwAcct1, err = pwRepo.CreateAccount(ctx, scope.Global.String(), pwAcct1) + require.NoError(t, err) + + pwAm2, err := password.NewAuthMethod(scope.Global.String()) + require.NoError(t, err) + pwAm2, err = pwRepo.CreateAuthMethod(ctx, pwAm2) + require.NoError(t, err) + pwAcct2, err := password.NewAccount(pwAm2.GetPublicId(), password.WithLoginName("account2"), password.WithPassword("password")) + require.NoError(t, err) + pwAcct2, err = pwRepo.CreateAccount(ctx, scope.Global.String(), pwAcct2) + require.NoError(t, err) + + oidcRepo, err := oidc.NewRepository(ctx, rw, rw, kmsCache) + require.NoError(t, err) + databaseWrapper, err := kmsCache.GetWrapper(context.Background(), scope.Global.String(), kms.KeyPurposeDatabase) + oidcAm1 := oidc.TestAuthMethod(t, conn, databaseWrapper, scope.Global.String(), oidc.InactiveState, "alice_rp1", + "my-dogs-name", oidc.WithIssuer(oidc.TestConvertToUrls(t, "https://alice1.com")[0]), oidc.WithApiUrl(oidc.TestConvertToUrls(t, "https://api1.com")[0])) + oidcAcct1, err := oidc.NewAccount(ctx, oidcAm1.GetPublicId(), "oidcAcct1") + require.NoError(t, err) + oidcAcct1, err = oidcRepo.CreateAccount(ctx, scope.Global.String(), oidcAcct1) + require.NoError(t, err) + oidcAm2 := oidc.TestAuthMethod(t, conn, databaseWrapper, scope.Global.String(), oidc.InactiveState, "alice_rp2", + "my-dogs-name", oidc.WithIssuer(oidc.TestConvertToUrls(t, "https://alice2.com")[0]), oidc.WithApiUrl(oidc.TestConvertToUrls(t, "https://api2.com")[0])) + oidcAcct2, err := oidc.NewAccount(ctx, oidcAm2.GetPublicId(), "oidcAcct2") + require.NoError(t, err) + oidcAcct2, err = oidcRepo.CreateAccount(ctx, scope.Global.String(), oidcAcct2) + require.NoError(t, err) + + _, err = iamRepo.AddUserAccounts(ctx, usr.GetPublicId(), usr.GetVersion(), + []string{ + pwAcct1.GetPublicId(), pwAcct2.GetPublicId(), + oidcAcct1.GetPublicId(), oidcAcct2.GetPublicId(), + }) + require.NoError(t, err) + usr, accts, err := iamRepo.LookupUser(ctx, usr.GetPublicId()) + require.NoError(t, err) + assert.ElementsMatch(t, []string{ + pwAcct1.GetPublicId(), pwAcct2.GetPublicId(), + oidcAcct1.GetPublicId(), oidcAcct2.GetPublicId(), + }, accts) + + _, err = pwRepo.DeleteAccount(ctx, scope.Global.String(), pwAcct1.GetPublicId()) + require.NoError(t, err) + _, err = oidcRepo.DeleteAccount(ctx, scope.Global.String(), oidcAcct1.GetPublicId()) + require.NoError(t, err) + + // pwAcct1 is still listed as an account for this user! oh no! + _, accts, err = iamRepo.LookupUser(ctx, usr.GetPublicId()) + require.NoError(t, err) + assert.ElementsMatch(t, []string{ + pwAcct1.GetPublicId(), pwAcct2.GetPublicId(), + oidcAcct1.GetPublicId(), oidcAcct2.GetPublicId(), + }, accts) + + // now we're ready for the migration we want to test. + m, err = schema.NewManager(ctx, schema.Dialect(dialect), d, schema.WithEditions( + schema.TestCreatePartialEditions(schema.Dialect(dialect), schema.PartialEditions{"oss": currentMigration}), + )) + require.NoError(t, err) + + _, err = m.ApplyMigrations(ctx) + require.NoError(t, err) + state, err = m.CurrentState(ctx) + require.NoError(t, err) + want = &schema.State{ + Initialized: true, + Editions: []schema.EditionState{ + { + Name: "oss", + BinarySchemaVersion: currentMigration, + DatabaseSchemaVersion: currentMigration, + DatabaseSchemaState: schema.Equal, + }, + }, + } + require.Equal(t, want, state) + + // pwAcct1 is no longer listed as an account for this user. Phew! + _, accts, err = iamRepo.LookupUser(ctx, usr.GetPublicId()) + require.NoError(t, err) + assert.ElementsMatch(t, []string{pwAcct2.GetPublicId(), oidcAcct2.GetPublicId()}, accts) + + _, err = pwRepo.DeleteAccount(ctx, scope.Global.String(), pwAcct2.GetPublicId()) + require.NoError(t, err) + _, accts, err = iamRepo.LookupUser(ctx, usr.GetPublicId()) + require.NoError(t, err) + assert.ElementsMatch(t, []string{oidcAcct2.GetPublicId()}, accts) + _, err = oidcRepo.DeleteAccount(ctx, scope.Global.String(), oidcAcct2.GetPublicId()) + require.NoError(t, err) + + // Deleting the account now removes the association with the user + _, accts, err = iamRepo.LookupUser(ctx, usr.GetPublicId()) + require.NoError(t, err) + assert.Empty(t, accts) +} diff --git a/internal/db/sqltest/Makefile b/internal/db/sqltest/Makefile index 20e8456dd..9a5c3c698 100644 --- a/internal/db/sqltest/Makefile +++ b/internal/db/sqltest/Makefile @@ -23,6 +23,7 @@ TESTS ?= tests/setup/*.sql \ tests/sentinel/*.sql \ tests/credential/*/*.sql \ tests/session/*.sql \ + tests/account/*/*.sql \ tests/target/*.sql \ tests/controller/*.sql diff --git a/internal/db/sqltest/tests/account/oidc/account.sql b/internal/db/sqltest/tests/account/oidc/account.sql new file mode 100644 index 000000000..e4fe998e8 --- /dev/null +++ b/internal/db/sqltest/tests/account/oidc/account.sql @@ -0,0 +1,35 @@ +-- account tests triggers: +-- insert_auth_account_subtype +-- delete_auth_account_subtype + +begin; +select plan(8); +select wtt_load('widgets', 'iam', 'kms', 'auth'); + +-- validate the setup data +select is(count(*), 1::bigint) from auth_oidc_account where public_id = 'aoa___walter'; +select is(count(*), 1::bigint) from auth_account where public_id = 'aoa___walter'; + +-- validate the insert triggers +prepare insert_oidc_account as + insert into auth_oidc_account + (auth_method_id, public_id, issuer, subject) + values + ('aom___widget', 'aoa___tania', 'https://widget.test', 'tania'); +select lives_ok('insert_oidc_account'); + +select is(count(*), 1::bigint) from auth_oidc_account where public_id = 'aoa___tania'; +select is(count(*), 1::bigint) from auth_account where public_id = 'aoa___tania'; + +-- validate the delete triggers +prepare delete_oidc_account as + delete + from auth_oidc_account + where public_id = 'aoa___tania'; +select lives_ok('delete_oidc_account'); + +select is(count(*), 0::bigint) from auth_oidc_account where public_id = 'aoa___tania'; +select is(count(*), 0::bigint) from auth_account where public_id = 'aoa___tania'; + +select * from finish(); +rollback; diff --git a/internal/db/sqltest/tests/account/password/account.sql b/internal/db/sqltest/tests/account/password/account.sql new file mode 100644 index 000000000..1203cf8fe --- /dev/null +++ b/internal/db/sqltest/tests/account/password/account.sql @@ -0,0 +1,35 @@ +-- account tests triggers: +-- insert_auth_account_subtype +-- delete_auth_account_subtype + +begin; +select plan(8); +select wtt_load('widgets', 'iam', 'kms', 'auth'); + +-- validate the setup data +select is(count(*), 1::bigint) from auth_password_account where public_id = 'apa___walter'; +select is(count(*), 1::bigint) from auth_account where public_id = 'apa___walter'; + +-- validate the insert triggers +prepare insert_password_account as + insert into auth_password_account + (auth_method_id, public_id, login_name) + values + ('apm___widget', 'apa___tania', 'tania'); +select lives_ok('insert_password_account'); + +select is(count(*), 1::bigint) from auth_password_account where public_id = 'apa___tania'; +select is(count(*), 1::bigint) from auth_account where public_id = 'apa___tania'; + +-- validate the delete triggers +prepare delete_password_account as + delete + from auth_password_account + where public_id = 'apa___tania'; +select lives_ok('delete_password_account'); + +select is(count(*), 0::bigint) from auth_password_account where public_id = 'apa___tania'; +select is(count(*), 0::bigint) from auth_account where public_id = 'apa___tania'; + +select * from finish(); +rollback;