-
Notifications
You must be signed in to change notification settings - Fork 599
Discussion for Scope being a list and not a string? #614
Comments
No discussion, but since I'm the guy who replaced the 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 |
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. |
There was no OAuth2 generic middleware in Katana, so not sure to understand what you mean.
Both specs only state that
That's exactly what we do in the virtual 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: options.Scope.Add("scope1");
options.Scope.Add("scope2"); |
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. |
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 |
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. |
One could argue that
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 |
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. |
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.
B) I agree that the defaults need to be more transparent. |
We have two different ways to do defaults right now.
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. |
Personally, option 1) has always been my favorite approach (#257 (comment)). |
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 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. |
@Eilon now that "delegate-style" middleware extensions have all been removed, clearing/removing a property list is really ugly 😄 Before: app.UseOpenIdConnectAuthentication(options => {
options.Scope.Clear();
options.Scope.Add("custom")
}); Now: var openIdConnectOptions = new OpenIdConnectOptions();
openIdConnectOptions.Scope.Clear();
openIdConnectOptions.Scope.Add("custom");
app.UseOpenIdConnectAuthentication(openIdConnectOptions); |
You could always add an extension method to simplify this case. Something like: public static class OpenIdConnectOptionsExtensions
{
public static OpenIdConnectOptions ResetScope(this OpenIdConnectOptions options, params string[] scopes) {
options.Scope.Clear();
foreach (var scope in scopes) {
options.Scope.Add(scope);
}
return options;
}
} With usage: app.UseOpenIdConnectAuthentication(new OpenIdConnectOptions().ResetScope("custom1", "custom2")); |
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. |
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 text was updated successfully, but these errors were encountered: