Skip to content

[Security] Track session usage whenever a new token is set #36335

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

Jump to bottom
Merged
fabpot merged 1 commit into from
Apr 4, 2020
Merged

[Security] Track session usage whenever a new token is set #36335

fabpot merged 1 commit into from
Apr 4, 2020

Conversation

@wouterj
Copy link
Member

@wouterj wouterj commented Apr 3, 2020 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36208
License MIT
Doc PR -

When using anonymous: lazy, the programatic login using the guard handler is broken. As the setToken() does not track usage, the index remains equal.

I tried fixing this more properly in e.g. the SessionStrategy::onAuthentication class, but I couldn't get it working (as $request->hasPreviousSession() returns false, the session strategy isn't called). setToken() can also not be made usage tracking afaics, because it would directly break (setToken(null) is called in ContextListener).

The current fix does however look really ugly, but I can't find anything better with my minor knowledge of this session usage tracking feature. I'm open for all ideas :)

@wouterj wouterj force-pushed the bug-36208/lazy-guard-manual-login branch 2 times, most recently from a7eb8fc to c1e0d9e Compare April 3, 2020 14:12
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Apr 3, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 3, 2020 •
edited
Loading

setToken(null) is called in ContextListener

Can't we make setToken increment the tracker when $token is not null?

@wouterj
Copy link
Member Author

wouterj commented Apr 3, 2020 •
edited
Loading

Can't we make setToken increment the tracker when $token is not null?

I just tried this (as it sounds like a nice solution), but without calling getToken() the TokenStorage::initializer() isn't called. This means ContextListener isn't called and thus usage tracking is not enabled in the UsageTrackingTokenStorage. So this can be fixed by also injecting the sessionTrackerEnabler in UsageTrackingTokenStorage. Is that preferred? (it seems a bit off, as it's then getting a closure injected call its own method)

Allowing setToken() to call the initializer before setting the token would completely remove the lazy feature (as setToken(null) is always called).

@wouterj wouterj force-pushed the bug-36208/lazy-guard-manual-login branch from c1e0d9e to fed965a Compare April 3, 2020 17:04
@wouterj wouterj changed the title [Security][Guard] Force incrementing session usage index upon programatic login [Security] Track session usage whenever a new token is set Apr 3, 2020
@wouterj wouterj force-pushed the bug-36208/lazy-guard-manual-login branch 2 times, most recently from 20b8804 to d09adf8 Compare April 3, 2020 17:41
@wouterj wouterj force-pushed the bug-36208/lazy-guard-manual-login branch from d09adf8 to 8d96dbd Compare April 3, 2020 17:47
@fabpot
Copy link
Member

fabpot commented Apr 4, 2020

Thank you @wouterj.

@fabpot fabpot merged commit 38cbcc6 into symfony:4.4 Apr 4, 2020
@wouterj wouterj deleted the bug-36208/lazy-guard-manual-login branch April 15, 2020 16:34
This was referenced Apr 28, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees

No one assigned

Labels

Bug Security Status: Reviewed

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@wouterj @nicolas-grekas @fabpot @carsonbot