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

Webauthn login support #1376

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

Spomky
Copy link

@Spomky Spomky commented May 12, 2023

This PR aims at adding Webauthn login.
Webauthn is a web standard that allows the use of strong public key-based credentials for user authentication.
It is proposed in reaction of the recent issue where authentication failure is a key point for such attacks.

  • Add an immutable ID for each user
  • Allow users to manage the authenticators
    • Menu link
    • Icon
    • Add a new one
    • Remove an existing one
    • Custom name
  • Login (popup)
    • Make it work
    • Better UI
  • Login (page)
    • Make it work
    • Better UI

@Spomky Spomky marked this pull request as draft May 12, 2023 18:52
@private-packagist
Copy link

private-packagist bot commented May 12, 2023

composer.lock

Click to show 126 changes in this composer.lock file

Package changes

Package Operation From To Changes
brick/math add - 0.11.0 view code
nyholm/psr7 add - 1.8.0 view code
phpstan/phpdoc-parser add - 1.21.0 view code
spomky-labs/cbor-bundle add - v3.0.0 view code
spomky-labs/cbor-php add - 3.0.2 view code
spomky-labs/pki-framework add - 1.1.0 view code
symfony/psr-http-message-bridge add - v2.2.0 view code
web-auth/cose-lib add - 4.2.0 view code
web-auth/metadata-service add - 4.5.2 view code
web-auth/webauthn-lib add - 4.5.2 view code
web-auth/webauthn-symfony-bundle add - 4.5.2 view code
web-token/jwt-core add - 3.2.7 view code
web-token/jwt-signature add - 3.2.7 view code
composer/composer upgrade 2.5.x-dev 50cded3 2.6.x-dev 6111ff5 diff
dasprid/enum upgrade 1.0.3 1.0.4 diff
doctrine/dbal upgrade 3.5.3 3.6.2 diff
doctrine/doctrine-bundle upgrade 2.8.2 2.9.1 diff
doctrine/instantiator upgrade 1.5.0 2.0.0 diff
doctrine/orm upgrade 2.14.1 2.15.1 diff
doctrine/persistence upgrade 3.1.4 3.2.0 diff
endroid/qr-code upgrade 4.7.0 4.8.2 diff
friendsofphp/proxy-manager-lts upgrade v1.0.14 v1.0.15 diff
google/recaptcha upgrade 1.2.4 1.3.0 diff
guzzlehttp/guzzle upgrade 7.5.0 7.6.1 diff
guzzlehttp/psr7 upgrade 2.4.3 ⚠️ 2.5.0 ✅ diff
knplabs/knp-menu upgrade v3.3.0 v3.4.0 diff
knpuniversity/oauth2-client-bundle upgrade v2.13.1 v2.15.0 diff
laminas/laminas-code upgrade 4.8.0 4.11.0 diff
laminas/laminas-stdlib upgrade 3.16.1 3.17.0 diff
lcobucci/jwt upgrade 4.3.0 5.0.0 diff
league/oauth2-client upgrade 2.6.1 2.7.0 diff
monolog/monolog upgrade 3.2.0 3.3.1 diff
nelmio/cors-bundle upgrade 2.2.0 2.3.1 diff
pagerfanta/core upgrade v3.7.0 v3.8.0 diff
pagerfanta/doctrine-orm-adapter upgrade v3.7.0 v3.8.0 diff
pagerfanta/twig upgrade v3.7.0 v3.8.0 diff
phpdocumentor/type-resolver upgrade 1.6.2 1.7.1 diff
predis/predis upgrade v2.1.1 v2.1.2 diff
psr/http-client upgrade 1.0.1 1.0.2 diff
psr/http-factory upgrade 1.0.1 1.0.2 diff
psr/http-message upgrade 1.0.1 1.1 diff
react/promise upgrade v2.9.0 v2.10.0 diff
scheb/2fa-backup-code upgrade v6.6.0 v6.8.0 diff
scheb/2fa-bundle upgrade v6.6.0 v6.8.0 diff
scheb/2fa-totp upgrade v6.6.0 v6.8.0 diff
scheb/2fa-trusted-device upgrade v6.6.0 v6.8.0 diff
seld/jsonlint upgrade 1.9.0 1.10.0 diff
spomky-labs/otphp upgrade 11.1.0 11.2.0 diff
symfony/asset upgrade v6.2.5 v6.2.7 diff
symfony/cache upgrade v6.2.5 v6.2.10 diff
symfony/cache-contracts upgrade v3.2.0 v3.2.1 diff
symfony/config upgrade v6.2.5 v6.2.7 diff
symfony/console upgrade v6.2.5 v6.2.10 diff
symfony/dependency-injection upgrade v6.2.6 v6.2.10 diff
symfony/deprecation-contracts upgrade v3.2.0 v3.2.1 diff
symfony/doctrine-bridge upgrade v6.2.5 v6.2.9 diff
symfony/dotenv upgrade v6.2.5 v6.2.8 diff
symfony/error-handler upgrade v6.2.5 v6.2.10 diff
symfony/event-dispatcher upgrade v6.2.5 v6.2.8 diff
symfony/event-dispatcher-contracts upgrade v3.2.0 v3.2.1 diff
symfony/expression-language upgrade v6.2.5 v6.2.7 diff
symfony/filesystem upgrade v6.2.5 v6.2.10 diff
symfony/finder upgrade v6.2.5 v6.2.7 diff
symfony/flex upgrade v2.2.4 v2.2.5 diff
symfony/form upgrade v6.2.5 v6.2.10 diff
symfony/framework-bundle upgrade v6.2.5 v6.2.10 diff
symfony/http-client upgrade v6.2.6 v6.2.10 diff
symfony/http-client-contracts upgrade v3.2.0 v3.2.1 diff
symfony/http-foundation upgrade v6.2.6 v6.2.10 diff
symfony/http-kernel upgrade v6.2.6 v6.2.10 diff
symfony/intl upgrade v6.2.5 v6.2.10 diff
symfony/lock upgrade v6.2.5 v6.2.8 diff
symfony/mailer upgrade v6.2.5 v6.2.8 diff
symfony/mime upgrade v6.2.5 v6.2.10 diff
symfony/monolog-bridge upgrade v6.2.5 v6.2.8 diff
symfony/options-resolver upgrade v6.2.5 v6.2.7 diff
symfony/password-hasher upgrade v6.2.5 v6.2.7 diff
symfony/process upgrade v6.2.5 v6.2.10 diff
symfony/property-access upgrade v6.2.5 v6.2.8 diff
symfony/property-info upgrade v6.2.5 v6.2.10 diff
symfony/proxy-manager-bridge upgrade v6.2.5 v6.2.7 diff
symfony/routing upgrade v6.2.5 v6.2.8 diff
symfony/runtime upgrade v6.2.5 v6.2.8 diff
symfony/security-bundle upgrade v6.2.6 v6.2.10 diff
symfony/security-core upgrade v6.2.5 v6.2.8 diff
symfony/security-csrf upgrade v6.2.5 v6.2.7 diff
symfony/security-http upgrade v6.2.6 v6.2.10 diff
symfony/serializer upgrade v6.2.5 v6.2.10 diff
symfony/service-contracts upgrade v3.2.0 v3.2.1 diff
symfony/string upgrade v6.2.5 v6.2.8 diff
symfony/translation upgrade v6.2.5 v6.2.8 diff
symfony/translation-contracts upgrade v3.2.0 v3.2.1 diff
symfony/twig-bridge upgrade v6.2.5 v6.2.8 diff
symfony/twig-bundle upgrade v6.2.5 v6.2.7 diff
symfony/uid upgrade v6.2.5 v6.2.7 diff
symfony/validator upgrade v6.2.5 v6.2.10 diff
symfony/var-dumper upgrade v6.2.5 v6.2.10 diff
symfony/var-exporter upgrade v6.2.5 v6.2.10 diff
symfony/web-link upgrade v6.2.5 v6.2.7 diff
symfony/yaml upgrade v6.2.5 v6.2.10 diff
twig/extra-bundle upgrade v3.5.0 v3.6.0 diff
twig/string-extra upgrade v3.5.0 v3.6.0 diff
twig/twig upgrade v3.5.0 v3.6.0 diff

Dev Package changes

Package Operation From To Changes
doctrine/data-fixtures upgrade 1.6.3 1.6.6 diff
doctrine/doctrine-fixtures-bundle upgrade 3.4.2 3.4.4 diff
masterminds/html5 upgrade 2.7.6 2.8.0 diff
myclabs/deep-copy upgrade 1.11.0 1.11.1 diff
nikic/php-parser upgrade v4.15.3 v4.15.5 diff
phpstan/phpstan upgrade 1.9.14 1.10.15 diff
phpstan/phpstan-doctrine upgrade 1.3.32 1.3.40 diff
phpstan/phpstan-symfony upgrade 1.2.22 1.3.2 diff
phpstan/phpstan-webmozart-assert upgrade 1.2.2 1.2.4 diff
phpunit/php-code-coverage upgrade 10.0.0 10.1.1 diff
phpunit/php-file-iterator upgrade 4.0.0 4.0.2 diff
phpunit/phpunit upgrade 10.0.4 10.1.3 diff
rector/rector upgrade 0.15.11 0.15.25 diff
sebastian/diff upgrade 5.0.0 5.0.3 diff
sebastian/environment upgrade 6.0.0 6.0.1 diff
sebastian/version upgrade 4.0.0 4.0.1 diff
staabm/phpstan-dba upgrade 0.2.56 0.2.72 diff
symfony/browser-kit upgrade v6.2.5 v6.2.7 diff
symfony/css-selector upgrade v6.2.5 v6.2.7 diff
symfony/debug-bundle upgrade v6.2.5 v6.2.7 diff
symfony/dom-crawler upgrade v6.2.5 v6.2.9 diff
symfony/stopwatch upgrade v6.2.5 v6.2.7 diff
symfony/web-profiler-bundle upgrade v6.2.5 v6.2.10 diff

Settings · Docs · Powered by Private Packagist

.env Outdated Show resolved Hide resolved
src/Entity/WebauthnCredential.php Outdated Show resolved Hide resolved
$publicKeyCredentialSource->getCounter()
);
}
parent::saveCredentialSource($publicKeyCredentialSource);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this forward the $flush parameter ? Otherwise, it is useless

Copy link
Author

Choose a reason for hiding this comment

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

The parent already calls the flush method

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the parent does not have this argument at all. So why adding it in the child class without using it ?

@private-packagist
Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Spomky
Copy link
Author

Spomky commented May 21, 2023

Hi @stof,

Many thanks for the first comments.
I pushed the last modifications and it is now working on my platform.
During the coming days, I will improve the way the authenticators are managed.
Note that the UX is far from being perfect. I need help for it to make sure it is in line with your requirements.

@Spomky
Copy link
Author

Spomky commented Jun 20, 2023

Hello,

I have two questions

  1. When I run symfony console doctrine:fixtures:load, the following exception is thrown:

    purging database
    loading App\DataFixtures\UserFixtures
    loading App\DataFixtures\PackageFixtures
    0/100 [>---------------------------] 0% (< 1 sec left) https://github.com/php-fig/log18:18:10 CRITICAL [console] Error thrown while running command "doctrine:fixtures:load". Message: "Invalid fallback user was not found" ["exception" => LogicException { …},"command" => "doctrine:fixtures:load","message" => "Invalid fallback user was not found"]

Is there anything I missed (env var or something else)?

  1. How to add an entity field? I often use doctrine migration, but it seems it is not used here.
    What is the process for adding the following field to the user entity?
    #[ORM\Column(type: 'string', name: 'user_handle', length: 200, nullable: true, unique: true)]
    private ?string $userHandle = null;

Many thanks.
Regard.

Copy link
Contributor

@94noni 94noni left a comment

Choose a reason for hiding this comment

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

some reviews passing by :)

@@ -20,4 +20,6 @@
SymfonyCasts\Bundle\VerifyEmail\SymfonyCastsVerifyEmailBundle::class => ['all' => true],
KnpU\OAuth2ClientBundle\KnpUOAuth2ClientBundle::class => ['all' => true],
Scheb\TwoFactorBundle\SchebTwoFactorBundle::class => ['all' => true],
SpomkyLabs\CborBundle\SpomkyLabsCborBundle::class => ['all' => true],
Webauthn\Bundle\WebauthnBundle::class => ['all' => true],
Copy link
Contributor

Choose a reason for hiding this comment

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

i see that you need on twig files templates/user/login.html.twig & templates/user/manage_authenticators.html.twig some javascript, than seems to be kind of generic no?
maybe the bundle can provide them?
a raw js file (or perhaps a stimulus controller)

"d3": "^3.5.17",
"instantsearch.js": "^2.7.4",
"instantsearch.js": "^4.56.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this required with your PR?

public function manage(): Response
{
$user = $this->getUser();
!$user instanceof User || $this->denyAccessUnlessGranted('IS_AUTHENTICATED_FULLY');
Copy link
Contributor

Choose a reason for hiding this comment

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

what about #[CurrentUser] User $user in the manage() ?

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.

4 participants