Introduce SignInResult/SignOutResult and SignIn/SignOut #4259

kevinchalet · 2016-03-09T20:20:40Z

(FWIW, that would be great if the authentication helpers had overloads accepting a params string[] argument for authenticationSchemes).

/cc @rynowak @HaoK @pranavkm

dnfclas · 2016-03-09T20:20:44Z

Hi @PinpointTownes, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

kevinchalet · 2016-03-09T20:21:30Z

src/Microsoft.AspNetCore.Mvc.Core/SignInResult.cs

+
+            if (authenticationSchemes.Count == 0)
+            {
+                // TODO: move the exception message to the resources file.


Question: what's the recommended way to add new messages to the resources file? The resx KoreBuild target?

Yup - build resx

Sadly, it doesn't work on my machine:

& : The module '.build' could not be loaded. For more information, run 'Import-Module .build'. At C:\Users\Chalet Kévin\Documents\GitHub\Forks\Mvc\.build\KoreBuild.ps1:64 char:2 + &"$koreBuildFolder\Sake\0.2.2\tools\Sake.exe" -I $koreBuildFolder\sha ... + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : ObjectNotFound: (.build\Sake\0.2.2\tools\Sake.ex e:String) [], ParentContainsErrorRecordException + FullyQualifiedErrorId : CouldNotAutoLoadModule

I guess it's related to aspnet/Security#688 (comment), since I'm also using Windows 7.

/cc @Eilon

You might need to delete the .build directory. We made a bunch of changes to how we procure and layout the build scripts.

Nope, no luck 😢

I ended up manually adding the property/method in the designer file.

HaoK · 2016-03-09T20:38:56Z

Meh, not sure its worth an extension method to do a foreach SignOut.

HaoK · 2016-03-09T20:40:40Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

+        /// <param name="principal">The <see cref="ClaimsPrincipal"/> containing the user claims.</param>
+        /// <returns>The created <see cref="SignInResult"/> for the response.</returns>
+        [NonAction]
+        public virtual SignInResult SignIn(IList<string> authenticationSchemes, ClaimsPrincipal principal)


Not sure this really makes sense, would you really expect someone to sign the same principal into multiple schemes? I would expect different principals would get signed into each scheme in general

FYI, some ASOS users are doing that to create a cookie and a token with the same principal.

That said, you're probably right, it's surely not a common case.

Yeah I think things look much cleaner with a single scheme for sign in / sign out. You only need multiple schemes for challenge.

Okay for SignIn but for SignOut, see #4259 (comment).

danroth27 · 2016-03-09T22:43:36Z

Thank you for the PR! However, this needs to wait until post 1.0.0. We aren't taking additional features into the current release.

kevinchalet · 2016-03-09T22:48:54Z

@danroth27 alright.

That said, it's not really a new feature stricto sensu, as it's mainly a copy/paste of existing code, which is itself a tiny wrapper around the authentication APIs.

davidfowl · 2016-03-15T03:35:40Z

@danroth27 @Eilon this is such a small change. Do we like the API? If so can we merge it? It feels like polish since we have ChallengeResult, SignInResult and SIgnOut result can clean up some of the template code as well.

@Tratcher @HaoK does it look good?

kevinchalet · 2016-03-15T03:57:44Z

src/Microsoft.AspNetCore.Mvc.Core/SignInResult.cs

+        /// <param name="properties"><see cref="AuthenticationProperties"/> used to perform the sign-in operation.</param>
+        public SignInResult(string authenticationScheme, ClaimsPrincipal principal, AuthenticationProperties properties)
+        {
+            if (authenticationScheme == null)


Question: should we keep the null guards even if the properties are publicly settable?

Either keep the null guards and make the properties read-only, or remove the null guards.

Actually, I wonder if we shouldn't simply move these guards to the ExecuteAsync method, prior to calling the authentication manager.

Anyway, I need to fix all the aspnet-contrib projects before going back to this PR. Everything stopped compiling/working and I can't find a workaround...

IMO better to validate on the parameters + properties. Generally ExecuteAsync won't be called in unit tests, so it be easy to use the result the wrong way.

I'm fine with dual properties + parameters validation, but we'll still have to move the "as at least one element" check of SignOutResult.AuthenticationSchemes elsewhere, since we could validate that for a constructor parameter, but not for a property.

Another approach might be to remove all the public setters and update SignOutResult.AuthenticationSchemes to expose a read-only list. TBH, I have no personal preference, as long as it's consistent everywhere.

We generally want all the action results to have get/set properties. It's really handy to be able to modify things in filters. Speaking of, we should probably make a defensive copy of whatever list the user passes in.

I've added a tiny check in both SignInResult.ExecuteAsync and SignOutResult.ExecuteAsync to ensure the AuthenticationScheme(s) property is not left is an incoherent state (null or empty, basically).

@rynowak do you strongly feel we should do some ToArray/ToList here? Authentication-related actions are probably not critical paths, so I guess the extra allocations are fine.

Eilon · 2016-03-15T17:29:05Z

Fine, fine, we can take this 😄

rynowak · 2016-03-18T15:03:10Z

src/Microsoft.AspNetCore.Mvc.Core/SignInResult.cs

+            var authentication = context.HttpContext.Authentication;
+            await authentication.SignInAsync(AuthenticationScheme, Principal, Properties);
+
+            logger.SignInResultExecuting(AuthenticationScheme, Principal);


Ideally this line would take place before the call to SignInAsync. This way if the 'actual' operation throws, we've already logged what we were trying to do.

Agreed, I'll update that and fix the existing results.

rynowak · 2016-03-18T15:09:21Z

⌚ from me for a few small things

@HaoK - any more feedback on this?

kevinchalet · 2016-03-18T17:24:51Z

(FWIW, that would be great if the authentication helpers had overloads accepting a params string[] argument for authenticationSchemes).

Any opinion?

HaoK · 2016-03-18T18:46:41Z

Looks fine to me

rynowak · 2016-03-23T16:49:32Z

@PinpointTownes - any updates?

(FWIW, that would be great if the authentication helpers had overloads accepting a params string[] argument for authenticationSchemes).

I wouldn't block this PR on that item, if you want to, go ahead and file that on the appropriate repo?

kevinchalet · 2016-03-23T17:02:49Z

Sadly no. Fixing all the aspnet-contrib/OpenIddict stuff to make DNX and CLI happy with the recent changes in corefx and NuGet took me much more time than I had expected (and that's an euphemism).

The fact MVC itself no longer compiles with DNX/VS is also extremely annoying, specially since CLI no longer works on Windows 7: https://github.com/dotnet/cli/issues/1966

TBH, I don't feel super comfortable not being able to run the unit tests before pushing new commits. But since you haven't updated MVC to include the frameworkAssemblies workaround that would make DNX happy and since CI builds always fail before running the tests, that's pretty much my only option.

I'll try some yolodev tonight and fix all the reviewed points.

By authentication helpers, I meant the SignOut/Challenge/Forbid methods exposed by ControllerBase, that all take IList. We could update them to take array params without having to update AuthenticationManager.

Le 23 mars 2016 à 17:50, Ryan Nowak [email protected] a écrit :

@PinpointTownes - any updates?

(FWIW, that would be great if the authentication helpers had overloads accepting a params string[] argument for authenticationSchemes).

I wouldn't block this PR on that item, if you want to, go ahead and file that on the appropriate repo?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

rynowak · 2016-03-23T17:07:32Z

By authentication helpers, I meant the SignOut/Challenge/Forbid methods exposed by ControllerBase, that all take IList. We could update them to take array params without having to update AuthenticationManager.

Yeah I think that would be a fine change

Sorry for all the difficulties 😢 if you don't manage to get to this I'm happy to fix up the remaining stuff and make sure sure it's included for RC2.

kevinchalet · 2016-03-23T18:01:50Z

src/Microsoft.AspNetCore.Mvc.Core/ChallengeResult.cs

            var authentication = context.HttpContext.Authentication;
-            if (AuthenticationSchemes.Count > 0)
+            if (AuthenticationSchemes != null && AuthenticationSchemes.Count > 0)


I've added a null check here, since AuthenticationSchemes can be null (there's no null guard in the constructor).

kevinchalet · 2016-03-23T18:06:05Z

📅 PR updated 😄

kevinchalet · 2016-03-23T18:11:04Z

Yeah I think that would be a fine change

Alright, I've updated the ControllerBase methods but kept the results themselves as-is (they still take IList<string> parameters)

Sorry for all the difficulties 😢 if you don't manage to get to this I'm happy to fix up the remaining stuff and make sure sure it's included for RC2.

No worries. Thanks for the offer: actually, it would be great if you could run the tests for me 😄

rynowak · 2016-03-23T18:26:05Z

src/Microsoft.AspNetCore.Mvc.Core/SignInResult.cs

+            if (authenticationSchemes.Count == 0)
+            {
+                // TODO: move the exception message to the resources file.
+                throw new ArgumentException("At least one scheme must be specified", nameof(authenticationSchemes));


I'll take a look at fixing this

Hum, are you commenting on an old commit? I'm pretty sure I've fixed that (by manually adding the resource entry )

Yeah that was an oopsie on my part

rynowak · 2016-03-23T18:55:33Z

- looks good to me. I'm going to squash and run tests for you

kevinchalet · 2016-03-23T18:57:43Z

I can squash it if you want, it will be easier for you if you want to cherry-pick it.

rynowak · 2016-03-23T18:58:42Z

go for it then 👍

kevinchalet · 2016-03-23T19:00:51Z

Done 👏

rynowak · 2016-03-23T21:07:46Z

Will merge as soon as I'm able to.... (restore issues). #ThanksPranav

rynowak · 2016-03-24T22:24:57Z

Still waiting on package/build issues, will update once I've been able to merge.

kevinchalet · 2016-03-24T22:46:16Z

Thanks!

Yeah, it's super messy ATM, the reversioning of the new System.Runtime package is literally killing me (openiddict/openiddict-core#76 (comment))

rynowak · 2016-03-28T20:17:47Z

Pushed as f9d24a8 Thanks 👍

kevinchalet · 2016-03-28T20:19:57Z

Thanks @rynowak 👏 👏

dnfclas added the cla-already-signed label Mar 9, 2016

kevinchalet reviewed Mar 9, 2016
View reviewed changes

This was referenced Mar 9, 2016

Consider adding SignInResult/SignOutResult and SignIn/SignOut #4258

Closed

A few MVC unit tests fail because of localized exception messages #4260

Closed

HaoK reviewed Mar 9, 2016
View reviewed changes

danroth27 added this to the Backlog milestone Mar 9, 2016

pranavkm referenced this pull request in aspnet/KoreBuild Mar 10, 2016

Use the absolute path instead of passing in relative dir

407ad6d

This was referenced Mar 15, 2016

SignOutAsync should take [] of AuthenticationSchemes aspnet/HttpAbstractions#585

Closed

SignOutAsync should take [] of AuthenticationSchemes aspnet/Security#742

Closed

kevinchalet reviewed Mar 15, 2016
View reviewed changes

Eilon removed this from the Backlog milestone Mar 15, 2016

Eilon assigned rynowak Mar 15, 2016

rynowak reviewed Mar 18, 2016
View reviewed changes

kevinchalet reviewed Mar 23, 2016
View reviewed changes

rynowak reviewed Mar 23, 2016
View reviewed changes

Introduce SignInResult/SignOutResult and ControllerBase.SignIn/SignOut

928ba0e

rynowak closed this Mar 28, 2016

kevinchalet deleted the signin_signout_results branch March 28, 2016 20:19

HaoK mentioned this pull request Jun 13, 2017

SignOut() without args is broken #6392

Closed

aspnet-hello mentioned this pull request Jan 2, 2018

SignOutAsync should take [] of AuthenticationSchemes dotnet/aspnetcore#2721

Closed

Introduce SignInResult/SignOutResult and SignIn/SignOut #4259

Introduce SignInResult/SignOutResult and SignIn/SignOut #4259

Conversation

kevinchalet commented Mar 9, 2016

dnfclas commented Mar 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danroth27 commented Mar 9, 2016

kevinchalet commented Mar 9, 2016

davidfowl commented Mar 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented Mar 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Mar 18, 2016

kevinchalet commented Mar 18, 2016

HaoK commented Mar 18, 2016

rynowak commented Mar 23, 2016

kevinchalet commented Mar 23, 2016

rynowak commented Mar 23, 2016

Choose a reason for hiding this comment

kevinchalet commented Mar 23, 2016

kevinchalet commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Mar 23, 2016

kevinchalet commented Mar 23, 2016

rynowak commented Mar 23, 2016

kevinchalet commented Mar 23, 2016

rynowak commented Mar 23, 2016

rynowak commented Mar 24, 2016

kevinchalet commented Mar 24, 2016

rynowak commented Mar 28, 2016

kevinchalet commented Mar 28, 2016