You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Dec 13, 2018. It is now read-only.
Repository navigation
This repository was archived by the owner on Dec 13, 2018. It is now read-only.
Discussion for Scope being a list and not a string? #614
Was there ever any discussion as to why Scope on the OIDC middleware is a collection and not a string? ResponseType is still a string. It feels cumbersome. Anyone else feel the same way?
The most important reason was for consistency: the OAuth2 generic middleware used a list of strings and the OIDC client middleware a single string. I opted for IList<string> because it was the most appropriate choice for the OAuth2 generic middleware, whose inheritors have to deal with non-standard implementations (Facebook, we hate you for using coma-separated scopes... 😄). Using a list hides how the scope is finally formatted by the specific implementation and makes the way users deal with scope consistent across the social providers.
Well, screw all of that, IMO. The katana OAuth2 MW is dead. And you know exactly what the OIDC and OAuth2 specs say for the scope. If the aberrant implementations needs something different, then do it in their own implementation.
The job of frameworks is to make the life of the consumer easier, not the life of the implementer.
Well, screw all of that, IMO. The katana OAuth2 MW is dead.
There was no OAuth2 generic middleware in Katana, so not sure to understand what you mean.
And you know exactly what the OIDC and OAuth2 specs say for the scope.
Both specs only state that scope is space-separated string, they don't mention anything about how we should implement it internally.
If the aberrant implementations needs something different, then do it in their own implementation.
The job of frameworks is to make the life of the consumer easier, not the life of the implementer.
That's exactly what we do in the virtual FormatScope method. But it seems you totally missed my point. If we used a list, consumers would have to use different scopes patterns for Facebook and Google:
options.Scope="scope1,scope2";// Facebook, Reddit.options.Scope="scope1 scope2";// Google (and the other standard providers)
Using a list abstracts the way the scope is formatted:
There was no OAuth2 generic middleware in Katana, so not sure to understand what you mean.
I was referring to the Katana OAuth2 authorization server middleware. Is that not what you mentioned above when you said that the point was for consistency? Perhaps we're not talking about the same thing.
Other complaints:
It's not obvious what the defaults are (probably something that is AAD focused).
It's not consistent with the other settings (again, response_type)
Most people that use the OIDC middleware don't implement the OAuth2 katana middleware, so the consistency there is not obvious. (this is my comment above so perhaps we're not talking about the same thing)
It's not consistent with how it was in Katana. I already noticed another issue on the issue tracker asking why Scope was no longer the way it used to be.
I know it's something small. But I teach this stuff to a lot of developers each year, and yet another little oddity like this requires yet another 5 minutes of explanation (or discovery on their part). Not time well spent (again for the consumer).
Again, I'm sure this doesn't matter in the end. I'm just raising the point that when I'm using this MW as a consumer I'm pretty sure that 99% of the time I will need to write this code:
And then i'm not really sure what I'm getting (or maybe the next dev on the project who inherits the code).
So my argument here is for clarity and simplicity for the consuming developer. I felt the string based approach in the Katana OIDC middleware achieved that.
I was referring to the Katana OAuth2 authorization server middleware. Is that not what you mentioned above when you said that the point was for consistency? Perhaps we're not talking about the same thing.
Hell no. By "OAuth2 generic middleware", I meant this thing. It's a whole new middleware, designed to support virtually any OAuth2 server. You can either subclass it or use the generic notifications. All the social providers inherit from OAuthMiddleware, including the 24 providers developed by the community.
As I said I already feel it's the aberrant implementation's fault if they don't follow the RFC/specs. This is the point of OIDC -- the fix the crap people did with OAuth2 and then called it authentication. But I digress.
The reason for opening the issue stands: I think the design is more important for the consumer (for clarity and simplicity) rather than the implementer.
It's not consistent with the other settings (again, response_type)
One could argue that response_type has only predefined (standard) sets that allow no customization (unlike scope, whose values are totally implementations-specific).
It's not consistent with how it was in Katana. I already noticed another issue on the issue tracker asking why Scope was no longer the way it used to be.
There are many things that have been updated in vNext. This change is relatively explicit IMHO.
And then i'm not really sure what I'm getting (or maybe the next dev on the project who inherits the code). So my argument here is for clarity and simplicity for the consuming developer. I felt the string based approach in the Katana OIDC middleware achieved that.
True. But on the other hand, it allows customers to only focus on the provider-specific scopes, without having to re-specify the standard ones. IMHO, it's clearer and simpler.
You have valid points, but I don't think using a string is fundamentally better than using a list. I've reviewed ~20 providers for aspnet-contrib and many specify custom scopes in the options classes. Until now, I've never heard someone complain about the scope thingy (well, except you, obviously 😄).
Fair enough. I'd just like someone at Microsoft to spend some time thinking about it. I know it's late, and I know this is a nitpicky request, but I wanted some attention spent on it.
The response_type comparison is interesting. It appears to be primarily single-value except for extensions. We only support code, id_token, and code id_token, and we use constants for those (OpenIdConnectResponseTypes).
B) I agree that the defaults need to be more transparent.
Either way we need better doc comments that explain the defaults. Google, MSA, and OIDC have defaults. Facebook doesn't. I don't think it's relevant to Twitter.
The only default that makes sense to me is "openid" (since it's the OIDC MW). I don't think it matters where -- either in options ctor, or in MW ctor.
Also, you should consider calling Distinct() on the scopes list before sending them to the OP -- currently they're duplicated in the authorization request if they're in the list multiple times. That's what apps will do when they don't know what the defaults are.
For what it's worth, I unfortunately spent the last couple of days proving some of @brockallen 's points related to scopes and defaults. The default in the OpenIDConnectOptions includes "profile" which pulls everything (at least in the case of Auth0). Auth0 provides a lot of claims. Enough claims to overwhelm Safari mobile's cookie domain size limits, which results in an obscure CookieChunkingManagerError for users using mobile browsers.
The default of "profile" definitely burned me in this case and it took a lot of digging to figure out how to change the scopes. Changing the scopes is clumsy as @PinpointTownes pointed out (and not easily discoverable when you are trying to learn all the other new stuff).
I am not sure if there is anything actionable because removing "profile" would have its own set of issues, but at the very least, I think others can learn from my mistake.
@leastprivilege and I just did a workshop this week on this topic and it was somewhat painful to do scopes as items in the collection. The API today favors the middleware implementer and not the consumer. Even now post-RC2 I think it'd still be nice to have scope as a single string.
Activity
kevinchalet commentedon Dec 13, 2015
No discussion, but since I'm the guy who replaced the
Scopeproperty by a list, I can shed some light on the subject 😄The most important reason was for consistency: the OAuth2 generic middleware used a list of strings and the OIDC client middleware a single string. I opted for
IList<string>because it was the most appropriate choice for the OAuth2 generic middleware, whose inheritors have to deal with non-standard implementations (Facebook, we hate you for using coma-separated scopes... 😄). Using a list hides how thescopeis finally formatted by the specific implementation and makes the way users deal withscopeconsistent across the social providers.brockallen commentedon Dec 14, 2015
Well, screw all of that, IMO. The katana OAuth2 MW is dead. And you know exactly what the OIDC and OAuth2 specs say for the scope. If the aberrant implementations needs something different, then do it in their own implementation.
The job of frameworks is to make the life of the consumer easier, not the life of the implementer.
kevinchalet commentedon Dec 14, 2015
There was no OAuth2 generic middleware in Katana, so not sure to understand what you mean.
Both specs only state that
scopeis space-separated string, they don't mention anything about how we should implement it internally.That's exactly what we do in the virtual
FormatScopemethod. But it seems you totally missed my point. If we used a list, consumers would have to use different scopes patterns for Facebook and Google:Using a list abstracts the way the scope is formatted:
brockallen commentedon Dec 14, 2015
I was referring to the Katana OAuth2 authorization server middleware. Is that not what you mentioned above when you said that the point was for consistency? Perhaps we're not talking about the same thing.
Other complaints:
I know it's something small. But I teach this stuff to a lot of developers each year, and yet another little oddity like this requires yet another 5 minutes of explanation (or discovery on their part). Not time well spent (again for the consumer).
Again, I'm sure this doesn't matter in the end. I'm just raising the point that when I'm using this MW as a consumer I'm pretty sure that 99% of the time I will need to write this code:
So I know exactly what I'm getting. Otherwise the code looks like:
And then i'm not really sure what I'm getting (or maybe the next dev on the project who inherits the code).
So my argument here is for clarity and simplicity for the consuming developer. I felt the string based approach in the Katana OIDC middleware achieved that.
kevinchalet commentedon Dec 14, 2015
Hell no. By "OAuth2 generic middleware", I meant this thing. It's a whole new middleware, designed to support virtually any OAuth2 server. You can either subclass it or use the generic notifications. All the social providers inherit from
OAuthMiddleware, including the 24 providers developed by the community.brockallen commentedon Dec 14, 2015
As I said I already feel it's the aberrant implementation's fault if they don't follow the RFC/specs. This is the point of OIDC -- the fix the crap people did with OAuth2 and then called it authentication. But I digress.
The reason for opening the issue stands: I think the design is more important for the consumer (for clarity and simplicity) rather than the implementer.
kevinchalet commentedon Dec 14, 2015
One could argue that
response_typehas only predefined (standard) sets that allow no customization (unlikescope, whose values are totally implementations-specific).There are many things that have been updated in vNext. This change is relatively explicit IMHO.
True. But on the other hand, it allows customers to only focus on the provider-specific scopes, without having to re-specify the standard ones. IMHO, it's clearer and simpler.
You have valid points, but I don't think using a string is fundamentally better than using a list. I've reviewed ~20 providers for aspnet-contrib and many specify custom scopes in the options classes. Until now, I've never heard someone complain about the
scopethingy (well, except you, obviously 😄).brockallen commentedon Dec 14, 2015
Fair enough. I'd just like someone at Microsoft to spend some time thinking about it. I know it's late, and I know this is a nitpicky request, but I wanted some attention spent on it.
Tratcher commentedon Dec 14, 2015
A) I prefer the List approach as it lets us worry about the formatting. It's even defined as a list in the spec https://tools.ietf.org/html/rfc6749#section-3.3.
code,id_token, andcode id_token, and we use constants for those (OpenIdConnectResponseTypes).B) I agree that the defaults need to be more transparent.
Tratcher commentedon Dec 14, 2015
We have two different ways to do defaults right now.
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectOptions.cs#L163
vs
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Google/GoogleMiddleware.cs#L72-L77
Which looks more useful?
Either way we need better doc comments that explain the defaults. Google, MSA, and OIDC have defaults. Facebook doesn't. I don't think it's relevant to Twitter.
kevinchalet commentedon Dec 14, 2015
Personally, option 1) has always been my favorite approach (#257 (comment)).
brockallen commentedon Dec 14, 2015
The only default that makes sense to me is "openid" (since it's the OIDC MW). I don't think it matters where -- either in options ctor, or in MW ctor.
Also, you should consider calling Distinct() on the scopes list before sending them to the OP -- currently they're duplicated in the authorization request if they're in the list multiple times. That's what apps will do when they don't know what the defaults are.
[-]Discussion for Scope beign a list and not a string?[/-][+]Discussion for Scope being a list and not a string?[/+]Eilon commentedon Jan 14, 2016
For the defaults we should do (1) where the defaults are initialized in the list, and it's up to the app developer to add/remove/clear/whatever.
kevinchalet commentedon Jan 14, 2016
@Eilon now that "delegate-style" middleware extensions have all been removed, clearing/removing a property list is really ugly 😄
Before:
Now:
smbecker commentedon Jan 15, 2016
You could always add an extension method to simplify this case. Something like:
With usage:
#614 Standardize the pattern for scope defaults.
#614 Standardize the pattern for scope defaults.
npnelson commentedon May 19, 2016
For what it's worth, I unfortunately spent the last couple of days proving some of @brockallen 's points related to scopes and defaults. The default in the OpenIDConnectOptions includes "profile" which pulls everything (at least in the case of Auth0). Auth0 provides a lot of claims. Enough claims to overwhelm Safari mobile's cookie domain size limits, which results in an obscure CookieChunkingManagerError for users using mobile browsers.
The default of "profile" definitely burned me in this case and it took a lot of digging to figure out how to change the scopes. Changing the scopes is clumsy as @PinpointTownes pointed out (and not easily discoverable when you are trying to learn all the other new stuff).
I am not sure if there is anything actionable because removing "profile" would have its own set of issues, but at the very least, I think others can learn from my mistake.
brockallen commentedon May 19, 2016
@leastprivilege and I just did a workshop this week on this topic and it was somewhat painful to do scopes as items in the collection. The API today favors the middleware implementer and not the consumer. Even now post-RC2 I think it'd still be nice to have scope as a single string.