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

Specify account labels #669

Merged
merged 6 commits into from
Mar 13, 2025
Merged

Specify account labels #669

merged 6 commits into from
Mar 13, 2025

Conversation

cbiesinger
Copy link
Collaborator

@cbiesinger cbiesinger commented Oct 29, 2024

Bug: #553


Preview | Diff

Copy link
Collaborator

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

lgtm modulo naming

@cbiesinger cbiesinger added the agenda+ Regular CG meeting agenda items label Nov 13, 2024
@hlflanagan
Copy link
Contributor

Discussed on https://github.com/fedidcg/meetings/blob/main/2024/2024-11-26-notes.md. Needs further review.

@cbiesinger
Copy link
Collaborator Author

I would like to reiterate why we think this proposal is better than expanding login_hint/domain_hint to cover this.

Conceptually, we envision this as an IDP-internal selector that the RP need not/should not be aware of. Hence the design of putting it in the config URL to filter accounts from the account endpoint, both of which are under control of the IDP.

Putting this under control of the RP makes it easy to make programming errors -- forgetting to specify an account label leads to a UI with accounts that will not work, which is not a great user experience. Keeping this under control of the config file avoids that.

I realize that I did not do a good job framing this well in the meeting, sorry about that. I hope this helps in addition to the "alternatives considered" in #553.

@hlflanagan
Copy link
Contributor

Discussed again on 11 February 2025 call. Discussion: Agree in principle, may need a bit of bikeshedding.

@hlflanagan hlflanagan removed the agenda+ Regular CG meeting agenda items label Feb 11, 2025
@cbiesinger
Copy link
Collaborator Author

On behalf of Chrome, we are happy to rename this.

Straw proposal:

  • in the accounts endpoint: labels (like in the existing proposal)
  • in the config file: account_label

Open to other suggestions!

@cbiesinger
Copy link
Collaborator Author

I have updated the PR with the new names. I believe this is ready to merge now, unless @bvandersloot-mozilla wants to take another look?

@npm1 npm1 merged commit e3e894c into w3c-fedid:main Mar 13, 2025
2 checks passed
@cbiesinger cbiesinger deleted the accountlabels branch March 13, 2025 18:28
github-actions bot added a commit that referenced this pull request Mar 13, 2025
SHA: e3e894c
Reason: push, by npm1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to mattdanielbrown/WebID that referenced this pull request Mar 13, 2025
SHA: e3e894c
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@wseltzer
Copy link
Collaborator

Discussed in 11 March meeting.

aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 19, 2025
Allows "supports_use_other_account" in the toplevel of the
config file.

Also renames "labels" in the account endpoint to "label_hints"
and flattens "accounts: { include: "foo" }" to just
a toplevel "account_label".

All changes behind the new "FedCmOtherAccountAndLabelsNewSyntax"
flag. Even with the flag enabled, the old syntax is still
accepted for now.

As per the discussions here:
w3c-fedid/FedCM#669
w3c-fedid/FedCM#678

Bug: 404568028
Change-Id: Iba33c5f546c68f35ca2a1228096a7a31a68767a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6370034
Reviewed-by: Nicolás Peña <[email protected]>
Reviewed-by: Alex Moshchuk <[email protected]>
Commit-Queue: Christian Biesinger <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1435018}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants