-
Notifications
You must be signed in to change notification settings - Fork 73
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
Keep HOWTO-chrome.md up to date #506
Conversation
explorations/HOWTO-chrome.md
Outdated
account_id=123&client_id=client1234&nonce=Ct60bD&disclosure_text_shown=true&is_account_auto_selected=true | ||
``` | ||
|
||
For the API caller, the browser will include the boolean `isAccountAutoSelected` when resolving the promise with the token: |
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.
For the API caller, the browser will include the boolean `isAccountAutoSelected` when resolving the promise with the token: | |
For the Relying Party, the browser will include the boolean `isAccountAutoSelected` when resolving the promise with the token: |
Thinking this might be more consistent language here, but not certain.
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.
While it's true that API is called on the RP site, the API caller could be the RP itself or the IdP (via SDK) or 3P library that uses FedCM API.
explorations/HOWTO-chrome.md
Outdated
``` | ||
{ | ||
"token": "eyJC...J9.eyJzdWTE2...MjM5MDIyfQ.SflV_adQssw....5c", | ||
"isAccountAutoSelected": true |
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.
providing this information seems like it would be unnecessary. What's the motivation for indicating this to the RP/IDP rather than just making this a client side only piece of information? Say for example, that the user doesn't auto select an account could that reveal information about whether or not it's a temp account versus a primary permanent account based on statistical analysis?
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.
Could you elaborate a bit on the attacking vector here? I'm not sure if I understand what you meant by temp account v.s. primary account. The boolean can only be true for "returning" account which means that the user has used that account in this browser client in the past.
For motivation and privacy & security considerations, please see #497
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.
Thanks, I'll have to give a read into this a bit further. I wasn't aware of the linked issue before this, but just came across it. Do you guys have a glitch.me demo of this feature built by chance? I'd like to play around with this feature a bit more to make sure I'm understanding the goals and am able to assess the potential impact correctly.
FWIW, don't block this on my review but my hunch does think this is probably oversharing information. If I come up with a practical attack then it could always be reverted later too.
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.
You could try https://webid-fcm.glitch.me after enabling the flag. The boolean will be displayed in the log section.
Note: in Chrome, after auto re-authen is triggered, it will be suppressed for 10 mins (or until a user explicitly signs in).
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 Kyle, ok with us merging this howto change or do you have any further comment?
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.
Yeah fine to merge as is. I'll take a look into this post merge when I pick up some time to see if I spot any concerns with it. If I do find something I'll open an issue with follow up suggested changes.
Co-authored-by: Kyle Den Hartog <[email protected]>
explorations/HOWTO-chrome.md
Outdated
``` | ||
{ | ||
"token": "eyJC...J9.eyJzdWTE2...MjM5MDIyfQ.SflV_adQssw....5c", | ||
"isAccountAutoSelected": true |
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 Kyle, ok with us merging this howto change or do you have any further comment?
SHA: 5b07ad1 Reason: push, by npm1 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 5b07ad1 Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR adds two new features and removes the flags requirement for shipped features.