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

[SecurityBundle] Set request stateless when firewall is stateless #48044

Conversation

alamirault
Copy link
Contributor

@alamirault alamirault commented Oct 29, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #40372
License MIT
Doc PR symfony/symfony-docs#...

Automatically add _stateless attribute to the request when firewall is stateless

@carsonbot carsonbot added this to the 6.2 milestone Oct 29, 2022
@alamirault alamirault changed the title [SecurityBundle] Add stateless attribute to request when firewall is … [SecurityBundle] Set request stateless when firewall is stateless Oct 29, 2022
@carsonbot
Copy link

Hey!

I think @TimoBakx has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@chalasr
Copy link
Member

chalasr commented Oct 31, 2022

Thanks for the PR.
As it's too late for 6.2, this PR should be rebased on the 6.3 branch (and the changelog entry be moved accordingly) once that branch is created, when 6.2.0 stable is released.

@wouterj wouterj modified the milestones: 6.2, 6.3 Oct 31, 2022
@alamirault alamirault force-pushed the feature/40372-mark-request-stateless-when-firewall-is-stateless branch from bbbc0ee to 6923cb3 Compare November 26, 2022 13:46
@alamirault
Copy link
Contributor Author

I rebased on branch 6.3 😄

@stof
Copy link
Member

stof commented Dec 2, 2022

This looks wrong to me. Something else than the Firewall could still be using the session.

@Seldaek
Copy link
Member

Seldaek commented Dec 2, 2022

@stof Sure, but that's why you want a warning IMO. If the firewall is stateless then you shouldn't rely on the session and opening one will most likely result in data loss or at least inefficiencies in the code as it reads from session storage for nothing.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍
Using the session on a stateless firewall looks like a mistake to me.
This allows spotting it.

@fabpot fabpot force-pushed the feature/40372-mark-request-stateless-when-firewall-is-stateless branch from 6923cb3 to ce458c6 Compare December 18, 2022 10:02
@fabpot
Copy link
Member

fabpot commented Dec 18, 2022

Thank you @alamirault.

@fabpot fabpot merged commit cb6f2c5 into symfony:6.3 Dec 18, 2022
@alamirault alamirault deleted the feature/40372-mark-request-stateless-when-firewall-is-stateless branch December 18, 2022 11:22
@tucksaun
Copy link
Contributor

I tend to agree with @stof here: while it's true that most stateless firewalls are used with stateless routes, this is not systematic and you can have stateless authentication (using JWT cookies or an HTTP middleware adding trusted headers to the request for example) but still require sessions for some specific pages (for CSRF tokens or flash messages for example).

I believe we should at least allow this behavior to be overridable if the current request is already marked as stateless: false.

nicolas-grekas added a commit that referenced this pull request Apr 12, 2023
…bute is not defined (tucksaun)

This PR was merged into the 6.3 branch.

Discussion
----------

[SecurityBundle] Set request stateless only if the attribute is not defined

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes-ish
| New feature?  | no
| Deprecations? | no
| Tickets       | #48044 (comment)
| License       | MIT
| Doc PR        | n/a

The current implementation makes sense for most cases but not for every case as one can have a stateless authentication but still requires sessions.
This PR allows setting the request as non-stateless while having a stateless firewall but keeping the new behavior by default.

Commits
-------

5f29c8d [SecurityBundle] Set request stateless if the attribute is not already defined
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Apr 13, 2023
…ed (alamirault)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

Set request stateless only if the attribute is not defined

symfony/symfony#48044 added in 6.3 was updated in symfony/symfony#49997.

This PR ajust behavior documentation

Commits
-------

20ee4d7 Set request stateless only if the attribute is not defined
@fabpot fabpot mentioned this pull request May 1, 2023
@johannes85
Copy link
Contributor

johannes85 commented Jun 19, 2023

Our software falls into the case described by @tucksaun, so after updateing to 6.3 it breakes our software.

Instead of using the signin methods provided by Symfony, we load the user object from a external server with a session id cookie written by an external SSO software. For this we use a custom, stateless authenticator.

We also use Symfony sessions, but not for authetication, which triggers the "Session was used while the request was declared stateless." exception.

Is there any way to override this behavior? I found his pull request (symfony/symfony-docs#18195) but I didn't find any documentation what to change in order to get the old behavior

@tucksaun
Copy link
Contributor

@johannes85 you can define the request as stateful via the routing:

#config/routes/security.yaml
app_logout:
    path: /logout
    methods: GET
    defaults:
        _stateless: false
// [...]
class DashboardController extends AbstractDashboardController
{
    #[Route('/admin', name: 'admin', stateless: false)]
    public function index(): Response
// [...]

@johannes85
Copy link
Contributor

johannes85 commented Jun 19, 2023

Thank you. I wasn't sure if it is that easy or if I do something "not the symfony way" which will break someting else.

@derrabus
Copy link
Member

I've opened #50715 for further discussion.

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.

[Security] Mark the request as _stateless if the firewall used is stateless