Auth 2.0: Add Auth Abstractions, Core by HaoK · Pull Request #798 · aspnet/HttpAbstractions

HaoK · 2017-03-21T20:22:22Z

Should I start to obsolete the old Http.Authentication at the same time?

HaoK · 2017-03-21T20:24:23Z

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public interface ISecureDataFormat<TData>


The Data interfaces are kinda specific to our Security repo implementations, these could live inside of the Security auth package instead... Thoughts @Tratcher ?

It's only used on Options types, right? Then it's fine to move to the concrete package.

👍 for moving the "data handling" back to the security repo 😄

Tratcher · 2017-03-21T21:15:51Z

They won't work anymore right? Then yes, start Obsoleting them.

Tratcher · 2017-03-21T21:27:53Z

+        /// </summary>
+        /// <param name="app">The <see cref="IApplicationBuilder"/> to add the middleware to.</param>
+        /// <returns>A reference to this instance after the operation has completed.</returns>
+        public static IApplicationBuilder UseAuthentication(this IApplicationBuilder app)


Why did Auth.Core have to move to HttpAbstractions?

Do we want to be able to test auth without the security infrastructure, i.e. IISIntegration would like to add auth but not depend on Security

So I moved the 'Core' pieces that are needed for the auth infrastructure service here. But the scheme specific helpers and base classes are still in Security.

So you can add schemes, and call into auth via what's in HttpAbstractions, but all the sugar around implementing an auth middleware lives in the Security PR (still to be recreated)

kevinchalet · 2017-03-23T19:41:58Z

Now that an important part of the new security stuff is back in this repo, is there a particular reason the authentication methods (AuthenticateAsync/ChallengeAsync and co.) couldn't be first-class citizen and added directly in the Http.Abstractions package, like what we used to have in 1.0.0 with IAuthenticationManager?

HaoK · 2017-03-23T19:43:31Z

@PinpointTownes extensions on HttpContext aren't sufficient?

HaoK · 2017-03-23T19:44:25Z

Altho there is a bit of wierdness in the new stuff that I don't like today, that you have to call AddAuthenticationCore for context.Authenticate to not blow up...

kevinchalet · 2017-03-23T19:46:36Z

@PinpointTownes extensions on HttpContext aren't sufficient?

They are. I'm just wondering if there's a technical reason for opting for extensions in this case 😄

HaoK · 2017-03-23T19:50:08Z

@PinpointTownes did you have an alternative in mind? We expect this to be mostly called by infrastructure/framework code like Security repo stuff/Identity/Authorize code.

HaoK · 2017-03-23T19:51:25Z

I went with extensions mostly to keep roughly the same shape as the old context.Authentication.Authenticate look and feel. As it made updating the existing usages very easy in a Find/Replace.

kevinchalet · 2017-03-23T19:55:48Z

@PinpointTownes did you have an alternative in mind?

I suppose you could keep the existing IAuthenticationManager as-is and simply update DefaultAuthenticationManager to use your IAuthenticationService (resolved via HttpContext.RequestServices)... or better, you could merge them.

It would have a pro: you wouldn't have to deprecate the existing abstractions.

HaoK · 2017-03-23T19:57:33Z

We can't touch the existing old interfaces as that's the interop story for the old 1.X packages still continuing to work in a new MVC. They will still work since they are using the existing unchanged IAuthenticationHandler and the old packages are still running the old code

kevinchalet · 2017-03-23T19:57:35Z

We expect this to be mostly called by infrastructure/framework code like Security repo stuff/Identity/Authorize code.

Plus MVC, since it relies on these methods for SignInResult/SignIn and co.

HaoK · 2017-03-23T19:58:27Z

Yeah...so basically the main consumer of the new APIs is myself :)

kevinchalet · 2017-03-23T20:00:37Z

We can't touch the existing old interfaces as that's the interop story for the old 1.X packages still continuing to work in a new MVC.

Hum my bad, I thought these things were supposed to be removed in 2.0.

It's a bit strange tho'... as there's no guarantee that code written for 1.0 will still compile with 2.0 (that's even the opposite, otherwise you wouldn't increase the major version, right?)

HaoK · 2017-03-23T20:03:44Z

Yeah so clearly 1.0 code won't compile, but 1.0 packages should still work/run in a 2.0 app as there's binary compat. Hence we can't really touch the HttpAbstractions auth behavior if we want those to still work... They can continue to party there like its 2016

kevinchalet · 2017-03-23T20:05:29Z

but 1.0 packages should still work/run in a 2.0 app as there's binary compat.

Really? So how do you explain things like openiddict/openiddict-core#359 (code written for EF Core 1.0 no longer works with 1.2/2.0 due to a signature change) or aspnet/SignalR#155 ?

HaoK · 2017-03-23T20:10:08Z

So, my guess is the reality will be more complicated yes :)

HaoK · 2017-03-23T21:40:53Z

Obsoleted old Auth APIs

HaoK · 2017-03-28T00:06:52Z

@davidfowl How do things look here? I'd like to start getting the additive changes in this week (Auth.Abstractions + Auth.Core). There's still one change that I'm not sure about, which is that Auth is now an optional service. A call to services.AddAuthenticationCore() is required for context.AuthenticateAsync to work. This is unlike the old HttpAbstractions.Authentication which always supported context.Authentication.Authenticate.

We could have hosting call AddAuthenticationCore similar to AddOptions, otherwise this is something that needs to be called during ConfigureServices, see aspnet/HttpSysServer@c57b98b#diff-2d7f0bf656ca8cab31fc9edcd837ee98R62

Tratcher · 2017-03-29T15:10:52Z

+        public static Task SignInAsync(this HttpContext context, string scheme, ClaimsPrincipal principal, AuthenticationProperties properties) =>
+            context.RequestServices.GetRequiredService<IAuthenticationService>().SignInAsync(context, scheme, principal, properties);
+
+


nit: extra line

Tratcher · 2017-03-29T15:12:15Z

+        /// <summary>
+        /// Returns the schemes in the order they were added (important for request handling priority)
+        /// </summary>
+        public IEnumerable<AuthenticationSchemeBuilder> Schemes


syntax
Schemes => _schemes;

Tratcher · 2017-03-29T15:15:41Z


 namespace Microsoft.AspNetCore.Http.Features.Authentication
 {
+    [Obsolete("See https://go.microsoft.com/fwlink/?linkid=845470")]


I thought you weren't going to check these Obsolete's in until after you got all of the other repos converted?

When I smush all the changes together for the merge, I won't check in anything outside of the 2 new assemblies, so you don't need to review anything outside of the Authentication packages in this PR

Tratcher · 2017-03-29T15:18:45Z

I think it's OK for AuthenticateAsync to not work until you add an auth middleware / services. We just need to make sure the caller (the AuthZ filter) gives you a good error message.

Tratcher · 2017-03-29T15:51:05Z

+        /// </summary>
+        public AuthenticationTicket Ticket { get; private set; }
+
+        // REVIEW: should we also remember AuthenticationScheme?


AuthenticationScheme will be in the AuthenticationTicket for success, and doesn't matter much otherwise. The caller already knows the scheme.

Tratcher · 2017-03-29T15:53:58Z

+            }
+        }
+
+        public IDictionary<string, AuthenticationSchemeBuilder> SchemeMap { get; } = new Dictionary<string, AuthenticationSchemeBuilder>(); // case sensitive?


Tratcher · 2017-03-29T15:59:59Z

+            SchemeMap[name] = builder;
+        }
+
+        public void ConfigureScheme(string name, Action<AuthenticationSchemeBuilder> configureBuilder)


Where is ConfigureScheme used? I couldn't find it in this PR or Security. What's the scenario? Otherwise it seems redundant with AddScheme.

Yeah we don't need this anymore, it was needed before we had named options, as it was used to configure the options that lived in the scheme.settings. Good catch I'll remove this

Tratcher · 2017-03-29T16:06:34Z

+        /// <param name="principal">the <see cref="ClaimsPrincipal"/> that represents the authenticated user.</param>
+        /// <param name="properties">additional properties that can be consumed by the user or runtime.</param>
+        /// <param name="authenticationScheme">the authentication middleware that was responsible for this ticket.</param>
+        public AuthenticationTicket(ClaimsPrincipal principal, AuthenticationProperties properties, string authenticationScheme)


We're inconsistent about the type of things called authentication scheme. In some places it's a AuthenticationScheme, and in others it's just a string. Should the strings be called authenticationSchemeName? DefaultAuthenticationSchemeName? Or is that just overkill?

Well we called it authenticationScheme historically, so I think that should be familiar enough, do you think its confusing currently? I think the type is enough to differentiate...

Tratcher · 2017-03-29T16:10:46Z

+{
+    public interface IAuthenticationSchemeProvider
+    {
+        Task<IEnumerable<AuthenticationScheme>> GetAllSchemesAsync();


Time to fill in the comments?

Yeah, I'll fill in the comments today

Tratcher · 2017-03-29T16:49:08Z

+
+  <PropertyGroup>
+    <Description>ASP.NET Core common types used by the various authentication middleware components.</Description>
+    <VersionPrefix>1.2.0</VersionPrefix>


extract versions

Tratcher · 2017-03-29T16:49:21Z

+  <PropertyGroup>
+    <Description>ASP.NET Core common types used by the various authentication middleware components.</Description>
+    <VersionPrefix>1.2.0</VersionPrefix>
+    <TargetFrameworks>net451;netstandard1.3</TargetFrameworks>


Tratcher · 2017-03-29T16:52:35Z

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public class AuthenticationMiddlewareTests


AuthenticationMiddleware moved back to Security

Tratcher · 2017-03-29T16:54:11Z

+
+namespace Microsoft.AspNetCore.Authentication
+{
+    public static class TestExtensions


Move back to Security?

Tratcher · 2017-03-29T16:54:32Z

+
+        }
+
+        //public class TestAuthHandler : IAuthenticationHandler


HaoK · 2017-03-29T23:41:20Z

Updated all doc comments, removed Forbid from IAuthenticationService, and addressed PR feedback, I think I got them all, but @Tratcher you should double check the important ones, since I fixed some that you noted in the Security PR.

Tratcher · 2017-03-30T22:50:21Z

-        }
-
+        /// <summary>
+        /// Used by as the default scheme by <see cref="IAuthenticationService.AuthenticateAsync(Http.HttpContext, string)"/>.


Http.HttpContext?

Feels a little bit weird to add usings for doc comments, but whatever :)

Tratcher · 2017-03-30T22:51:09Z

 namespace Microsoft.AspNetCore.Authentication
 {
+    /// <summary>
+    /// AuthenticationSchemes are basically a name for a specific <see cref="IAuthenticationHandler"/>


basically?

Tratcher · 2017-03-30T22:51:40Z

@@ -28,7 +37,14 @@ public AuthenticationScheme(string name, Type handlerType)
        }

        // TODO: add display name?


Time to remove TODOs and file followup bugs?

Yeah this one is already tracked so I can just remove

Tratcher · 2017-03-30T23:05:04Z

+                throw new ArgumentException(nameof(authenticationScheme));
+            }
+
+            var handler = await Handlers.GetHandlerAsync(httpContext, authenticationScheme);


If you're just using forms, social, or WsFed then there's one thing to sign in and out of (cookies).
If you're using Identity then there are several signouts and the app signs out of them all.
If you're using OIDC there are two signouts, and the app needs to choose which.
And if you're using Bearer then signout doesn't work anyways.

So having a default sign out scheme, or even defaulting to the sign in scheme would only help the first scenario which is less common. Forget I asked.

Tratcher · 2017-03-30T23:06:09Z

+    <ProjectReference Include="..\Microsoft.AspNetCore.Authentication.Abstractions\Microsoft.AspNetCore.Authentication.Abstractions.csproj" />
+    <ProjectReference Include="..\Microsoft.AspNetCore.Http\Microsoft.AspNetCore.Http.csproj" />
+    <ProjectReference Include="..\Microsoft.AspNetCore.Http.Extensions\Microsoft.AspNetCore.Http.Extensions.csproj" />
+    <PackageReference Include="Microsoft.Extensions.TaskCache.Sources" Version="1.2.0-*" PrivateAssets="All" />


Extract versions

Tratcher · 2017-03-30T23:07:42Z

+  </PropertyGroup>
+  <ItemGroup>
+    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.Authentication.Core\Microsoft.AspNetCore.Authentication.Core.csproj" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.0.0-*" />


Extract versions

HaoK · 2017-03-31T18:42:38Z

Good enough for initial merge @Tratcher ?

HaoK · 2017-03-31T18:58:43Z

Merged 13925be the new packages, will open a new PR with obsolete's which will be checked in at the same time we switch all of security over (RIP build)

HaoK · 2017-03-31T19:02:28Z

Obsolete PR #806

dnfclas added the cla-already-signed label Mar 21, 2017

HaoK commented Mar 21, 2017

View reviewed changes

Tratcher reviewed Mar 21, 2017

View reviewed changes

HaoK mentioned this pull request Mar 23, 2017

Authentication 2.0 Iteration 3 (with Names/OptionFactory) aspnet/Security#1151

Closed

Tratcher reviewed Mar 29, 2017

View reviewed changes

HaoK force-pushed the haok/auth2 branch from 58aefba to dff0c5f Compare March 29, 2017 19:57

HaoK added 5 commits March 29, 2017 12:57

Auth 2.0: Add Auth Abstractions, Core

9d26c4e

Add auth to sln

ea970a3

Cleanup

f87c88b

Move data back to security auth

027b65c

Move auth middleware back to Security

380d9cc

HaoK added 6 commits March 29, 2017 12:57

Obsolete old Auth

47fe3dd

Fixup

4f3ed5d

Fix rogue obsoletes

8dc98b9

Add doc comments

e17091a

PR fixes

a01fba5

Ordinal

a9e2842

HaoK force-pushed the haok/auth2 branch from dff0c5f to a9e2842 Compare March 29, 2017 20:27

HaoK added 2 commits March 29, 2017 13:29

PR cleanup

2a2327a

Add doc comments + cleanup

55f136b

Add missing comments

f57aacf

Tratcher reviewed Mar 30, 2017

View reviewed changes

HaoK added 2 commits March 31, 2017 11:29

Extract versions

763e589

Fix test csproj

4a1ffda

Tratcher approved these changes Mar 31, 2017

View reviewed changes

HaoK closed this Mar 31, 2017

HaoK mentioned this pull request Apr 13, 2017

The Grand Auth Redesign of 2017 aspnet/Security#1179

Closed

13 tasks done

natemcmaster deleted the haok/auth2 branch November 20, 2018 06:30

		public static Task SignInAsync(this HttpContext context, string scheme, ClaimsPrincipal principal, AuthenticationProperties properties) =>
		context.RequestServices.GetRequiredService<IAuthenticationService>().SignInAsync(context, scheme, principal, properties);

		@@ -28,7 +37,14 @@ public AuthenticationScheme(string name, Type handlerType)
		}

		// TODO: add display name?

Conversation

HaoK commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinchalet commented Mar 23, 2017

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

kevinchalet commented Mar 23, 2017

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

kevinchalet commented Mar 23, 2017

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

kevinchalet commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

kevinchalet commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

kevinchalet commented Mar 23, 2017

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

HaoK commented Mar 23, 2017

Uh oh!

HaoK commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinchalet commented Mar 23, 2017 •

edited

Loading

kevinchalet commented Mar 23, 2017 •

edited

Loading

HaoK commented Mar 28, 2017 •

edited

Loading