Allow more extension points for GoogleAuthenticationMiddleware

[bahrens]

Currently, the GoogleAuthenticationMiddleware only allows the requested the claims to be set once for all requests via the GoogleAuthenticationOptions. We are implementing a "multi-tenant" solution which requires us to request different scopes per tenant. It would be nice if there were hooks to allow for more control over scopes on a per request basis. In addition to requesting scopes per tenant, Google has URI parameters that can be set that we'd need to also vary per tenant. Some items are internal (like the GoogleAuthenticationHandler) which makes it difficult to override.

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Google/GoogleAuthenticationMiddleware.cs#L19

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Google/GoogleAuthenticationHandler.cs#L17

[Tratcher]

There is some support for dynamic scopes. You can set them in the AuthenticationProperties collection used with the Challenge. See https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Google/GoogleAuthenticationHandler.cs#L88

[bahrens]

Can the AuthenticationProperties collection be set somewhere? The GoogleAuthenticationHandler is internal, so it can't be inherited from in order to override the BuildChallengeUrl method.

[Tratcher]

It is set per request via HttpContext.Authentication.Challenge:
https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.Http.Abstractions/Authentication/AuthenticationManager.cs#L33
Are you working with MVC or some other component?

[edwinf]

@Tratcher we're working with IdentityServer 3, which doesn't allow us to set any additional Authentication Properties before issuing the challenge. (I'm going to try to put a PR out to change that). A delegate / interface as mentioned in #44 would allow us to access the options without having to modify the layer sitting on top of this middleware, thus making it more plug and play.

Besides the scope we'd also want to adjust the challenge URL occasionally to include the [hd parameter | https://developers.google.com/identity/protocols/OpenIDConnect#hd-param], which is not possible at the moment with the current implementation. We could definitely write a PR for that one, but since people might have put it on their AuthorizationEndpoint already, backwards compatibility on the query string would have to be accounted for.

Given those two points, we were hoping that the Handlers could be made public, so we could easily extend them and override just the specific pieces that we'd need to modify, as the majority of users won't need them.

If there's a different way that the team would like to see to address the above items, we'd be more then willing to write up a PR for your consideration with your direction on the design.

[Tratcher]

It wouldn't be enough to make the handler public for extensions. You'd also have to extend the middleware class to create your new handler, and copy the IApplicationBuilder extensions to create your new middleware. At that point you'd almost be better off forking the code and modifying it rather than trying to extend the existing one.

Another approach would be to try exposing more extension hooks via the Options.Notifications. e.g. https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/Notifications/OAuthAuthenticationNotifications.cs#L37
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Google/Notifications/GoogleAuthenticationNotifications.cs#L26

[edwinf]

Good point,

what if , based on this comment the query string parameter dictionary was refactored into the OAuthAuthenticationHandler base class, and then an extension hook was added to allow you to modify the dictionary before it was returned as the fully formatted uri.

Something like this?
https://github.com/edwinf/Security/commit/ad09136ad3738c74febd5a5a84e7586483753193

[CrustyJew]

Just like to add my voice to the desire for this feature as well. I think the implementation for overriding properties as defined in the Google Handler would work perfectly for my needs.

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Google/GoogleHandler.cs#L88

[Eilon]

Closing because this issue is from before the 1.0 release, and a lot of this functionality might already be possible via other means.