Purge old realmless users #1135
Conversation
google-cla
bot
added
the
cla: yes
|
I tried to do this as but that didn't seem to work |
| deleteBefore := time.Now().UTC().Add(maxAge) | ||
| // Delete users who were created/updated before the expiry time. | ||
| rtn := db.db.Unscoped(). | ||
| Where("users.system_admin = false AND users.created_at < ? AND users.updated_at < ?", deleteBefore, deleteBefore). |
There was a problem hiding this comment.
I don't think this does what you think it does?
- Realm membership changes don't change
updated_at, so you could end up immediately deleting someone - That column is updated anytime the direct user record changes, we can happen at login with the revoke check
There was a problem hiding this comment.
The latter isn't a problem - it's okay to not-delete users who are logging in to keep fresh.
I suppose we can ensure users is bumped on realm-remove
There was a problem hiding this comment.
I suppose we can ensure users is bumped on realm-remove
I couldn't figure out a good way to do that. If we can "touch" the update_at on the user when realm membership is changed, I think this is fine (maybe add a comment).
There was a problem hiding this comment.
I've added TestRemoveRealmUpdatesTime and I can't seem to repro your claim that modifying the realm relationship isn't bumping time
There was a problem hiding this comment.
Interesting... what happens if the user comes serialized from the cache?
There was a problem hiding this comment.
I believe the reason is that we have wrapped calls with SaveUser here:
which always saves both the User entry and the associations (even if the table fields haven't changed)
| deleteBefore := time.Now().UTC().Add(maxAge) | ||
| // Delete users who were created/updated before the expiry time. | ||
| rtn := db.db.Unscoped(). | ||
| Where("users.system_admin = false AND users.created_at < ? AND users.updated_at < ?", deleteBefore, deleteBefore). |
There was a problem hiding this comment.
Interesting... what happens if the user comes serialized from the cache?
|
not sure if we can use it - but firebase stores the last login date of a user. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
|
Hmm - you might need to rebase @whaught |
|
/retest |
|
/retest |
Fixes #606
Proposed Changes
Release Note