8000 Experimental authenticator integration by scheb · Pull Request #8 · scheb/2fa · GitHub
[go: up one dir, main page]
Skip to content

Experimental authenticator integration #8

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
merged 1 commit into from
Jun 3, 2020
Merged

Experimental authenticator integration #8

merged 1 commit into from
Jun 3, 2020

Conversation

scheb
Copy link
Owner
@scheb scheb commented May 13, 2020

Closes #5

@scheb scheb force-pushed the authenticators branch from 6c77857 to 657d95d Compare May 13, 2020 14:34
scheb
scheb commented May 13, 2020
View reviewed changes
@scheb scheb force-pushed the authenticators branch 2 times, most recently from f332257 to 4aa5c9f Compare May 14, 2020 12:25
@scheb
Copy link
Owner Author
scheb commented May 14, 2020

I need to redirect the user to the 2fa form, while they're in that intermediate "2fa not finished" state. Until now, I've done this in the authentication listener:

2fa/src/bundle/Security/Http/Firewall/TwoFactorListener.php

Lines 173 to 175 in 5d25f88

$this->dispatchTwoFactorAuthenticationEvent(TwoFactorAuthenticationEvents::REQUIRE, $request, $currentToken);
$response = $this->authenticationRequiredHandler->onAuthenticationRequired($request, $currentToken);
$event->setResponse($response);

With authenticators I can't do this anymore, because authenticators can only generate a response when the login was successful/failed and not on every request.

Would it be feasible to have my own kernel.request listener for that, which is called directly after the firewall and generates a redirect response?

@scheb scheb force-pushed the authenticators branch from 4aa5c9f to 34634fd Compare May 15, 2020 16:03
wouterj
wouterj reviewed May 15, 2020
View reviewed changes
Copy link
Contributor
@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Did a very quick scan (I'll test somewhere next week, I'm very busy till tuesday), looks great afaics!

@wouterj
Copy link
Contributor
wouterj commented May 15, 2020

Would it be feasible to have my own kernel.request listener for that, which is called directly after the firewall and generates a redirect response?

From what I understand, this code is executed when all authenticators failed to authenticate the request? Would listening on the new LoginSuccessEvent or LoginFailedEvent (or both?) work?

@scheb
Copy link
Owner Author
scheb commented May 15, 2020

@wouterj Thank you for the feedback, very helpful!

Did a very quick scan (I'll test somewhere next week, I'm very busy till tuesday), looks great afaics!

If you want to play around with it, the repository contains a little test application located in app. Just run composer install and symfony serve in that directory to start it up.

From what I understand, this code is executed when all authenticators failed to authenticate the request? Would listening on the new LoginSuccessEvent or LoginFailedEvent (or both?) work?

No, the code needs to be executed as long as the user is in that intermediate state, when 2fa has not been completed yet. The purpose is to redirect the user to the 2fa form page, to ask for the 2fa code. The events you mentioned would only trigger when an authenticator was executed within the request.

I was playing around with different solutions today and I'll most likely add a listener for kernel.request to detect the "2fa incomplete" state and redirect the user. Alternatively, I'll investigate to add my own AccessListener-like implementation, which would handle the "2fa not completed" state.

I can give you an update as soon as I have something running that I believe is feasible.

@wouterj
Copy link
Contributor
wouterj commented May 16, 2020 •
edited
Loading

Ah, please note that the firewall listeners (like AccessListener and ContextListener) have not been removed. So I can also think of a solution where we create a new ListenerFactoryInterface in Symfony that would allow custom factories to create firewall listeners.

That would allow you to reuse the lazy firewall logic, which I think would make sense.

@scheb
Copy link
Owner Author
scheb commented May 16, 2020

Yea, would be cool to have an easier way to inject a listener into the firewall. Having a ListenerFactoryInterface sounds like a good solution to me. For now I'll probably go for a compiler pass, to add it to the list of listeners. I have some unfinished code laying around, but I need to do some other refactorings first.

This whole authenticator experiment uncovered some bad design choices in my bundle and I figured there's better ways to do it. So I'm working on that at the moment ;)

@scheb scheb force-pushed the master branch 4 times, most recently from 6c69228 to cf00de5 Compare May 17, 2020 19:07
@scheb scheb force-pushed the authenticators branch from bcb16df to 60a317c Compare May 18, 2020 18:18
@scheb scheb force-pushed the authenticators branch 2 times, most recently from faafc2a to 0e9a73f Compare May 29, 2020 13:41
@scheb scheb changed the title [WIP] Authenticator integration proof of concept Experimental authenticator integration May 30, 2020
@scheb scheb force-pushed the authenticators branch 5 times, most recently from 31f6631 to 38c9e99 Compare May 30, 2020 20:10
@wouterj wouterj mentioned this pull request May 31, 2020
Merged
wouterj
wouterj reviewed May 31, 2020
View reviewed changes
Copy link
Contributor
@wouterj wouterj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just tested it in an application (sorry for the delay) and did a quick scan over the code.

I found one bug while testing: The new system has removed AnonymousToken (anonymous sessions now have no token). In order to whitelist pages to allow no token, a new special PUBLIC_ACCESS attribute (AccessListener::PUBLIC_ACCESS) was introduced.

This attribute is special: it is handled directly in Symfony's AccessListener and not the access decision manager (it can only be used in access_control).

The TwoFactorAccessDecider needs a small update to also take this attribute into account as accessible. See wouterj@40699ef for a diff (feel free to copy the patch directly - I don't really care about attribution for this).
In case you want to test yourself, this is the diff I had to apply to test it in your great sample app: wouterj@749d046 (you also need the patch from symfony/symfony#37031, I just found this bug while testing this bundle)

fabpot added a commit to symfony/symfony that referenced this pull request Jun 1, 2020
@fabpot
bug #37031 [Security] Fixed PUBLIC_ACCESS in authenticated sessions (…
e5b5d9e 
…wouterj)

This PR was merged into the 5.1 branch.

Discussion
----------

[Security] Fixed PUBLIC_ACCESS in authenticated sessions

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Found while testing scheb/2fa#8, sorry for not spotting it before the stable release 😞

Currently, authenticated users are denied access for pages that have `PUBLIC_ACCESS` set, as this attribute is only checked when no token was set. It should be checked for both cases.

Commits
-------

0ac530f Also check PUBLIC_ACCESS for authenticated tokens
@scheb
Copy link
Owner Author
scheb commented Jun 1, 2020

Thanks for looking into it. As you can see from the many force-pushes, I'm still messing around a lot in the code, but I consider the implementation now relatively stable.

Good that you mentioned PUBLIC_ACCESS, I totally forgot about this new attribute. Had a look at wouterj@40699ef and this looks good to me. Now I'm glad that I didn't remove that class, something I was thinking about recently 😆 I'd also need to backport the change to scheb/two-factor-bundle for full compatibility with Symfony 5.1.

@scheb scheb force-pushed the authenticators branch from 38c9e99 to 6ce21f8 Compare June 2, 2020 22:32
@scheb scheb force-pushed the authenticators branch from 6ce21f8 to aca9986 Compare June 2, 2020 22:38
@scheb
Copy link
Owner Author
scheb commented Jun 2, 2020

Almost done, some test cases are still missing :)

@scheb scheb force-pushed the authenticators branch from aca9986 to 120e835 Compare June 3, 2020 14:36
@scheb scheb force-pushed the authenticators branch from 120e835 to d79a5c7 Compare June 3, 2020 14:40
@scheb scheb merged commit 4af52b2 into master Jun 3, 2020
@scheb scheb deleted the authenticators branch June 3, 2020 15:55
@wouterj
Copy link
Contributor
wouterj commented Jun 3, 2020

Very cool to see this merged. 🎈 Thanks for your hard work! It's one of the first major security bundles with support in their master branch

@scheb
Copy link
Owner Author
scheb commented Jun 3, 2020

Thanks for your help to make this work! I'll release a v5 with authenticators support soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wouterj wouterj wouterj left review comments

@Spomky Spomky Spomky left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate with new "Authenticators" security
3 participants
@scheb @wouterj @Spomky
0