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.

Sign up for GitHub

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

Jump to bottom

Returning accounts go first in getUserInfo #483

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Returning accounts go first in getUserInfo #483

wants to merge 6 commits into from

Conversation

npm1
Copy link
Collaborator

@npm1 npm1 commented Jun 23, 2023 •
edited by pr-preview bot
Loading

This PR fixes the order of returned accounts in getUserInfo(). The returning list should list the returning accounts first, and then all the remaining accounts.

Fixes #625

Preview | Diff

npm1 added 3 commits June 23, 2023 14:00
@npm1
Returning accounts go first in getUserInfo
be49ab5 
This PR fixes the way an account is chosen as returning for getUserInfo: if approvedClients is available, that is considered the source of truth. Also, the returning list should list the returning accounts first, and then all the remaining accounts.
@npm1
Add i mentions
b1a303d
@npm1
fix
1a80739
@npm1 npm1 requested a review from samuelgoto June 23, 2023 18:07
@npm1
Copy link
Collaborator Author

npm1 commented Jun 23, 2023

@bvandersloot-mozilla please take a look

cbiesinger
cbiesinger reviewed Jun 23, 2023
View reviewed changes
spec/index.bs Outdated Show resolved Hide resolved
@npm1
Copy link
Collaborator Author

npm1 commented Jun 27, 2023

Ping

cbiesinger
cbiesinger approved these changes Jun 27, 2023
View reviewed changes
Copy link
Collaborator

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

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

not sure if you were waiting for me here but lgtm

@npm1
Copy link
Collaborator Author

npm1 commented Jan 4, 2024

I noticed this PR is still relevant. Rebased since it was pretty old but still want to merge this. Anyone have any further comments?

@npm1 npm1 added the agenda+ Regular CG meeting agenda items label Jul 25, 2024
@timcappalli timcappalli self-requested a review July 30, 2024 15:58
bvandersloot-mozilla
bvandersloot-mozilla requested changes Jul 31, 2024
View reviewed changes
Copy link
Collaborator

@bvandersloot-mozilla bvandersloot-mozilla left a comment

Choose a reason for hiding this comment

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

Computing a secret bit and using it to re-sort a list returned to the webpage is not a good pattern. If we are going to alter the output, it should at least be observable why.
This is hard to work with. Is accounts[0] previously used? accounts[n-1]?

spec/index.bs Outdated
@@ -1635,23 +1635,27 @@ When invoking the {{IdentityProvider/getUserInfo()}} method given an {{IdentityP
{{DOMException}}.
1. Let |accountsList| be the result of [=fetch the accounts list=] with |config|, |provider|,
and |globalObject|.
1. Let |hasReturningAccount| be false.
1. For each |account| in |accountsList|:
1. Let |isReturningAccount| be a new [=list=] of the same length as |accountsList|, with all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be on the accounts, rather than adding a new boolean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The accounts are dictionaries parsed from the IdP's response, so we cannot add properties to them.

spec/index.bs
1. Let |notReturningUserInfos| be a new [=list=].
1. For each |i| from 0 to the length of |accountsList| minus 1:
1. Let |account| be |accountsList|[|i|].
1. Let |userInfo| be an {{IdentityUserInfo}} with the following values:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or at least in the IdentityUserInfo? that way we can expose it to the webpage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems like a broader change (adding a webexposed member to the IdentityUserInfo. The goal of this PR is to fix a bug in the existing returned values.

@npm1
Copy link
Collaborator Author

npm1 commented Jul 31, 2024

Computing a secret bit and using it to re-sort a list returned to the webpage is not a good pattern. If we are going to alter the output, it should at least be observable why. This is hard to work with. Is accounts[0] previously used? accounts[n-1]?

The idea is that accounts[0] is previously used. @yi-gu do you remember why we do not expose whether an account is returning or not explicitly in this API? It's not clear to me.

@bvandersloot-mozilla
Copy link
Collaborator

The idea is that accounts[0] is previously used.
But you can't guarantee that.

@npm1
Copy link
Collaborator Author

npm1 commented Aug 1, 2024

The idea is that accounts[0] is previously used.
But you can't guarantee that.

You can. getUserInfo only returns anything if there are returning accounts. And we surface returning accounts first, followed by other accounts. So accounts[0] is a returning account.

@npm1 npm1 removed the agenda+ Regular CG meeting agenda items label Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bvandersloot-mozilla bvandersloot-mozilla bvandersloot-mozilla requested changes

@cbiesinger cbiesinger cbiesinger approved these changes

@samuelgoto samuelgoto Awaiting requested review from samuelgoto

@timcappalli timcappalli Awaiting requested review from timcappalli

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
chrome
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Returning accounts go first in getUserInfo
4 participants
@npm1 @bvandersloot-mozilla @cbiesinger @samuelgoto