-
Notifications
You must be signed in to change notification settings - Fork 11
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
Finetune OpenID Connect scope documentation #288
Conversation
I pushed another related commit:
feel free to finetune further |
I agree with most of this, thanks. I'm not sure about the scopes. I think the piece to fix here is the scopes requirement as the description also doesn't list the scopes: "For each OpenID provider, a name and the issuer location MUST be listed." It doesn't say scopes is required and the scopes description basically allows it being empty. Thus some suggestions below. Please also add a CHANGELOG entry referring to this PR. |
already pushed some updates (all to be squashed before merging) some more notes
Indeed, currently
This allows a backend to specify a non-empty list without |
openapi.yaml
Outdated
A description and related links can OPTIONALLY be added. The [issuer | ||
location](https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig) | ||
(also referred to as 'authority' in client libraries) is the URL of the | ||
For each OpenID provider, an id and the issuer location MUST be listed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each OpenID provider, an id and the issuer location MUST be listed. | |
For each OpenID provider, the issuer location with an id and a title MUST be listed. |
Thanks.
I now realized that name was referring to title. So to be very explicit, we should list id, title and issuer.
What exactly? The first paragraphs are just an overview and I think the information in there is already part of the fields, isn't it?
Oh yes, add that, please. I just checked whether we can also enforce this with the schema, but OpenAPI unfortunately doesn't allow that. JSON Schema has the contains keyword, but it's not part of OpenAPI yet. We could add x-contains as reminder for later (JSON Schema is on its way) though: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#specificationExtensions that would be
|
I just mean that it looks redundant and prone to inconsistencies to have the same information:
I would propose to move the whole paragraph of the "issuer location" that's currently in the endpoint description to the description of the But, I don't feel very strong about this, it's just a minor suggestion to streamline things a bit |
Yes, that works for me. Merge it, please. :-) |
It's still like in my original PR, so I'll leave it like that.
One could argue that it's not that big of an issue because leaving out |
I think your proposed "If scopes are specified, the list MUST at least contain the
I think if we have a requirement in text, we should try to enforce them as much as possible in the schemas. Otherwise we could also just leave it out of the text as well and just refer to OpenID Connect. But I'm not strongly feeling about this workaround. |
ok, squashed everything and force-pushed |
Thank you! |
https://openeo.org/documentation/1.0/developers/api/reference.html#operation/authenticate-oidc
under
/credientials/oidc
, thescopes
fields:This is bit confusing: it is required on one hand, but can be empty (in which case default scopes must be used)
I'm also not sure what default scopes mean in this context, but as far as I know, only
openid
is a required scope to enable OpenID Connect. I would just simplify the doc to state that.