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

Specify the fields API #668

Merged
merged 10 commits into from
Jan 9, 2025
Merged

Specify the fields API #668

merged 10 commits into from
Jan 9, 2025

Conversation

cbiesinger
Copy link
Collaborator

@cbiesinger cbiesinger commented Oct 28, 2024 •
edited by pr-preview bot
Loading

TallTed
TallTed suggested changes Oct 28, 2024
View reviewed changes
@cbiesinger
Copy link
Collaborator Author

Thanks @TallTed, addresssed your comments.

npm1
npm1 reviewed Oct 29, 2024
View reviewed changes
TallTed
TallTed suggested changes Nov 8, 2024
View reviewed changes
@cbiesinger cbiesinger added the agenda+ Regular CG meeting agenda items label Nov 13, 2024
@wseltzer
Copy link
Collaborator

wseltzer commented Nov 19, 2024 •
edited
Loading

Discussed in 19 Nov 2024 WG call minutes. Not yet resolved.

@cbiesinger
Copy link
Collaborator Author

I addressed TallTed's comments.

@cbiesinger
Copy link
Collaborator Author

I have updated the PR to make it optional for a user agent to show a disclosure text/permission prompt.

npm1
npm1 reviewed Dec 4, 2024
View reviewed changes
@cbiesinger
Copy link
Collaborator Author

I've now also added a note that IDPs should support continue_on.

npm1
npm1 reviewed Dec 5, 2024
View reviewed changes
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.

The last bullet point about context/mode seems unrelated to fields, should it go with the rest or totally outside of the new dfn?

@wseltzer
Copy link
Collaborator

wseltzer commented Jan 7, 2025 •
edited
Loading

Discussed at Jan 7 meeting minutes

@cbiesinger
Copy link
Collaborator Author

Because this came up in the meeting -- if a user agent does not (want to) support this feature, then it would not "support[] showing a permission prompt" and would not fall into the "MUST" cases for the UI. That was at least the intention, if I missed a case please do point it out!

bvandersloot-mozilla
bvandersloot-mozilla reviewed Jan 7, 2025
View reviewed changes
spec/index.bs
is defined, and the |provider|'s {{IdentityProviderConfig/clientId}} is not in the list of
|account|["{{IdentityProviderAccount/approved_clients}}"], then the user agent MUST display
the |metadata|["{{IdentityProviderClientMetadata/terms_of_service_url}}"] link.
1. The user agent MUST prompt the user for permission to share the data in |fields|,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this force the display of a dialog with these fields shown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No because line 1380 makes this step only apply if the user agent supports showing a permission prompt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I missed that because tabs are hard and that line was broken up in the diff. 👍

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 autogenerated "Diff" link (in the initial comment in the PR) may be helpful for cases like this

@philsmart
Copy link
Contributor

I realise 'fields' are meaningful parts of data structures, but the naming seems less obvious to an IdP. They sound more specifically like 'requested (identity) attributes/claims'. Would it be worth looking at the naming of this, at least when it is POSTed to the assertions endpoint.

I guess overlapping with OIDCs use of Scopes, and SAML's use of RequestedAttributes.

@samuelgoto
Copy link
Collaborator

I realise 'fields' are meaningful parts of data structures, but the naming seems less obvious to an IdP. They sound more specifically like 'requested (identity) attributes/claims'. Would it be worth looking at the naming of this, at least when it is POSTed to the assertions endpoint.

I guess overlapping with OIDCs use of Scopes, and SAML's use of RequestedAttributes.

I think the closest to fields is OIDC's claims, rather than OAuth's scope. We picked fields because we figured it would be worth making it (a) OIDC-agnostic (e.g. it has to make sense for SAML too) and (b) align with the HTML spec, where <forms> have <fieldset> and fields. I do expect fields to connect to HTML's autofill at some point, so that's why I still think it is properly named. Happy to hear other suggestions, but just wanted to give some context of the considerations we made when picking it.

@philsmart
Copy link
Contributor

philsmart commented Jan 9, 2025 •
edited
Loading

Thanks, @samuelgoto. That's a good point about being protocol agnostic. From an identity perspective, my understanding is that the RP will request certain attributes, and those are returned as verified (by the IdP) claims or assertions about the subject (user in most cases).

I guess this pops up across the Identity and Access landscape in different guidelines and specifications e.g. LDAP, SAML, SCIM, OIDC, NIST SP800-63('Access to the service requires a defined list of attributes'), NIST SP800-63c('The federation transaction can also provide the RP with a set of identity attributes'), and so on. I personally wouldn't oppose reusing an existing term here, but I am not very familiar with alignment to the HTML specification, so maybe 'fields' is best.

bvandersloot-mozilla
bvandersloot-mozilla approved these changes Jan 9, 2025
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.

Since this is not required, looks fine to me!

@npm1 npm1 merged commit 3f25d0d into w3c-fedid:main Jan 9, 2025
2 checks passed
@cbiesinger cbiesinger deleted the fields branch January 9, 2025 18:39
github-actions bot added a commit that referenced this pull request Jan 9, 2025
@cbiesinger @github-actions
Specify the fields API (#668)
fca31d8 
SHA: 3f25d0d
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 Jan 9, 2025
@cbiesinger @github-actions
Specify the fields API (w3c-fedid#668)
d23c291 
SHA: 3f25d0d
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TallTed TallTed TallTed approved these changes

@npm1 npm1 npm1 approved these changes

@bvandersloot-mozilla bvandersloot-mozilla bvandersloot-mozilla approved these changes

Assignees
No one assigned
Labels
agenda+ Regular CG meeting agenda items
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cbiesinger @wseltzer @philsmart @samuelgoto @TallTed @npm1 @bvandersloot-mozilla