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(server): remove inactive sessions #9121

Merged
merged 2 commits into from
Apr 27, 2024
Merged

Conversation

jrasm91
Copy link
Contributor

@jrasm91 jrasm91 commented Apr 27, 2024

  • Remove session tokens that haven't been used in 90 days.
  • Fix an issue with make sql

Copy link

cloudflare-pages bot commented Apr 27, 2024

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: 464cf76
Status: ✅  Deploy successful!
Preview URL: https://0bc2cf09.immich.pages.dev
Branch Preview URL: https://feat-remove-inactive-session.immich.pages.dev

View logs

@@ -26,6 +27,14 @@ describe('SessionService', () => {
expect(sut).toBeDefined();
});

describe('search', () => {
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 test deleting outdated tokens and preserving recent tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, had to run though and that's all I finished on the first pass lol

Copy link
Member

Choose a reason for hiding this comment

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

All good. Should this PR still be draft then? 😛

Copy link
Member

Choose a reason for hiding this comment

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

Nope I'm on it :)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally I'd also like to have e2e tests for testing the repository method but since this is a nightly job that's a bit tricky I think.

@@ -19,6 +21,25 @@ export class SessionService {
this.access = AccessCore.create(accessRepository);
}

async handleCleanup() {
const sessions = await this.sessionRepository.search({
updatedBefore: DateTime.now().minus({ days: 90 }).toJSDate(),
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to make the timeout 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.

Maybe?

Copy link
Member

Choose a reason for hiding this comment

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

@bo0tzz where would you put that in the admin settings? User Settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We maybe should move password and auth and oauth into a parent auth category in the UI.

Copy link
Member

Choose a reason for hiding this comment

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

An overarching auth section is probably the cleanest, but under server or user settings would work for me as well.

Copy link
Member

Choose a reason for hiding this comment

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

Moving the auth/oauth settings around and adding the delay for deleting sessions will be its own PR

@jrasm91 jrasm91 merged commit 034c928 into main Apr 27, 2024
23 checks passed
@jrasm91 jrasm91 deleted the feat/remove-inactive-sessions branch April 27, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants