Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

Various Documentation improvements #27

Merged
merged 17 commits into from
Aug 12, 2019

Conversation

jaxoncreed
Copy link
Contributor

@jaxoncreed jaxoncreed commented May 14, 2019

Covers:

  • Describes the use case and implementation of PoP tokens
  • The description of asking for consent on the example page is more in line with reality
  • Extremely detailed walkthrough

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

in the png, '6. Generates and id_token' -> '6. Generates an id_token'
step 5 of 'sending request' seems incorrect to me, IIUC RP only contacts OP to retrieve their jwks, not to ask them to perform validation.

@zenomt
Copy link

zenomt commented May 23, 2019

is this PR just to document how the reference implementation currently works, or to also suggest new functionality that is yet to be implemented? i see there's mention (in "README.md -> The Solution, step 3"), of the origin of the redirect_uri becoming "the audience".

i haven't checked the source to see if that's what the current NSS OP actually does, but regardless that has some issues (most importantly, the aud MUST include the client_id, which as discussed in #12 the client_id can't be just the redirect_uri origin, or even the redirect_uri). i think at least that part should be called out as "as currently implemented" or "proposed behavior" so that it can be commented on appropriately.

if this is documenting what's currently implemented, or at least what's currently designed/planned, then discussing issues with it isn't appropriate in this PR's conversation, and should be in an actual Issue once this change is merged. however, if this PR is also proposing new behavior, then discussing issues with the new behavior would be appropriate here.

@michielbdejong
Copy link

@jaxoncreed ping

@jaxoncreed jaxoncreed changed the title [WIP] Various Documentation improvements Various Documentation improvements Jun 14, 2019
@jaxoncreed
Copy link
Contributor Author

I've now finished the detailed application workflow, so I think we're good to go. It's not the 100% rewrite this needs, but it provides some much better context.

michielbdejong
michielbdejong previously approved these changes Jun 14, 2019
Copy link

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

I didn't read/verify all of it but looks like a great improvement over the current text. We can always iterate if we discover errata. A second review from @zenomt and/or @dmitrizagidulin would be helpful!

@michielbdejong
Copy link

We should probably update the list of people with write access to this repo to match the list of people who know most about how webid-oidc works. Created #32 in relation to this.

@michielbdejong
Copy link

@kjetilk @Mitzi-Laszlo @timbl @justinwb @RubenVerborgh: two of you need to approve this before it can be merged.

@RubenVerborgh
Copy link

@michielbdejong Will do; useful to use the "assign reviewers" functionality for that.

RubenVerborgh
RubenVerborgh previously approved these changes Jun 14, 2019
Copy link

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Technically correct, but I feel that we are too much describing an existing spec here. We should point more and explain less (unless the actual specs are not clear). However, good from a documentation perspective, but need to revisited when turning into an actual spec, where we want to avoid repetition of other specs.

README.md Outdated
@@ -43,7 +43,7 @@ See also: [Motivation for WebID-OIDC](motivation.md).
### Benefits and Capabilities

* Fully decentralized cross-domain authentication (any peer node can serve as
an identity provider as well as a relying party to any other node)
an identity provider as well as a relying party to any other node) made possible by [PoP Tokens](https://tools.ietf.org/html/rfc7800).

Choose a reason for hiding this comment

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

Perhaps say a word about what they are, or at least expand acronym?

README.md Outdated

#### The Problem

Unlike standard implementations of OIDC, WebID-OIDC must deal with a number of RSs many of which the OP will not know about. OIDC defines the `aud` claim which defines the RSs for which a token can be used.

Choose a reason for hiding this comment

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

But we are using a standard, right? Important that people understand this is not a custom extension.

application-workflow-detailed.md Outdated Show resolved Hide resolved
@zenomt
Copy link

zenomt commented Jun 14, 2019

in the current version of the sequence diagram, authorization steps 3 and 4 (request OP configuration) are to Alice's POD, but i think you mean for them to go to her OP.

@jaxoncreed
Copy link
Contributor Author

@zenomt right you are


That URL might look a little complex, but it's essentially a request to `https://secureauth.example/authorize` with the following URL parameters:

- `scope=open_id`: a list of [oidc scpes](https://auth0.com/docs/scopes/current/oidc-scopes) (attributes of the RS to which this token should have access). `open_id` is a scope that is needed to verify Alice's identity.
Copy link

Choose a reason for hiding this comment

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

[oidc scpes]
-> [OIDC scopes]

- `scope=open_id`: a list of [oidc scpes](https://auth0.com/docs/scopes/current/oidc-scopes) (attributes of the RS to which this token should have access). `open_id` is a scope that is needed to verify Alice's identity.
- `client_id=7243fd594bdcf9c71a9b902274afaa30`: indicates the id of the client. The value for this field should be obtained in the registration phase.
- `response_type=id_token%20token` indicates the desired response data. Note that you cannot use response types that were not previously indicated during registration.
- `request=eyJhbGciOiJub25lIn0.eyJyZWRpc...`: A JWT containing the public key of the client and signed by the client using the private key. This is unique to webId-oidc. We will eventually use this to generate our pop-token.
Copy link

Choose a reason for hiding this comment

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

webId-oidc
-> WebID-OIDC

- `client_id=7243fd594bdcf9c71a9b902274afaa30`: indicates the id of the client. The value for this field should be obtained in the registration phase.
- `response_type=id_token%20token` indicates the desired response data. Note that you cannot use response types that were not previously indicated during registration.
- `request=eyJhbGciOiJub25lIn0.eyJyZWRpc...`: A JWT containing the public key of the client and signed by the client using the private key. This is unique to webId-oidc. We will eventually use this to generate our pop-token.
- `request=eyJhbGciOiJub25lIn0.eyJyZWRpc...`: A JWT containing the public key of the client and signed by the client using the private key. This is unique to WebId-OIDC. We will eventually use this to generate our pop-token.
Copy link

Choose a reason for hiding this comment

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

hopefully clearer...
WebId
-> WebID

@jaxoncreed
Copy link
Contributor Author

Can we resurrect this and approve it @kjetilk. I keep sending links to this document because people keep asking how the login works and can't find this.

@kjetilk
Copy link
Member

kjetilk commented Jul 30, 2019

Can we resurrect this and approve it @kjetilk. I keep sending links to this document because people keep asking how the login works and can't find this.

Right! But as of now, it has no approving reviews... As I said above, I'd prefer that others provide reviews and then I review and merge it, because I don't feel up to the task of performing a technical review myself. So, @justinwb , @RubenVerborgh , @TallTed , @michielbdejong , please have another look and update your reviews.

@RubenVerborgh
Copy link

I can't; not knowledgeable enough about OIDC.

@kjetilk
Copy link
Member

kjetilk commented Jul 30, 2019

My problem too... I think the Authentication Panel should convene urgently to resolve this.

@dmitrizagidulin
Copy link
Member

@kjetilk

I think the Authentication Panel should convene urgently to resolve this.

I agree!

dmitrizagidulin
dmitrizagidulin previously approved these changes Aug 5, 2019
Copy link
Member

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

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

Read through this carefully on a call with @jaxoncreed.

Looks great, thank you SO much for doing this work, Jackson!

@@ -331,12 +331,12 @@ GET https://secureauth.example/authorize?scope=openid&client_id=7243fd594bdcf9c7

That URL might look a little complex, but it's essentially a request to `https://secureauth.example/authorize` with the following URL parameters:

- `scope=open_id`: a list of [OIDC scpes](https://auth0.com/docs/scopes/current/oidc-scopes) (attributes of the RS to which this token should have access). `open_id` is a scope that is needed to verify Alice's identity.
- `scope=openid`: a list of [OIDC scopes](https://auth0.com/docs/scopes/current/oidc-scopes) (attributes of the RS to which this token should have access). `open_id` is a scope that is needed to verify Alice's identity.
Copy link

Choose a reason for hiding this comment

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

`scope=open_id`

became

`scope=openid`

so

`open_id` is a scope

should become

`openid` is a scope

@justinwb justinwb self-requested a review August 5, 2019 16:43
@justinwb
Copy link
Member

justinwb commented Aug 5, 2019

Should link to the new document at application-workflow-detailed.md, but the links to this and application-workflow.md should be put in context based on the use case. File names may also need to be changed to align with that and prevent confusion.

Copy link
Member

@justinwb justinwb left a comment

Choose a reason for hiding this comment

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

Nice improvement to the current state of the documentation. There's still content required to provide full coverage, especially as it relates to the different patterns of use and how to apply them, but I think that this moves the ball forward in this current instantiation of the spec. The rest of the efforts should be channeled towards https://github.com/solid/specification. With that in mind, I'm good with approving these changes here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants