-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: allow multiple origins set per RelyingParty #431
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @obroshnij 👋 thank you so much for your time and effort!
I like where this is going!
I will try to give a proper test to this in the next week to see how it works in a real app. I'll let you know once I've done it 🙂
Then again, thanks!
04774a3
to
e0261d0
Compare
@santiagorodriguez96 thanks a lot for a review and feedback. I have address the points that you brought earlier. Please let me know if there is still anything I should improve |
@santiagorodriguez96 I know you guys are pretty busy, but just so that I can also manage my expectations, is there a chance to get this patch merged any time soon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @obroshnij! Thank you for taking the time and the patience for addressing the comments. Sorry that it took me some time to get back to you again.
I've left a couple more comments in order to keep this moving forward 🙂
Then again thank you!
458d738
to
b090300
Compare
hey @santiagorodriguez96 thanks a lot for another review. I have addressed the points you brought up and I think it should be ready for the next review |
3b58fdb
to
3f85603
Compare
* add a possibility to set `allowed_origins` configuration option that would be an alternative to `origin` * update Readme * add deprecation warning * adjust test suite * overwrite writer to consistently trigger deprecation warnings * fix origin extraction code
79501ae
to
761516b
Compare
hi @santiagorodriguez96 thanks for running the CI here. I have fixed the rubocop issues now |
Hi @obroshnij! The PR looks good to me! I'd want to hold off for merging as my plan was to release this as part of I'll try to release |
awesome, I'm looking forward to that 🚀 |
As described in the issue #428
when building a Relying Party that is supposed to be used by iOS and Android native clients, we need to be able to specify multiple allowed origins per RP (https://developer.android.com/identity/sign-in/credential-manager#verify-origin)
This is yet not supported by this library though is also part of standard specification https://w3c.github.io/webauthn/#sctn-validating-origin
Current workaround is to fallback to creating multiple RP abstractions per expected client platform (Android, iOS, etc) and select one of them based on
User-Agent
header (for example). That is of course troublesome and can actually be avoided.This PR allows a relying party to specify either string for origin or array of strings like in the example:
The only drawback of this approach is that in case an array is used for
config.origin
, - it's impossssible to "guess" relying party by origin (which maybe we should not even do?) thus having array there and no explicitly setconfig.rp_id
would result inRpIdVerificationError
which imo should be expected.Hope this PR makes sense. Please let me know should there be any change requests/questions