Support multiple relying parties #296

padulafacundo · 2019-12-30T13:48:29Z

Create a new RelyingParty class to allow the user of the gem to set up multiple instances (tries to solve #285).
This class also makes the registration/authentication ceremonies simpler.

lib/webauthn/relying_party.rb

grzuy · 2020-01-09T14:20:26Z

Nice work!
Interesting to see this prototyped...

I think this is something to seriously consider merging at some point, maybe after waiting a bit for other pieces to mature and settle on.

Thank you!

grzuy · 2020-01-09T14:21:47Z

lib/webauthn/attestation_statement.rb

@@ -13,26 +13,26 @@ class FormatNotSupportedError < Error; end
 ATTESTATION_FORMAT_ANDROID_KEY = "android-key"
 ATTESTATION_FORMAT_TPM = "tpm"

- def self.from(format, statement)
+ def self.from(format, statement, relying_party: WebAuthn.configuration.relying_party)


[We talked about this IRL, but writing a comment for the record]

This extra keyword argument feels a little bit odd, but I can't think of a better alternative right now... :-)

I like it - passing configuration to be used as an argument, by default being a copy of the global configuration. I'm a little torn on the 'relying party' name but I'll not 🚲🏡 too much here 😄

Good to know :-)
Thanks.

Did you use a keyword for backwards compatibility? If you are doing a major version bump perhaps you should change this first? (to positional args, or change all of them to keyword args)

grzuy · 2020-01-09T14:24:54Z

lib/webauthn/relying_party.rb

+ public_key: public_key,
+ sign_count: sign_count,
+ user_verification: user_verification
+ )


I like this... Feels semantically right that no credential is returned unless it's been verified 👌

brauliomartinezlm · 2020-02-17T20:36:31Z

I have just rebased this PR that with the extraction of TPM to a gem and the refactor of moving things to the an attestation object was quite an update.
Let me know if you find any issues after my rebase. I'll be trying to move this PR forward in the following days, but I want to take some time to see if we can avoid passing that relying party parameter through so many layers as we are.

brauliomartinezlm · 2020-04-06T15:01:38Z

Hey folks! I've rebased this PR again. I'm planning to do what I said in my previous comment shortly, hope to have the time this time 🙏

lib/webauthn/relying_party.rb

lib/webauthn/authenticator_attestation_response.rb

spec/conformance/conformance_cache_store.rb

lib/webauthn/signature_verifier.rb

bdewater · 2020-06-21T21:16:06Z

Except for some linter issues this seems ready to go?

brauliomartinezlm · 2020-06-22T00:51:26Z

@bdewater yeah, we're really close.
In my TODO list there is:

~~Fixing Rubocop in this PR (DONE)~~
Make sure/validate we are comfortable with the API for verify_authentication with the block syntax with the alternative to pass in public_key and sign_count in case you don't want to use the block. For this I would love @bdewater and @grzuy feedback specially. You can see the usage in both the demo app or this comprehensive test I wrote for the new API and the coexistence with the old
Update README to reflect the new API (might be done in a followup PR if we want to have this in sooner as the current docs are still a valid usage)

…t configuration

…lock

bdewater · 2020-06-25T18:05:30Z

At first glance this looks fine, I will make time this weekend for a more thorough review.

Regarding the readme: I was thinking it's getting pretty long and I'm not sure if a lot of people will need this. Perhaps a markdown file in /docs would be better?

grzuy · 2020-06-27T20:11:16Z

Make sure/validate we are comfortable with the API for verify_authentication with the block syntax with the alternative to pass in public_key and sign_count in case you don't want to use the block. For this I would love @bdewater and @grzuy feedback specially.

I think this PR is a good fit for a v3.0.0.alpha1.

We unblock people needing/asking this for a few months already, we potentially collect some feedback on the new RelyingParty public API, and we have some wiggle room for making some minor adjustments if needed before freezing the API on v3.0.0.

grzuy · 2020-06-27T20:19:23Z

I think this is a good fit for a v3.0.0.alpha1.

Will release this within the next hour.

grzuy · 2020-06-27T20:33:36Z

Update README to reflect the new API (might be done in a followup PR if we want to have this in sooner as the current docs are still a valid usage)

Follow up PR sounds good to me.

grzuy · 2020-06-27T20:44:51Z

Excellent work @padulafacundo and @brauliomartinezlm 🙌 🙌

Thanks @bdewater for all the code review help here also!

grzuy · 2020-06-27T20:45:22Z

Anyone feel free to continue providing feedback/comments on this PR despite merged.

sandstrom

Since I submitted the original issue (#285) around this I thought I'd add some feedback here.

Sorry for being late to the game, I hadn't noticed the activity that's taken place here. Some of my suggestions may be "too late", also they are fairly subjective so do what you want with them.

Either way I'm happy to see that there is now support for multiple PRs within the same app. That's awesome! 😄

sandstrom · 2020-06-29T11:44:01Z

lib/webauthn/attestation_statement.rb

@@ -13,26 +13,26 @@ class FormatNotSupportedError < Error; end
 ATTESTATION_FORMAT_ANDROID_KEY = "android-key"
 ATTESTATION_FORMAT_TPM = "tpm"

- def self.from(format, statement)
+ def self.from(format, statement, relying_party: WebAuthn.configuration.relying_party)


Did you use a keyword for backwards compatibility? If you are doing a major version bump perhaps you should change this first? (to positional args, or change all of them to keyword args)

sandstrom · 2020-06-29T11:45:56Z

lib/webauthn/configuration.rb

- raise RootCertificateFinderNotSupportedError, "Finder must implement `find` method"
- end
- end
+ def rp_id


Is there a reason why you aren't spelling out def relying_party?

sandstrom · 2020-06-29T11:59:34Z

lib/webauthn/credential.rb

- def self.from_get(credential)
- WebAuthn::PublicKeyCredentialWithAssertion.from_client(credential)
+ def self.from_get(credential, relying_party: WebAuthn.configuration.relying_party)
+ WebAuthn::PublicKeyCredentialWithAssertion.from_client(credential, relying_party: relying_party)


Quite subjective, but if I where you I think I'd move the API over to something like this:

relying_party = WebAuthn::RelyingParty.new(…) relying_party.public_key_credential_with_attestation(credential)

Closer to how most data base adapters (for example) operate. Everything is based of a single client instance. If I wanted to avoid RelyingParty.new I'd maybe add the possibility to keep a WebAuthn.default_rp that was still configurable via configure.

I'd probably cease support for the current API (for simplicity and less maintenance). With a clear "upgrade guide" for different method calls it should still be a simple migration.

If I wanted to support existing API plus new "relying party instance API", I would replace most (all?) methods that take a relying party instance as an argument with a similarly named method on the relying party instance, such that the instance can pass itself in instead. In other words, all diffs in this PR that add an argument passing in RP I'd instead add that method on RP instance.

sandstrom · 2020-06-29T12:01:05Z

lib/webauthn/public_key_credential/creation_options.rb

- rp[:name] ||= configuration.rp_name
- rp[:id] ||= configuration.rp_id
+ rp[:name] ||= relying_party.name
+ rp[:id] ||= relying_party.id

 RPEntity.new(**rp)


For consistency, would it make sense to rename this to RelyingPartyEntity?

grzuy · 2020-06-29T23:18:56Z

Since I submitted the original issue (#285) around this I thought I'd add some feedback here.

Sorry for being late to the game, I hadn't noticed the activity that's taken place here. Some of my suggestions may be "too late", also they are fairly subjective so do what you want with them.

Thank you very much for taking the time to review and provide feedback!
We appreciate it :-)

Either way I'm happy to see that there is now support for multiple PRs within the same app. That's awesome! smile

This reverts commit 2f54c92.

This reverts commit dbff817. Co-authored-by: Braulio Martinez <[email protected]>

* WIP: Relying Party model * fixup! WIP: Relying Party model * refactor: make `WebAuthn.configuration` a wrapper for a `RelyingParty` instance * fixup! refactor: make `WebAuthn.configuration` a wrapper for a `RelyingParty` instance * refactor: delegate Configuration methods * refactor: always pass a RelyingParty * use relying party on when creating options * refactor: remove unnecessary aliases * fix: make reader private * refactor: make parent class AuthenticatorResponse responsible for relying party * fix: post rebase fixes * test: get rid of all keyword 2.7 warnings * fix: rebase trailing diff * fix: make rubocop happy * fix: make AttestationObject#deserialize accept relying_party param * refactor: algorithm validation uses relying party configuration * fix: make RP initializer use finders setter to process params correctly * fix: cache store should recieve origin from server * refactor: CR comments on API flexibility * refactor: CR comments * test: add functional comprehensive test for multiple RPs and a default configuration * refactor: Allow #verify_authentication to be used with or without a block * style: fix rubocop Co-authored-by: Braulio Martinez <[email protected]>

padulafacundo commented Dec 30, 2019

View reviewed changes

lib/webauthn/relying_party.rb Outdated Show resolved Hide resolved

padulafacundo marked this pull request as ready for review January 3, 2020 13:04

grzuy reviewed Jan 9, 2020

View reviewed changes

brauliomartinezlm mentioned this pull request Feb 17, 2020

Using webauth-ruby with multiple domains #308

Closed

brauliomartinezlm force-pushed the relying_party branch 3 times, most recently from ab07f0a to 2ae33e7 Compare February 17, 2020 20:33

brauliomartinezlm force-pushed the relying_party branch from 2ae33e7 to 3441374 Compare February 17, 2020 20:52

grzuy changed the title ~~Relying Party model~~ Support multiple relying parties Feb 22, 2020

brauliomartinezlm force-pushed the relying_party branch from 782d6bc to 1408dce Compare April 6, 2020 14:59

grzuy marked this pull request as draft April 26, 2020 21:32

grzuy reviewed May 16, 2020

View reviewed changes

lib/webauthn/relying_party.rb Outdated Show resolved Hide resolved

grzuy reviewed May 16, 2020

View reviewed changes

lib/webauthn/relying_party.rb Outdated Show resolved Hide resolved

lib/webauthn/relying_party.rb Outdated Show resolved Hide resolved

lib/webauthn/relying_party.rb Outdated Show resolved Hide resolved

lib/webauthn/relying_party.rb Outdated Show resolved Hide resolved

grzuy mentioned this pull request May 16, 2020

refactor: use WebAuthn::RelyingParty cedarcode/webauthn-rails-demo-app#117

Merged

grzuy marked this pull request as ready for review May 16, 2020 22:07

grzuy reviewed May 16, 2020

View reviewed changes

lib/webauthn/authenticator_attestation_response.rb Outdated Show resolved Hide resolved

spec/conformance/conformance_cache_store.rb Outdated Show resolved Hide resolved

lib/webauthn/signature_verifier.rb Outdated Show resolved Hide resolved

brauliomartinezlm force-pushed the relying_party branch from ee0e581 to 2d6d49a Compare May 23, 2020 20:36

brauliomartinezlm mentioned this pull request May 25, 2020

Remove SignatureVerifier #313

Merged

brauliomartinezlm force-pushed the relying_party branch from 40e8ad8 to 1c739ca Compare May 31, 2020 20:05

brauliomartinezlm force-pushed the relying_party branch from 1c739ca to 3181c5c Compare June 13, 2020 22:13

brauliomartinezlm force-pushed the relying_party branch 2 times, most recently from 46fc826 to 899c3a0 Compare June 20, 2020 22:54

This was referenced Jun 21, 2020

Add optional origin argument to AuthenticatorAttestationResponse#verify #320

Closed

Rubocop: ignore vendor directory #321

Merged

brauliomartinezlm force-pushed the relying_party branch from a22a381 to df318ca Compare June 22, 2020 02:39

brauliomartinezlm added 11 commits June 22, 2020 18:45

fix: rebase trailing diff

02d58b9

fix: make rubocop happy

086afdd

fix: make AttestationObject#deserialize accept relying_party param

a8f3bef

refactor: algorithm validation uses relying party configuration

802cb08

fix: make RP initializer use finders setter to process params correctly

ba86a29

fix: cache store should recieve origin from server

0ef99c5

refactor: CR comments on API flexibility

e1545e8

refactor: CR comments

9ebac8a

test: add functional comprehensive test for multiple RPs and a defaul…

0015ee1

…t configuration

refactor: Allow #verify_authentication to be used with or without a b…

e06fab3

…lock

style: fix rubocop

fdff64d

brauliomartinezlm force-pushed the relying_party branch from df318ca to fdff64d Compare June 22, 2020 21:46

grzuy merged commit 2f54c92 into master Jun 27, 2020

grzuy deleted the relying_party branch June 27, 2020 20:43

grzuy mentioned this pull request Jun 27, 2020

Support multiple domains (instance-based) #285

Closed

sandstrom reviewed Jun 29, 2020

View reviewed changes

brauliomartinezlm mentioned this pull request Jul 4, 2020

Docs: document Instance based configuration approach #327

Merged

grzuy added a commit that referenced this pull request Feb 28, 2021

Revert "Support multiple relying parties (#296)"

dbff817

This reverts commit 2f54c92.

grzuy pushed a commit that referenced this pull request Feb 28, 2021

Revert "Revert "Support multiple relying parties (#296)""

f930c47

This reverts commit dbff817. Co-authored-by: Braulio Martinez <[email protected]>

santiagorodriguez96 mentioned this pull request Mar 25, 2021

support for multiple origins #344

Closed

bdewater mentioned this pull request Sep 8, 2022

3.0.0.alpha2 #368

Merged

santiagorodriguez96 mentioned this pull request Oct 4, 2022

Allow multiple origins #373

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple relying parties #296

Support multiple relying parties #296

padulafacundo commented Dec 30, 2019

grzuy commented Jan 9, 2020

grzuy Jan 9, 2020

bdewater Jan 9, 2020

grzuy Jan 10, 2020

sandstrom Jun 29, 2020

grzuy Jan 9, 2020

brauliomartinezlm commented Feb 17, 2020

brauliomartinezlm commented Apr 6, 2020

bdewater commented Jun 21, 2020

brauliomartinezlm commented Jun 22, 2020 •

edited

Loading

bdewater commented Jun 25, 2020

grzuy commented Jun 27, 2020 •

edited

Loading

grzuy commented Jun 27, 2020 •

edited

Loading

grzuy commented Jun 27, 2020

grzuy commented Jun 27, 2020

grzuy commented Jun 27, 2020

sandstrom left a comment

sandstrom Jun 29, 2020

sandstrom Jun 29, 2020

sandstrom Jun 29, 2020 •

edited

Loading

sandstrom Jun 29, 2020

grzuy commented Jun 29, 2020 •

edited

Loading

Support multiple relying parties #296

Support multiple relying parties #296

Conversation

padulafacundo commented Dec 30, 2019

grzuy commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brauliomartinezlm commented Feb 17, 2020

brauliomartinezlm commented Apr 6, 2020

bdewater commented Jun 21, 2020

brauliomartinezlm commented Jun 22, 2020 • edited Loading

bdewater commented Jun 25, 2020

grzuy commented Jun 27, 2020 • edited Loading

grzuy commented Jun 27, 2020 • edited Loading

grzuy commented Jun 27, 2020

grzuy commented Jun 27, 2020

grzuy commented Jun 27, 2020

sandstrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandstrom Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzuy commented Jun 29, 2020 • edited Loading

brauliomartinezlm commented Jun 22, 2020 •

edited

Loading

grzuy commented Jun 27, 2020 •

edited

Loading

grzuy commented Jun 27, 2020 •

edited

Loading

sandstrom Jun 29, 2020 •

edited

Loading

grzuy commented Jun 29, 2020 •

edited

Loading