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

Add backup code functionality for 2FA #719

Closed
wants to merge 23 commits into from

Conversation

@bytehead
Copy link
Member

bytehead commented Sep 3, 2019

See #265 (comment)

Recovery codes for backend users:
Bildschirmfoto 2019-09-03 um 22 57 01

Recovery codes for frontend users:
Bildschirmfoto 2019-09-03 um 22 57 09

To-Do:

  • Tests
  • Check IS_AUTHENTICATED_FULLY if a user wants to see the backup codes
@bytehead bytehead changed the title [POC] Add backup code functionality for 2FA [RFC] Add backup code functionality for 2FA Sep 3, 2019
@bytehead bytehead marked this pull request as ready for review Sep 3, 2019
@bytehead bytehead self-assigned this Sep 3, 2019
@bytehead bytehead added the feature label Sep 3, 2019
@bytehead bytehead added this to the 4.9 milestone Sep 3, 2019
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Sep 3, 2019

Check IS_AUTHENTICATED_FULLY if a user wants to see the backup codes

We should also check this if a user wants to change their password or close their account. This implies that the login module can handle this case.

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Sep 5, 2019

We should also check this if a user wants to change their password or close their account. This implies that the login module can handle this case.

I think that's not related to this PR. We should open another PR to fix this everywhere. I already have some ideas, but I'm not sure it's gonna work 🤷‍♂

@bytehead bytehead force-pushed the bytehead:feature/2fa-backup-codes branch from b418f70 to 0103f50 Sep 6, 2019
@leofeyer leofeyer force-pushed the contao:master branch from 2ea6ebd to 91d73a0 Sep 26, 2019
@leofeyer leofeyer mentioned this pull request Nov 11, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-backup-codes branch 2 times, most recently from 3ca04d6 to b61d518 Nov 30, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-backup-codes branch from b61d518 to 900962a Dec 7, 2019
@bytehead bytehead changed the title [RFC] Add backup code functionality for 2FA Add backup code functionality for 2FA Dec 8, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-backup-codes branch from 177ebe7 to 4e4db47 Dec 9, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-backup-codes branch 2 times, most recently from 3700268 to 3eb4f42 Dec 23, 2019
@bytehead bytehead force-pushed the bytehead:feature/2fa-backup-codes branch from 3eb4f42 to cc876b2 Dec 30, 2019
bytehead added 11 commits Jul 17, 2019
CS
@leofeyer leofeyer requested review from aschempp, ausi and Toflar Jan 7, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 8, 2020

I have created a PR with my suggested changes: bytehead#1

@bytehead bytehead requested a review from leofeyer Jan 8, 2020
@bytehead bytehead dismissed leofeyer’s stale review Jan 8, 2020

All change requests are resolved.

bytehead and others added 2 commits Jan 8, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 9, 2020

@bytehead My latest commit 0cc68f5 adjusts the wording so the button says "generate backup codes" if there are none, "show backup codes" if there are codes and "regenerate backup codes" to generate new codes. Do you agree?

@bytehead

This comment has been minimized.

Copy link
Member Author

bytehead commented Jan 9, 2020

Perfect!

public function isBackupCode(string $code): bool
{
return \in_array($code, json_decode($this->backupCodes, true));
}

/**
* {@inheritdoc}
*/
public function invalidateBackupCode(string $code): void
{
$backupCodes = json_decode($this->backupCodes, true);
$key = array_search($code, $backupCodes);

if ($key !== false)
{
unset($backupCodes[$key]);
$this->backupCodes = json_encode($backupCodes);
}
}
Comment on lines +638 to +656

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 9, 2020

Contributor

I don't really like extending that functionality in the user class. Can't the logic be in the BackupCodeManager?

This comment has been minimized.

This comment has been minimized.

Copy link
@bytehead

bytehead Jan 9, 2020

Author Member

In fact that we use our own backup code manager, it's probably possible to use empty method stubs on the user class instead...

if (!$this->get('security.helper')->isGranted('IS_AUTHENTICATED_FULLY')) {
throw new InsufficientAuthenticationException('User is not fully authenticated');
}
Comment on lines +45 to +47

This comment has been minimized.

Copy link
@aschempp

aschempp Jan 9, 2020

Contributor

Wouldn't this mean the module generates an exception if you open the page with a REMEMBERME cookie?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jan 9, 2020

Member

Unfortunately yes. And since we have no "re-enter your password" screen yet, the user will be redirected to the login module, which will happily tell them that they are already logged in. 😢

@leofeyer leofeyer dismissed stale reviews from Toflar and themself via a22ba09 Jan 9, 2020
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 15, 2020

Closed in favor of #1184.

@leofeyer leofeyer closed this Jan 15, 2020
@leofeyer leofeyer removed this from the 4.9 milestone Jan 15, 2020
@bytehead bytehead deleted the bytehead:feature/2fa-backup-codes branch Jan 15, 2020
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.