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

Rebuild the login process #1178

Merged
merged 2 commits into from Jan 14, 2020
Merged

Rebuild the login process #1178

merged 2 commits into from Jan 14, 2020

Conversation

@aschempp
Copy link
Contributor

aschempp commented Jan 13, 2020

This extends #1130

Instead of implementing the loginAttemps check in multiple places, and based on things I learned while implementing #1118, I have implemented 2FA directly inside the ContaoLoginFactory. This means whatever we need is "hardcoded" and always works in the Contao environment.

Most of the code in the two-factor bundle is not needed for our case, and I think it makes the whole setup easier. It's now easy to "see" that the AuthenticationProvider is responsible for starting 2FA after successful username/password login.

I found some other issues on the run, like that the frontend login module did not correctly redirect after 2FA authentication.

Merging this will affect #719 and and #559 but it should not make them more complicated, because we can still use everything that was created design-wise and just replace the configuration with hardcoded stuff (like configuring the backup code manager).

@aschempp aschempp added the feature label Jan 13, 2020
@aschempp aschempp added this to the 4.9 milestone Jan 13, 2020
@aschempp aschempp requested review from ausi, Toflar, bytehead and leofeyer Jan 13, 2020
@aschempp aschempp self-assigned this Jan 13, 2020
Copy link
Member

leofeyer left a comment

The PR looks good to me (except CS, but I'll happily handle that). I like how if further simplifies the configuration. 👍

@richardhj

This comment has been minimized.

Copy link
Member

richardhj commented Jan 13, 2020

I found some other issues on the run, like that the frontend login module did not correctly redirect after 2FA authentication.

Related: #967.

Copy link
Member

leofeyer left a comment

After three failed login attempts, the user is now locked for 15 seconds. Shouldn't the locking start at 5 seconds? Also, shouldn't the locking start after the second failed attempt?

  • $user->loginAttempts is 0
  • The user submits wrong credentials -> locked for 0 * 5 seconds
  • $user->loginAttempts is increased to 1
  • The user submits wrong credentials again -> locked for 1 * 5 seconds
  • $user->loginAttempts is increased to 2
  • and so on
@aschempp

This comment has been minimized.

Copy link
Contributor Author

aschempp commented Jan 14, 2020

After three failed login attempts, the user is now locked for 15 seconds. Shouldn't the locking start at 5 seconds? Also, shouldn't the locking start after the second failed attempt?

I discussed this with @Toflar and we agreed that would not make sense. I don't wanna be locked for 5 seconds on the first invalid attempt, if my 2FA code expired for a second. We previously allowed three invalid attempts before (starting to) lock, so I implemented the same approach here.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2020

You are not locked upon the first invalid attempt but on the second one (see my comment above).

We previously allowed three invalid attempts before

Agreed. Still then the locking should start at 5 seconds and not at 15 seconds, shouldn't it?

@aschempp

This comment has been minimized.

Copy link
Contributor Author

aschempp commented Jan 14, 2020

Agreed. Still then the locking should start at 5 seconds and not at 15 seconds, shouldn't it?

Well you had three attempts… who cares if it's 15sec or 5sec after that? 5sec locking does not make much sense, the lock might be gone until the page is actually loaded. I think 15sec is fine, and its easier to calculate 😂

@aschempp aschempp requested a review from leofeyer Jan 14, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2020

Hm, I would really prefer to stick to the RFC.

e.g., say T = 5, then after 1 attempt, the server waits for 5 seconds, at the second failed attempt, it waits for 5*2 = 10 seconds, etc.

In our case, the user would start getting delayed after the second unsuccessful login attempt.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

aschempp commented Jan 14, 2020

Hm, I would really prefer to stick to the RFC.
In our case, the user would start getting delayed after the second unsuccessful login attempt.

/cc @contao/developers

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 14, 2020

I'm in favour of the version this PR implements. So do not delay until the 3rd try. It's not user friendly at all otherwise. Especially for the 2FA which happens to me quite often, I don't want to wait 5 seconds for my second try.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2020

You would have to wait 5 seconds for your third try, not your second!

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 14, 2020

I'd be fine with that then.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

aschempp commented Jan 14, 2020

You would have to wait 5 seconds for your third try, not your second!

e.g., say T = 5, then after 1 attempt, the server waits for 5 seconds, at the second failed attempt, it waits for 5*2 = 10 seconds, etc.

Not according to the RFC …

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 14, 2020

The RFC provides an example. It's not a MUST. It's perfectly fine to implement our own version.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2020

It is true that the RFC suggests locking after the first failed attempt, however, the way we have implemented it, it would start after the second one (see my comment above).

  • $user->loginAttempts is 0
  • The user submits wrong credentials -> locked for 0 * 5 seconds
  • $user->loginAttempts is increased to 1
  • The user submits wrong credentials again -> locked for 1 * 5 seconds
  • $user->loginAttempts is increased to 2
  • and so on
@aschempp

This comment has been minimized.

Copy link
Contributor Author

aschempp commented Jan 14, 2020

Obviously, any attempts and locking scheme works. I just think locking after 3 attempts makes more sense (also because it has been that way in Contao and the install tool forever), and locking for 5 secs does not make sense, as the lock is already obsolete when the page is loaded…

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2020

Of course it makes sense. An attacker would not wait for the page to reload – they would send many parallel requests. 😄

@aschempp

This comment has been minimized.

Copy link
Contributor Author

aschempp commented Jan 14, 2020

Yeah, so their parallel request is locked for 15 secs instead of 5 😂 🤷‍♂

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 14, 2020

Ok, there are two variants which I can both live with:

  1. Follow the RFC and delay for $user->loginAttempts * 5 seconds. This will start delaying the user after their second failed attempt, because $user->loginAttempts is initially 0.

  2. Allow three failed attempts before applying the delaying and start with 5 seconds after the third failed attempt; something like ($user->loginAttempts - 2) * 5.

But please do not mix the two by starting to increase the delaying after the first attempt when ignoring the first three attempts at the same time.

@aschempp aschempp force-pushed the feature/rebuild-login-process branch from 5e1dd5f to 9e07341 Jan 14, 2020
@aschempp aschempp force-pushed the feature/rebuild-login-process branch from 9e07341 to cf59408 Jan 14, 2020
@Toflar
Toflar approved these changes Jan 14, 2020
@leofeyer leofeyer merged commit 2c30e5d into master Jan 14, 2020
9 checks passed
9 checks passed
Coverage
Details
Coding Style
Details
PHP 7.2
Details
PHP 7.3
Details
PHP 7.4
Details
Prefer Lowest
Details
Bundles
Details
Windows
Details
codecov/project 89.63% (-0.1%) compared to 735df94
Details
@leofeyer leofeyer deleted the feature/rebuild-login-process branch Jan 14, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 15, 2020

Thank you @bytehead and @aschempp.

@bytehead

This comment has been minimized.

Copy link
Member

bytehead commented Jan 15, 2020

Bildschirmfoto 2020-01-15 um 15 40 30

In case of a wrong submitted 2FA code, the message should be different, right?

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Jan 15, 2020

There's already an issue for that: #1186

@bytehead

This comment has been minimized.

Copy link
Member

bytehead commented Jan 15, 2020

ups 🙈 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.