Skip to content

fix(enterprise/dbcrypt): do not skip deleted users when encrypting or deleting #9694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 15, 2023
Merged

fix(enterprise/dbcrypt): do not skip deleted users when encrypting or deleting #9694

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
355b2c1
add some more logging to test
johnstcn Sep 15, 2023
39aa29e
validate working with multiple user statuses
johnstcn Sep 15, 2023
3950c09
fmt query
johnstcn Sep 15, 2023
49b9d90
create deleted users
johnstcn Sep 15, 2023
5c1b687
robustificate data generation
johnstcn Sep 15, 2023
9bb1a5d
only dump on test failure
johnstcn Sep 15, 2023
7da5972
more logging
johnstcn Sep 15, 2023
2f63e43
do not skip deleted users when encrypting or decrypting
johnstcn Sep 15, 2023
11f85bf
make AllUserIDs a fully-fledged query citizen
johnstcn Sep 15, 2023
0c11d6c
fixup! make AllUserIDs a fully-fledged query citizen
johnstcn Sep 15, 2023
0f164a3
fixup! fixup! make AllUserIDs a fully-fledged query citizen
johnstcn Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
only dump on test failure
  • Loading branch information
@johnstcn
johnstcn committed Sep 15, 2023
commit 9bb1a5d4ab57a73ba415235e094553e224f0acd5
23 changes: 12 additions & 11 deletions enterprise/cli/server_dbcrypt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"database/sql"
"encoding/base64"
"strings"
"testing"

"github.com/google/uuid"
Expand Down Expand Up @@ -42,9 +41,15 @@ func TestServerDBCrypt(t *testing.T) {
})
db := database.New(sqlDB)

t.Cleanup(func() {
if t.Failed() {
t.Logf("Dumping data due to failed test. I hope you find what you're looking for!")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, this should be a general pattern in coder/coder. Great idea!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Actually I'm wondering if it would make sense to have/write a full psql dump as well (or instead). It'd be great if the test author didn't need to consider all failure scenarios and the information would always be there in full (and repeatable by importing dump).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add that as a follow-up PR! Maybe something like dbtestutil.WithDumpOnFailure(t)?

dumpUsers(t, sqlDB)
}
})

// Populate the database with some unencrypted data.
users := genData(t, db)
dumpUsers(t, sqlDB, "NOT ENCRYPTED")

// Setup an initial cipher A
keyA := mustString(t, 32)
Expand All @@ -57,7 +62,6 @@ func TestServerDBCrypt(t *testing.T) {

// Populate the database with some encrypted data using cipher A.
newUsers := genData(t, cryptdb)
dumpUsers(t, sqlDB, "PARTIALLY ENCRYPTED A")

// Validate that newly created users were encrypted with cipher A
for _, usr := range newUsers {
Expand All @@ -76,7 +80,6 @@ func TestServerDBCrypt(t *testing.T) {
err = inv.Run()
require.NoError(t, err)

dumpUsers(t, sqlDB, "ENCRYPTED A")
// Validate that all existing data has been encrypted with cipher A.
for _, usr := range users {
requireEncryptedWithCipher(ctx, t, db, cipherA[0], usr.ID)
Expand All @@ -89,7 +92,6 @@ func TestServerDBCrypt(t *testing.T) {

// Generate some more encrypted data using the new cipher
users = append(users, genData(t, db)...)
dumpUsers(t, sqlDB, "ENCRYPTED AB")

inv, _ = newCLI(t, "server", "dbcrypt", "rotate",
"--postgres-url", connectionURL,
Expand All @@ -103,7 +105,6 @@ func TestServerDBCrypt(t *testing.T) {
require.NoError(t, err)

// Validate that all data has been re-encrypted with cipher B.
dumpUsers(t, sqlDB, "ENCRYPTED B")
for _, usr := range users {
requireEncryptedWithCipher(ctx, t, db, cipherBA[0], usr.ID)
}
Expand Down Expand Up @@ -150,7 +151,6 @@ func TestServerDBCrypt(t *testing.T) {
}

// Validate that all data has been decrypted.
dumpUsers(t, sqlDB, "DECRYPTED")
for _, usr := range users {
requireEncryptedWithCipher(ctx, t, db, &nullCipher{}, usr.ID)
}
Expand All @@ -172,7 +172,6 @@ func TestServerDBCrypt(t *testing.T) {
require.NoError(t, err)

// Validate that all data has been re-encrypted with cipher C.
dumpUsers(t, sqlDB, "ENCRYPTED C")
for _, usr := range users {
requireEncryptedWithCipher(ctx, t, db, cipherC[0], usr.ID)
}
Expand All @@ -189,7 +188,6 @@ func TestServerDBCrypt(t *testing.T) {
require.NoError(t, err)

// Assert that no user links remain.
dumpUsers(t, sqlDB, "DELETED")
for _, usr := range users {
userLinks, err := db.GetUserLinksByUserID(ctx, usr.ID)
require.NoError(t, err, "failed to get user links for user %s", usr.ID)
Expand Down Expand Up @@ -227,6 +225,9 @@ func genData(t *testing.T, db database.Store) []database.User {
OAuthAccessToken: "access-" + usr.ID.String(),
OAuthRefreshToken: "refresh-" + usr.ID.String(),
})
// Fun fact: our schema allows _all_ login types to have
// a user_link. Even though I'm not sure how it could occur
// in practice, making sure to test all combinations here.
_ = dbgen.UserLink(t, db, database.UserLink{
UserID: usr.ID,
LoginType: usr.LoginType,
Expand All @@ -240,8 +241,8 @@ func genData(t *testing.T, db database.Store) []database.User {
return users
}

func dumpUsers(t *testing.T, db *sql.DB, header string) {
t.Logf("%s %s %s", strings.Repeat("=", 20), header, strings.Repeat("=", 20))
func dumpUsers(t *testing.T, db *sql.DB) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit the OS test env for this test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there were the possibility of either leaking sensitive information or performing destructive database changes, I would say yes. I don't believe either of those are the case here, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, I'll be removing this in a follow-up PR and replacing it with a more general "pg_dump on test failure".

t.Helper()
rows, err := db.QueryContext(context.Background(), `SELECT
u.id,
u.login_type,
Expand Down