fix(enterprise/dbcrypt): do not skip deleted users when encrypting or deleting #9694
Conversation
| FROM users u | ||
| LEFT OUTER JOIN user_links ul ON u.id = ul.user_id | ||
| LEFT OUTER JOIN git_auth_links gal ON u.id = gal.user_id | ||
| ORDER BY u.created_at ASC;`) |
There was a problem hiding this comment.
Maybe we should add a test that this function works as expected since it's static and will only be run on failure, meaning it will most likely be outdated by the time it's needed. 😆
Perhaps this could be motivation for a testqueries package. Similar to what I did here: #9519 to generate a new (slim) package that can be wrapped around sql.DB.
There was a problem hiding this comment.
I'd definitely be in favour of this; I'm fairly sure there are at least a couple of queries that only ever get run in unit tests.
There was a problem hiding this comment.
Maybe we should add a test that this function works as expected since it's static and will only be run on failure, meaning it will most likely be outdated by the time it's needed. 😆
Golden files?
There was a problem hiding this comment.
I think verifying a few fields in the test is enough. Otherwise you need to stabilize the generated IDs.
There was a problem hiding this comment.
I think instead of making that query more complex I'll go ahead and add a follow-up PR to dump the entire test database on failure (optional, opt-in). Less work, less fuss, more benefit.
| @@ -240,6 +240,14 @@ func User(t testing.TB, db database.Store, orig database.User) database.User { | |||
| }) | |||
| require.NoError(t, err, "user last seen") | |||
| } | |||
|
|
|||
| if orig.Deleted { | |||
| err = db.UpdateUserDeletedByID(genCtx, database.UpdateUserDeletedByIDParams{ | |||
There was a problem hiding this comment.
FYI There is also dormant status, hopefully it does not change anything.
There was a problem hiding this comment.
All combinations of login_type, status, and deleted should be covered by this test.
There was a problem hiding this comment.
I'm fairly sure user dormancy and deletion are completely orthogonal i.e. both can be set independently.
| @@ -41,15 +44,38 @@ 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!") | |||
There was a problem hiding this comment.
oh, this should be a general pattern in coder/coder. Great idea!
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I can add that as a follow-up PR! Maybe something like dbtestutil.WithDumpOnFailure(t)?
| } | ||
| return users | ||
| } | ||
|
|
||
| func dumpUsers(t *testing.T, db *sql.DB) { |
There was a problem hiding this comment.
Should we limit the OS test env for this test?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
This PR fixes an issue where deleted users are not taken into account when encrypting or decrypting data.
GetUserswill by default omit soft-deleted users.