Closed nsklikas closed 3 years ago
scopes_mapping
(maybe simplyscopes
is a better name?)
this is a structure that is mapping scopes to claims. I would call it scopes_to_claims
.
advertised_scopes
: A list with the scopes that will be advertised in the well-known endpoint
[...] as "scopes_supported", right? so, probably this should be called scopes_supported
scopes_supported
(maybeallowed_scopes
is better?): A list with the scopes that are allowed to be used
these are allowed to be requested, so probably allowed_scopes
is better.
(using my naming: scopes_supported
and allowed_scopes
)
It seems to me that scopes_supported
is always a subset of allowed_scopes
.
Otherwise, what happens if the two sets are totally different?
Maybe, we should have scopes_supported
and scopes_supported_extras
: then it is clear that the _extras are scopes that are supported but not advertised/shown on the well-known endpoint.
Would that make sense?
Hi @c00kiemon5ter I already merged the related PR to this issue but feel free to get a @nsklikas 's feedback on this topic and doing another PR for improve the semantic of those variables
I agree with your naming (scopes_to_claims
, scopes_supported
, allowed_scopes
), I will change it.
About allowed_scopes
vs scopes_supported_extras
, I prefer allowed_scopes
because it allows for a similar behavior globally and per client.
Also consider this:
You support 3 scopes A
, B
and C
and you wish to advertise them all, but you want to allow most of your clients to use only scopes A
and B
and only a few to use C
. In this case you would have to use:
scopes_supported = ["A", "B", "C"]
allowed_scopes
in ALL clients' metadata.But this configuration would allow you to define (using your naming):
scopes_supported = ["A", "B", "C"]
allowed_scopes = ["A", "B"]
allowed_scopes = ["C"]
to only the few clients that require this scope.Of course both these approaches are equivalent. I think it is useful to be able to advertise more scopes than the ones you support (globally), but if you wish (and the others agree) I can change it.
ah, my thinking was a single global setting, not per-client. You are right, _extras do not help in this case.
I will use this issue to try to describe how I think we should handle scopes. When we agree to something I will go forward and open a PR.
Right now we have
scope_supported
which is used to define the scopes that are advertised in the well-known endpoint as well as the scopes that we allow. This can be overridden per client with theallowed_scopes
parameter. If someone wants to override the scopes mapping then he must use the custom_scopes add on.We would like to be able not to advertise all the scopes that are supported. Also I think that it would be nice to have a scopes mapping per client. So I propose that remove the existing scopes configuration and introduce the following:
scopes_mapping
(maybe simplyscopes
is a better name?): A dict defining the scopes that are allowed to be used per client and the claims they map to (defaults to the scopes mapping described in the spec). If we want to define a scope that doesn't map to claims (e.g. offline_access) then we simply map it to an empty list. E.g.:scopes_supported
(maybeallowed_scopes
is better?): A list with the scopes that are allowed to be used (defaults to the keys inscopes_mapping
).advertised_scopes
: A list with the scopes that will be advertised in the well-known endpoint (defaults toscopes_supported
).In addition to those configuration options we should also have the corresponding per client ones for
scopes_supported
andscopes_mapping
.@rohe @peppelinux what do you think? Should I go ahead and implement this?