Restructuring pass of core spec information (IDP and Browser APIs) #424

bvandersloot-mozilla · 2023-02-08T17:43:52Z

This should improve the clarity of these central sections of the spec and call out a few shortcomings

This should address the following issues from Issue #416: 7-9, 12, 13, 15. This leaves only 16 & 19 from issue #416 remaining.

Preview | Diff

npm1

Thanks for working on this! I think I don't love the placement of the dictionaries because there is no context on how they'd be used. I think they belong better closer to where they were. It might still be ok to have the order of the sections swapped if that is your preference, although that will mean the dictionaries are defined after they are used in the processing model.

npm1 · 2023-02-08T22:32:50Z

spec/index.bs


+The Sign-up and Sign-in APIs are used by the [=RP=]s to ask the browser


There are no Sign-up and Sign-in APIs (and this is explained in the Note below), so perhaps remove lines 274 - 279?

spec/index.bs

npm1 · 2023-02-08T22:45:13Z

spec/index.bs

-dictionary IdentityProviderClientMetadata {
-  USVString privacy_policy_url;
-  USVString terms_of_service_url;
+dictionary IdentityProviderWellKnown {


The location of these dictionaries seems off to me. There is no context for these definitions

unifying convo here: #424 (comment)

npm1 · 2023-02-08T22:47:50Z

spec/index.bs

 <xmp class="idl">
-dictionary IdentityProviderToken {
-  required USVString token;
+dictionary IdentityProviderAccount {


unifying convo here: #424 (comment)

npm1 · 2023-02-08T22:50:24Z

spec/index.bs


-TODO: add an ASCII image to explain how the states fit into the algorithms.
+Issue: Displaying an account chooser displaying all of the provided options should not be included normatively


As I has mentioned on the proposal, I don't understand why this is an issue, can you elaborate?

Generally we shouldn't dictate that particular UI get displayed because a browser could obtain user choice can be rendered in multiple ways... e.g. advance configuration, an "always use this account" checkbox, or defaulting to choose the only account if only one is available.

This spec has to toe a fine line between requiring some user consent/information without dictating UI/UX choices. and I am just pointing out that this is probably over that line IMO

There is a lot going on in this PR and I'm a bit worried that it is going to be hard to determine / review what's just moving things around vs new text that we are introducing. For example, I missed this entirely, and if it weren't for npm double checking this would've probably slipped through. I don't know what the merits are of this specific point here being discussed, but a bit of a meta point is that I think it would be useful to try to make some of the moves so that we can easily tell (by looking at the diff) that we are keeping the normative algorithms / interfaces / etc intact without having to look at every single LOC.

I don't know if that's possible, but just wanted to throw it out there in this review that I worry about the scope of this PR.

npm1 · 2023-02-08T22:52:05Z

spec/index.bs

 <!-- ============================================================ -->
-## The <code>\[[DiscoverFromExternalSource]](origin, options, sameOriginWithAncestors)</code> internal method ## {#browser-api-rp-sign-in}
+## Manifest ## {#idp-api-manifest}


nit: we should rename this to Config

npm1 · 2023-02-08T22:54:57Z

spec/index.bs

-    ::  A random number of the choice of the [=RP=]. It is generally used to associate a client
-        session with a {{IdentityProviderToken/token}} and to mitigate replay attacks. Therefore, this value should have
-        sufficient entropy such that it would be hard to guess.
+The file is parsed expecting a {{IdentityProviderWellKnown}} JSON object with the following semantics:


Since this is where IdentityProviderWellKnown is explained, I would add the IDL here. WDYT? Similar for the rest. These are just dictionaries explaning how the IDP files should be structured, so I think it is OK in a non-normative section.

unifying convo here: #424 (comment)

bvandersloot-mozilla · 2023-02-09T15:20:19Z

Thanks for working on this! I think I don't love the placement of the dictionaries because there is no context on how they'd be used. I think they belong better closer to where they were. It might still be ok to have the order of the sections swapped if that is your preference, although that will mean the dictionaries are defined after they are used in the processing model.

Maybe. I don't mind having the definition apart from its use. My concern is that the IDL definitions are normative. They describe the JSON structure required and expected. So I'd really like them to be in a normative section.

npm1 · 2023-02-09T16:44:37Z

Maybe. I don't mind having the definition apart from its use. My concern is that the IDL definitions are normative. They describe the JSON structure required and expected. So I'd really like them to be in a normative section.

I don't know about those IDL definitions being normative here. They are describing the expected format of the JSON file from IDP and are helpful as dictonaries because we can just quote them in the algorithm, but they are not really webexposed in the IDL sense so I think that those would be fine in the non-normative section.

Perhaps we can keep them where they are (although preference to move them to before their first use, and that does not seem to be their location right now) but add intro paragraphs to explain what we're defining? Right now there is no context to those IDLs

bvandersloot-mozilla · 2023-02-09T17:24:45Z

They end up being normative in a few ways. For one, IdentityProviderConfig defines the structure of the object here: navigator.credentials.get({"identity":{"providers":[HERE]}}). Also, these objects are converted from JSON objects in the "fetch the X file" algorithms, so changing those definitions changes the behavior of those algorithms.

Although I definitely agree, there could be more context for their definition. I just put them in the list of defined algorithms after their first use, which seemed to be the convention followed in that section.

npm1 · 2023-02-09T19:51:04Z

Although I definitely agree, there could be more context for their definition. I just put them in the list of defined algorithms after their first use, which seemed to be the convention followed in that section.

If you mean the algorithms then yea I think that makes sense? For dictionaries, it should be easier and clearer to explain what they are and define them before they are used in the algorithms.

bvandersloot-mozilla · 2023-02-10T13:13:06Z

If you mean the algorithms then yea I think that makes sense?

I mean to say that the algorithms and dictionaries are serving the same purpose here: to precisely specify what is required of an implementer in digestible pieces. The dictionary's IDL serves the purpose of defining the requirements of the structure (including the details of de-serializing it). So perhaps both would benefit from being explained more clearly.

For dictionaries, it should be easier and clearer to explain what they are and define them before they are used in the algorithms.

Maybe? But in the Browser API section you only really need to know how they are used, not what they are and why. If the reader is confused by what the semantics of the values are, they can use bikeshed links like this to navigate around for more context. Alternatively, this would be a great avenue for editorial improvement to the Introduction- especially giving context to some of those typograms at the end.

npm1 · 2023-02-10T14:22:39Z

Ok, I think I'm ok with this. We should rebase though since there are merge conflicts. I also left some other comments but they can be resolved in a separate PR since this is mainly just reordering things.

bvandersloot-mozilla · 2023-02-10T16:54:40Z

Okay, I think I have resolved merge conflicts well. Sounds good!

spec/index.bs

samuelgoto

I went through this PR and am a bit worried that it is hard for me to be confident that we are keeping the normative parts intact (I'm assuming that was the intent?). because we changed the orders of the sections (which sounds right to me), between the browser API and the IDP HTTP API, the diff is hard to parse for errors. I don't feel strongly about this, because we can just look more manually for copying/pasting errors, but if you can think of ways to make the move a bit easier to detect for errors, that'd be great.

spec/index.bs

samuelgoto · 2023-02-10T17:43:34Z

spec/index.bs


-TODO: add an ASCII image to explain how the states fit into the algorithms.
+Issue: Displaying an account chooser displaying all of the provided options should not be included normatively


There is a lot going on in this PR and I'm a bit worried that it is going to be hard to determine / review what's just moving things around vs new text that we are introducing. For example, I missed this entirely, and if it weren't for npm double checking this would've probably slipped through. I don't know what the merits are of this specific point here being discussed, but a bit of a meta point is that I think it would be useful to try to make some of the moves so that we can easily tell (by looking at the diff) that we are keeping the normative algorithms / interfaces / etc intact without having to look at every single LOC.

I don't know if that's possible, but just wanted to throw it out there in this review that I worry about the scope of this PR.

bvandersloot-mozilla · 2023-02-13T13:33:20Z

I went through this PR and am a bit worried that it is hard for me to be confident that we are keeping the normative parts intact (I'm assuming that was the intent?). because we changed the orders of the sections (which sounds right to me), between the browser API and the IDP HTTP API, the diff is hard to parse for errors. I don't feel strongly about this, because we can just look more manually for copying/pasting errors, but if you can think of ways to make the move a bit easier to detect for errors, that'd be great.

It is 100% my intent that normative parts of the spec remain intent. I agree that the Diffs are rather difficult to read. The rendered diff makes it a little easier, but not too much. Let my try splitting it across a few commits to see if they are piecemeal more readable.

bvandersloot-mozilla · 2023-02-13T14:16:05Z

So I split it into 6 commits and left out the Issue that seemed controversial.

Only the first commit (1fe4569) is still hard to read. However if you check it out and do the diff with --diff-algorithm=minimal, you get a much cleaner diff (attached): 1fe4569.patch

samuelgoto

So I split it into 6 commits and left out the Issue that seemed controversial.

This indeed helped review the patch. I did check manually the first commit and couldn't find anything else missing.

I'm going to merge this since (a) this is a good restructuring that is going to help going forward, (b) the intent is to keep it purely non-normative, so in case we do find that this caused normative changes we can look at this history here to see where things may have lost, (c) there are other PRs that depend on this and (d) I think future PRs are going to be more incremental than this.

spec/index.bs

samuelgoto · 2023-02-13T21:20:20Z

spec/index.bs

 1. A [[#idp-api-manifest]] endpoint in an agreed upon location that points to
 1. An [[#idp-api-accounts-endpoint]] endpoint
 1. A [[#idp-api-client-id-metadata-endpoint]] endpoint
 1. An [[#idp-api-id-assertion-endpoint]] endpoint

-
-The [=IDP=] must also host a file at the ".well-known/web-identity" path of its [=registrable domain=] that has JSON contents that are convertable to an {{IdentityProviderWellKnown}} object.


We are losing this non-normative text here about the location of the ".well-known/web-identity" file. We are keeping the normative section, so, not a substantative change, but seemed worth noting that this is going to be lost in your move here.

) SHA: 10c8d09 Reason: push, by samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…3c-fedid#424) SHA: 10c8d09 Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…3c-fedid#424) SHA: 10c8d09 Reason: push, by bvandersloot-mozilla Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

npm1 reviewed Feb 8, 2023

View reviewed changes

samuelgoto reviewed Feb 10, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

samuelgoto reviewed Feb 10, 2023

View reviewed changes

bvandersloot-mozilla added 6 commits February 13, 2023 08:38

Rename IDP API section and move it after Browser API

1fe4569

Move sign in and compute account state into the state machine section

2871c38

Editorial changes inside of Browser API

237f209

Editorial changes to treat the well-known file like other endpoints

16a11ea

Add note to validate the nonce of the assertion endpoint

43dc3f6

Lift IDL definitions into the normative Browser API section

666226b

bvandersloot-mozilla force-pushed the main branch from 359ebbf to 666226b Compare February 13, 2023 14:05

This was referenced Feb 13, 2023

Add Identity Provider branding name as an optional field #432

Merged

Convert required email field in user account to optional #433

Open

samuelgoto approved these changes Feb 13, 2023

View reviewed changes

samuelgoto merged commit 10c8d09 into w3c-fedid:main Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructuring pass of core spec information (IDP and Browser APIs) #424

Restructuring pass of core spec information (IDP and Browser APIs) #424

bvandersloot-mozilla commented Feb 8, 2023 •

edited by pr-preview bot

Loading

npm1 left a comment

npm1 Feb 8, 2023

npm1 Feb 8, 2023

bvandersloot-mozilla Feb 9, 2023

npm1 Feb 8, 2023

bvandersloot-mozilla Feb 9, 2023

npm1 Feb 8, 2023

bvandersloot-mozilla Feb 9, 2023

samuelgoto Feb 10, 2023

npm1 Feb 8, 2023

npm1 Feb 8, 2023

bvandersloot-mozilla Feb 9, 2023

bvandersloot-mozilla commented Feb 9, 2023

npm1 commented Feb 9, 2023

bvandersloot-mozilla commented Feb 9, 2023

npm1 commented Feb 9, 2023

bvandersloot-mozilla commented Feb 10, 2023

npm1 commented Feb 10, 2023

bvandersloot-mozilla commented Feb 10, 2023

samuelgoto left a comment

samuelgoto Feb 10, 2023

bvandersloot-mozilla commented Feb 13, 2023

bvandersloot-mozilla commented Feb 13, 2023

samuelgoto left a comment

samuelgoto Feb 13, 2023


		The Sign-up and Sign-in APIs are used by the [=RP=]s to ask the browser


		TODO: add an ASCII image to explain how the states fit into the algorithms.
		Issue: Displaying an account chooser displaying all of the provided options should not be included normatively

Restructuring pass of core spec information (IDP and Browser APIs) #424

Restructuring pass of core spec information (IDP and Browser APIs) #424

Conversation

bvandersloot-mozilla commented Feb 8, 2023 • edited by pr-preview bot Loading

npm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvandersloot-mozilla commented Feb 9, 2023

npm1 commented Feb 9, 2023

bvandersloot-mozilla commented Feb 9, 2023

npm1 commented Feb 9, 2023

bvandersloot-mozilla commented Feb 10, 2023

npm1 commented Feb 10, 2023

bvandersloot-mozilla commented Feb 10, 2023

samuelgoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvandersloot-mozilla commented Feb 13, 2023

bvandersloot-mozilla commented Feb 13, 2023

samuelgoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvandersloot-mozilla commented Feb 8, 2023 •

edited by pr-preview bot

Loading