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

Check if token has proper roles #1082

Merged
merged 9 commits into from Feb 11, 2020
Merged

Conversation

@bytehead
Copy link
Member

bytehead commented Dec 10, 2019

This fixes the issue that protected elements will be shown direct after successful login but required and not yet fullfilled 2FA authentication.

To-Do:

  • Adjust tests
@bytehead bytehead self-assigned this Dec 10, 2019
@bytehead bytehead added the defect label Dec 10, 2019
@bytehead bytehead added this to the 4.8 milestone Dec 10, 2019
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 10, 2019

Nice. 👍

@aschempp aschempp self-requested a review Dec 10, 2019
Copy link
Contributor

aschempp left a comment

Instead of checking the token instance, we should check for the roles. A 2FA token does not have any roles (https://github.com/scheb/two-factor-bundle/blob/master/Security/Authentication/Token/TwoFactorToken.php#L60-L63), which is the correct implementation to prevent security access.

We shouldn't event validate the user… just something like

$token = $this->getToken(self::FRONTEND_FIREWALL);

return null !== $token && \in_array('ROLE_MEMBER', $token->getRoles();
@bytehead bytehead force-pushed the bytehead:bugfix/2fa-token-checker branch 2 times, most recently from 66ccb9f to bbb0d6f Dec 10, 2019
@bytehead bytehead requested a review from aschempp Dec 10, 2019
@bytehead bytehead requested a review from leofeyer Dec 10, 2019
@bytehead bytehead changed the title Check if token is instance of TwoFactorTokenInterface Check if token has proper roles Dec 10, 2019
@bytehead bytehead closed this Jan 15, 2020
@bytehead bytehead deleted the bytehead:bugfix/2fa-token-checker branch Jan 15, 2020
@leofeyer leofeyer removed this from the 4.8 milestone Jan 15, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 15, 2020

Why did you close the PR?

@bytehead bytehead restored the bytehead:bugfix/2fa-token-checker branch Jan 15, 2020
@bytehead bytehead reopened this Jan 15, 2020
@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 15, 2020

Sorry, that wasn't intentional.

@leofeyer leofeyer added this to the 4.8 milestone Jan 15, 2020
@bytehead bytehead changed the base branch from 4.8 to 4.9 Feb 5, 2020
@bytehead bytehead modified the milestones: 4.8, 4.9 Feb 5, 2020
@leofeyer leofeyer requested a review from aschempp Feb 7, 2020
@bytehead bytehead force-pushed the bytehead:bugfix/2fa-token-checker branch from 5e386bf to 5dd66be Feb 10, 2020
@bytehead bytehead changed the base branch from 4.9 to 4.8 Feb 10, 2020
@bytehead bytehead modified the milestones: 4.9, 4.8 Feb 10, 2020
bytehead added 7 commits Dec 10, 2019
@bytehead bytehead force-pushed the bytehead:bugfix/2fa-token-checker branch from 5dd66be to b70c2ba Feb 10, 2020
@contao contao deleted a comment from codecov bot Feb 10, 2020
@contao contao deleted a comment from codecov bot Feb 10, 2020
@contao contao deleted a comment from codecov bot Feb 10, 2020
@bytehead bytehead requested a review from leofeyer Feb 10, 2020
@leofeyer leofeyer requested a review from aschempp Feb 11, 2020
@leofeyer leofeyer merged commit 7824079 into contao:4.8 Feb 11, 2020
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.1
Details
PHP 7.2
Details
PHP 7.3
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 88.74% remains the same compared to b70c2ba
Details
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 11, 2020

Thank you @bytehead.

@bytehead bytehead deleted the bytehead:bugfix/2fa-token-checker branch Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.