w3c-fedid / FedCM

A privacy preserving identity exchange Web API
https://w3c-fedid.github.io/FedCM/
Other
375 stars 72 forks source link

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

Closed bvandersloot-mozilla closed 1 year ago

bvandersloot-mozilla commented 1 year ago

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

bvandersloot-mozilla commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

bvandersloot-mozilla commented 1 year ago

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 commented 1 year ago

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