8000 [HttpFoundation] Regression in using the session in kernel.terminate · Issue #40778 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Regression in using the session in kernel.terminate #40778

New issue

Have a question about this project?< 8000 /strong> 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
Closed
wouterj opened this issue Apr 12, 2021 · 2 comments · Fixed by #40785
Closed

[HttpFoundation] Regression in using the session in kernel.terminate #40778

wouterj opened this issue Apr 12, 2021 · 2 comments · Fixed by #40785
Labels
Bug HttpFoundation Status: Needs Review

Comments

@wouterj
Copy link
Member
wouterj commented Apr 12, 2021 •
edited
Loading

Symfony version(s) affected: 5.x

Description

I was testing one of our main apps in 5.x and discovered a regression with the new session logic when using the token storage, causing "There is currently no session available." (related change: #38616)

We use a listener on kernel.terminate to log some information (including the current username). However, RequestStack::getSession() only works when there is a main request (which is not the case in the kernel.terminate event). Before 5.3, the session service was usable outside a main request.
The fix on our side was quite easy: use the security.untracked_token_storage service. Yet, I do believe there should be no hard breaks

How to reproduce

  1. Create a listener on kernel.terminate
  2. Try to use the session

Possible Solution

Maybe store the main $request->getSession() (in RequestStack::push()) in a variable and use it when there is no main request anymore? (and immediately deprecate this behavior) This way, there is no hard break between the version upgrade and there is a way to direct the user into the right debugging path.
@wouterj wouterj added the Bug label Apr 12, 2021
@wouterj wouterj changed the title [HttpFoundation] Regression in using the UsageTrackingTokenStorage in kernel.terminate [HttpFoundation] Regression in using the session in kernel.terminate Apr 12, 2021
@jderusse
Copy link
Member

The kernel.terminate event is dispatched after finishRequest which removes the request from the RequestStack.

If I'm not wrong, there is no BC-Break here: people that use the session service will still get the session from the container (with a deprecation now). This is "just" about people migrating from session service to the new RequestStack->getSession

I'm 👎 for adding the lat previous request' session in the requestStack. This would defeat the the point of removing the state from services.

People willing to fixing the deprecation should stop using state from service, including session, including session from an empty RequestStack.
Instead, I suggesst to use the Request object in the KernelTerminate event.

@wouterj
Copy link
Member Author
wouterj commented Apr 12, 2021

@jderusse then we should fix this in the UsageTrackingTokenStorage:

symfony/src/Symfony/Component/Security/Core/Authentication/Token/Storage/UsageTrackingTokenStorage.php

Line 89 in 9092d5b

return $this->container->get('request_stack')->getSession();

We're not using the session directly in our listeners, the token storage is. If I understand you correctly, you propose to fix the usage tracking token storage to not use the session if there is no main requests (anymore)?

@wouterj wouterj mentioned this issue Apr 12, 2021
Merged
@fabpot fabpot closed this as completed Apr 13, 2021
fabpot added a commit that referenced this issue Apr 13, 2021
@fabpot
feature #40785 [Security] Deprecate using UsageTrackingTokenStorage o…
64cc548 
…utside the request-response cycle (wouterj)

This PR was merged into the 5.3-dev branch.

Discussion
----------

[Security] Deprecate using UsageTrackingTokenStorage outside the request-response cycle

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | yes
| Tickets       | Fix #40778
| License       | MIT
| Doc PR        | -

Currently, you get an "There is currently no session available" exception when using the `security.token_storage` service outside the main request-response cycle (e.g. in a `kernel.terminate` listener). This PR deprecates such usage and requires developers to update their definitions to explicitly use `security.untracked_token_storage` instead.

A different solution would be to silently disable tracking in these cases, but I think that might create some unnecessary technical debt.

Commits
-------

7452476 [Security] Fix UsageTrackingTokenStorage outside the request cycle
@thewalkingcoder thewalkingcoder mentioned this issue Jun 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpFoundation Status: Needs Review
Projects
None yet
Milestone
No milestone
4 participants
@fabpot @jderusse @wouterj @carsonbot
0