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

Set login constants in request listener #4968

Merged
merged 10 commits into from Aug 11, 2022

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Jul 8, 2022

Fixes #2576 and other issues.

In #2628 we removed the usage of the deprecated FE_USER_LOGGED_IN constant within the core and switched to using the token checker instead, in order to fix #2576.

However, the core issue still remains: for any Contao extension that still uses the FE_USER_LOGGED_IN constant, the constant will still have the wrong value for any authenticated request without a previous authenticated session. For example: if you implement a custom guard authenticator that authenticates a Contao user e.g. via an Authorization header, or some other custom solution, the constant will always be false (if no session cookie of a previous authenticated request is passed along with the request).

The cause of this is the setting of the constants within ContaoFramework::initialize. ContaoFramework::initialize will be executed very early - earlier than the Firewall listener, which is a regular request listener. So since $this->tokenChecker->hasFrontendUser() will be executed before Symfony's Firewall listener, it will always be false, because no security token has been set by the Symfony Firewall yet.

To fix this we must not query the TokenChecker before the Symfony Firewall listener. i.e. we need to set this constant in a regular request listener that has a lower priority than the Firewall request listener of Symfony. This is implemented in this PR.

@fritzmg fritzmg added the bug label Jul 8, 2022
@fritzmg fritzmg added this to the 4.9 milestone Jul 8, 2022
@fritzmg fritzmg requested a review from a team July 8, 2022 12:36
@fritzmg fritzmg self-assigned this Jul 8, 2022
@aschempp
Copy link
Member

aschempp commented Jul 9, 2022

I think there are two things to consider/fix here:

  1. The listener should not set the constants if the framework was not initialized
  2. if the framework is initialized after the firewall (e.g. if a Symfony route matches), we must still set the constants when booting the framework.

Otherwise 👍 for using a listener after firewall!

@fritzmg fritzmg marked this pull request as draft July 9, 2022 12:15
@fritzmg fritzmg marked this pull request as ready for review July 11, 2022 16:11
@fritzmg fritzmg requested a review from aschempp July 12, 2022 09:12
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

I would change two things here

  1. always use getCurrentRequest for the constants to keep it consistent
  2. I don't think we need two methods. Calling setLoginConstants() when the framework is not initialised should set the internal flag. Calling the method (again) if the framework is initialised (internally) should set the constants automatically.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 17, 2022

always use getCurrentRequest for the constants to keep it consistent

Are you sure? Should we not pass the request of the request event?

@fritzmg fritzmg force-pushed the set-login-constants-in-request-listener branch from 83fd688 to 7d72d58 Compare July 17, 2022 15:04
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

Not sure about that request, but it probably does not matter. But why did you remove the mode-check and the session initialization?

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 18, 2022

But why did you remove the mode-check

It does not really make sense to check whether this is a front end request or not via $this->getMode()? Ah I see, because of the $isFrontend variable.

and the session initialization?

I don't understand the purpose of $session->start() here and I do not think it is needed.

Also the previous condition had $this->request->hasPreviousSession() - which also does not make sense. This was likely a workaround, as the Contao framework is initialized early before the firewall listener and thus this bit of code never worked for the initial authentication request.

@aschempp
Copy link
Member

Also the previous condition had $this->request->hasPreviousSession() - which also does not make sense. This was likely a workaround, as the Contao framework is initialized early before the firewall listener and thus this bit of code never worked for the initial authentication request.

It made sense because without a previous session you could never be logged in – since a security token would have been stored in the DB. But I agree if the constant is only set after the firewall this should never be necessary.

@leofeyer leofeyer merged commit 0af6da8 into contao:4.9 Aug 11, 2022
@leofeyer
Copy link
Member

Thank you @fritzmg.

fritzmg added a commit to fritzmg/contao that referenced this pull request Aug 12, 2022
Description
-----------

-

Commits
-------

829a207 set login constants in request listener
cc98d05 update tests
6975586 only set constants if framework was initialized
3a5bb25 code style
df1a2b5 expect deprecation
7abe0f2 English please
7d72d58 simplify implementation
98a490d use getMode
d60c2cb Merge remote-tracking branch 'origin/4.9' into set-login-constants-in…
leofeyer pushed a commit that referenced this pull request Aug 16, 2022
Description
-----------

-

Commits
-------

43edcc8 Set login constants in request listener (see #4968)
04d66c7 adjust tests and ContaoFramework::initialize for 4.13
2657d47 check for main request
f3eae56 sort services
361a15b whoopsie
3a071ad check login constants
@fritzmg fritzmg deleted the set-login-constants-in-request-listener branch December 14, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants