Skip to content
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

Asp Mvc Core support #1109

Closed
wants to merge 2 commits into from
Closed

Asp Mvc Core support #1109

wants to merge 2 commits into from

Conversation

buzallen
Copy link

@buzallen buzallen commented Nov 4, 2017

Notes on the PR -

  • I added a project 'Google.ApisAuth.AspMvcCore'
  • Used the namespace 'Google.Apis.Auth.OAuth2.AspMvcCore'
  • Moved the target to netstandard 1.6 for this project (needed for some references)
  • I couldn't find a replacement in core for the '[AsyncTimeout(10000)]' attribute on the IndexAsync action on the AuthCallbackController class.
  • When parsing the query string that was returned in the 'AuthorizationCodeActionFilter' the error keys were not in the dictionary (not sure if this is intentional) but it caused an exception so I first checked if the key exists. This was not an issue in the mvc 5 version.

Thanks,
Brian

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 4, 2017
@chrisdunelm
Copy link
Contributor

@buzallen Thanks for this PR.
We are currently in the process of working out how we are going to support ASP.NET core; and we're aware that we currently offer no packages, examples, or docs for auth on ASP.NET core.
There is no fixed timetable for these decisions, but we expect to have a better idea within 3 or 4 weeks; so unfortunately we won't be able to properly comment on this PR or consider it for merging for a little while.

@buzallen
Copy link
Author

buzallen commented Nov 4, 2017

Thanks. Glad to know something is coming.

@jskeet
Copy link
Collaborator

jskeet commented Jan 26, 2018

Apologies that we've dropped the ball on this. I can give it a first pass today, but I suspect we'll need Chris's input too (and he's out today).

Auth in general is still a bit up in the air, but I think we really do need this package.

@jskeet
Copy link
Collaborator

jskeet commented Jan 26, 2018

(One thing we'll need to consider is whether to target ASP.NET Core 1.x, 2.x or both - I'll have to do some testing to check what happens.)

@jskeet
Copy link
Collaborator

jskeet commented Jan 26, 2018

Right, I've now knocked together a little test app, and the good news is that it works, pretty much following the regular MVC instructions.

In terms of the code itself, assuming we're happy to deviate from the design of the existing package:

  • I'd like to move ParseRequest into AuthorizationCodeActionFilter unless there's a good reason for it to be public
  • I'd like to revisit the design of AuthCallbackController, at least renaming it to have a Base suffix so it doesn't need to be specified with a fully-qualified name. I'd probably add a constructor to accept a flow metadata to work with DI.
  • I suspect we don't need FlowMetadata to be abstract. The only bit that's likely to need logic is fetching the user ID... we could represent that with a delegate, maybe.
  • There's a TODO from years ago about integrating with an MVC authorize attribute; I'd like to at least investigate that further

In short: I'm really grateful that you've got this working, and it's a great first step. I don't want to merge it in yet though, and I definitely want to consult with Chris about his thoughts. I'd also like to spend some time looking at what other auth providers do - if there's a convention we can follow to simplify things, we really should do so.

Now that it's on our radar again, I'll push to keep some momentum. If you see me dropping the ball again, please holler.

@chrisdunelm
Copy link
Contributor

Might we be better to use the Google authentication flow built in to ASP.NET Core, rather than using our 'custom' flow?

I have a test app (in ASP.NET Core 2) that uses the standard services.AddAuthentication().AddGoogle(...) in ConfigureServices in Startup.cs, which extracts the OAuth access token and creates a GoogleCredential.
This uses existing ASP.NET Core code to manage state and refresh, etc; therefore integrates in all the expected ways; and, I assume, would fit better into most ASP.NET Core apps?

I have not, however, ever written a "real" ASP.NET Core application that includes authentication, so it's quite likely I've missed or misunderstood something. If this is the case, I'm very happy to be corrected.

@jskeet
Copy link
Collaborator

jskeet commented Jan 31, 2018

That sounds infinitely better, yes :) I haven't looked at the auth flow in ASP.NET Core yet. Let's look at that code together in person.

@chrisdunelm
Copy link
Contributor

#1163 is the beginning of an implementation using the OpenIdConnect flow built into ASP.NET Core; which is the approach I'd prefer.

@strich
Copy link

strich commented Feb 24, 2018

Hey folks. Is this still in progress? As it stands right now I'm really struggling to get even the most basic auth going in a .NET Core ASPNET project.

@chrisdunelm you mention getting it working with the official Microsoft.AspNetCore.Authentication.Google package? This would be by far the most elegant if so! Can you provide the demo project you made for it?

@gijswijs
Copy link

I can confirm that this works. I prefer the approach of using OpenIdConnect, but because of reasons, it turned out that I had to use this implementation. It was quite the struggle to get everything to work, but I managed.

@chrisdunelm
Copy link
Contributor

Closing, as superseded by #1163.

@chrisdunelm chrisdunelm closed this Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants