Skip to content
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.

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

feat(backend): periodically clean up expired rows #388

Merged
merged 9 commits into from
Aug 16, 2022

Conversation

dclipp
Copy link
Contributor

@dclipp dclipp commented Jul 9, 2022

Changes proposed in this pull request

  • add logic to auth to periodically delete database rows that we consider "expired"
  • add rule for cleaning up accessTokens table

Context

fixes #385

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass

@dclipp dclipp requested review from sabineschaller and njlie July 9, 2022 02:38
@dclipp dclipp self-assigned this Jul 9, 2022
@github-actions github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Jul 9, 2022
packages/backend/src/app.ts Outdated Show resolved Hide resolved
packages/backend/src/app.ts Outdated Show resolved Hide resolved
packages/backend/src/app.ts Outdated Show resolved Hide resolved
@dclipp dclipp force-pushed the dc-cleanup-expired-2 branch from fb6922a to 4c684b9 Compare July 22, 2022 23:00
@dclipp dclipp requested a review from sabineschaller July 23, 2022 00:23
@github-actions github-actions bot added pkg: auth Changes in the GNAP auth package. and removed pkg: backend Changes in the backend package. labels Jul 23, 2022
@matdehaast matdehaast requested a review from wilsonianb August 1, 2022 16:16
accessTokenExpirySeconds: envInt('ACCESS_TOKEN_EXPIRY_SECONDS', 10 * 60) // Default 10 minutes
accessTokenExpirySeconds: envInt('ACCESS_TOKEN_EXPIRY_SECONDS', 10 * 60), // Default 10 minutes
databaseCleanupWorkers: envInt('DATABASE_CLEANUP_WORKERS', 1),
accessTokenMinDaysBeforeDeletion: 30
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought 10 minutes was reasonable but it's not actually part of the spec. I can just hardcode it if you think that's cleaner, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this default is actually necessary.
We currently only have databaseCleanupRules for accessTokens, for which expiresIn is nullable but we appear to always define it.

I vote we make expiresIn non-nullable and remove accessTokenMinDaysBeforeDeletion

Copy link
Contributor

Choose a reason for hiding this comment

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

I second the above ☝️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could remove accessTokenMinDaysBeforeDeletion and make expiresIn non-nullable, but it seems like the spec recommends against removing expired tokens immediately:

Note that in many cases, the access token will have expired for regular use. To facilitate token rotation, the AS SHOULD honor the rotation request of the expired access token since it is likely that the client instance is attempting to refresh the expired token.

That makes me think that we'd still need to define a constant somewhere to prevent the auth service from cleaning up tokens before the client has a chance to request renewal of an expired token.

Maybe it doesn't need to be configurable, though. Let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Good point.
How about a configurable accessTokenDeletionDays that processDatabaseCleanup uses?
An access token would be deleted if (createdAt + expiresIn +accessTokenDeletionDays) < now.

Should the old access token always be deleted (expired or otherwise) by token rotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, right now the old token is deleted during rotation (see #335 / #375)

packages/auth/src/app.ts Outdated Show resolved Hide resolved
packages/auth/src/app.ts Outdated Show resolved Hide resolved
@dclipp dclipp force-pushed the dc-cleanup-expired-2 branch from afceb40 to 4e48b7a Compare August 11, 2022 22:28
Co-authored-by: Brandon Wilson <brandon@coil.com>
@matdehaast matdehaast merged commit ab909da into main Aug 16, 2022
@matdehaast matdehaast deleted the dc-cleanup-expired-2 branch August 16, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: auth Changes in the GNAP auth package. type: source Changes business logic
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Clean up expired tokens after reasonable time
5 participants