Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Split SIgnIn/SignOut #1267

Closed
wants to merge 1 commit into from
Closed

Split SIgnIn/SignOut #1267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@

namespace Microsoft.AspNetCore.Authentication.Cookies
{
public class CookieAuthenticationHandler : AuthenticationHandler<CookieAuthenticationOptions>
public class CookieAuthenticationHandler :
AuthenticationHandler<CookieAuthenticationOptions>,
IAuthenticationSignInHandler,
IAuthenticationSignOutHandler
{
private const string HeaderValueNoCache = "no-cache";
private const string HeaderValueMinusOne = "-1";
Expand Down Expand Up @@ -249,7 +252,19 @@ protected virtual async Task FinishResponseAsync()
}
}

protected override async Task HandleSignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
public async Task SignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
{
if (user == null)
{
throw new ArgumentNullException(nameof(user));
}

properties = properties ?? new AuthenticationProperties();
await HandleSignInAsync(user, properties);
Logger.SignedIn(Scheme.Name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we must now do that in EVERY handler? What a bummer 👎

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You actually have a handler that implements sign in?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASOS does (exactly like OAuthAuthorizationServerMiddleware used to).

We use that for both the authorization and token endpoints: http://kevinchalet.com/2017/01/30/implementing-simple-token-authentication-in-aspnet-core-with-openiddict/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yeah if you want to log that you signed in, you need to log it yourself (and granted the logging extensions code is ridiculously gross)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I imagine you can just add a Logger.Log at the top of your method is the only change for you, and implement the interface method directly instead of HandleSignIn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, do we get a sensible advantage if we split IAuthenticationHandler? 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SignIn/SignOut default will be more likely to be correct in applications with only a single cookie configured So OIDC + Cookie won't need to specify DefaultSignInScheme anymore, well once we actually change RemoteAuthenticationHandler to stop requiring SignInScheme now that it will work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, pretty sure I said "advantage"... making the default scheme selection more magical than it is already is not what I'd call an advantage.

I can already hear my users say "oh, my cookies+Facebook app worked fine before I added OpenIddict and now, I have an exception"... 😢

}

protected virtual async Task HandleSignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
{
_signInCalled = true;

Expand Down Expand Up @@ -327,7 +342,14 @@ protected override async Task HandleSignInAsync(ClaimsPrincipal user, Authentica
await ApplyHeaders(shouldRedirect, signedInContext.Properties);
}

protected override async Task HandleSignOutAsync(AuthenticationProperties properties)
public async Task SignOutAsync(AuthenticationProperties properties)
{
properties = properties ?? new AuthenticationProperties();
await HandleSignOutAsync(properties);
Logger.SignedOut(Scheme.Name);
}

protected virtual async Task HandleSignOutAsync(AuthenticationProperties properties)
{
_signOutCalled = true;

Expand Down
35 changes: 35 additions & 0 deletions src/Microsoft.AspNetCore.Authentication.Cookies/LoggingExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;

namespace Microsoft.Extensions.Logging
{
internal static class LoggingExtensions
{
private static Action<ILogger, string, Exception> _authSchemeSignedIn;
private static Action<ILogger, string, Exception> _authSchemeSignedOut;

static LoggingExtensions()
{
_authSchemeSignedIn = LoggerMessage.Define<string>(
eventId: 10,
logLevel: LogLevel.Information,
formatString: "AuthenticationScheme: {AuthenticationScheme} signed in.");
_authSchemeSignedOut = LoggerMessage.Define<string>(
eventId: 11,
logLevel: LogLevel.Information,
formatString: "AuthenticationScheme: {AuthenticationScheme} signed out.");
}

public static void SignedIn(this ILogger logger, string authenticationScheme)
{
_authSchemeSignedIn(logger, authenticationScheme, null);
}

public static void SignedOut(this ILogger logger, string authenticationScheme)
{
_authSchemeSignedOut(logger, authenticationScheme, null);
}
}
}
12 changes: 0 additions & 12 deletions src/Microsoft.AspNetCore.Authentication.JwtBearer/JwtBearerHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Security.Claims;
using System.Text;
using System.Text.Encodings.Web;
Expand All @@ -13,7 +12,6 @@
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Microsoft.IdentityModel.Protocols;
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
using Microsoft.IdentityModel.Tokens;
using Microsoft.Net.Http.Headers;
Expand Down Expand Up @@ -328,15 +326,5 @@ private static string CreateErrorDescription(Exception authFailure)

return string.Join("; ", messages);
}

protected override Task HandleSignOutAsync(AuthenticationProperties properties)
{
throw new NotSupportedException();
}

protected override Task HandleSignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
{
throw new NotSupportedException();
}
}
}
10 changes: 10 additions & 0 deletions src/Microsoft.AspNetCore.Authentication.OpenIdConnect/LoggingExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ internal static class LoggingExtensions
private static Action<ILogger, Exception> _remoteSignOut;
private static Action<ILogger, Exception> _remoteSignOutSessionIdMissing;
private static Action<ILogger, Exception> _remoteSignOutSessionIdInvalid;
private static Action<ILogger, string, Exception> _signOut;

static LoggingExtensions()
{
Expand Down Expand Up @@ -253,6 +254,10 @@ static LoggingExtensions()
logLevel: LogLevel.Error,
formatString: "The remote signout request was ignored because the 'sid' parameter didn't match " +
"the expected value, which may indicate an unsolicited logout.");
_signOut = LoggerMessage.Define<string>(
eventId: 49,
logLevel: LogLevel.Information,
formatString: "AuthenticationScheme: {AuthenticationScheme} signed out.");
}

public static void UpdatingConfiguration(this ILogger logger)
Expand Down Expand Up @@ -494,5 +499,10 @@ public static void RemoteSignOutSessionIdInvalid(this ILogger logger)
{
_remoteSignOutSessionIdInvalid(logger, null);
}

public static void SignedOut(this ILogger logger, string authenticationScheme)
{
_signOut(logger, authenticationScheme, null);
}
}
}
11 changes: 9 additions & 2 deletions src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ namespace Microsoft.AspNetCore.Authentication.OpenIdConnect
/// <summary>
/// A per-request authentication handler for the OpenIdConnectAuthenticationMiddleware.
/// </summary>
public class OpenIdConnectHandler : RemoteAuthenticationHandler<OpenIdConnectOptions>
public class OpenIdConnectHandler : RemoteAuthenticationHandler<OpenIdConnectOptions>, IAuthenticationSignOutHandler
{
private const string NonceProperty = "N";
private const string UriSchemeDelimiter = "://";
Expand Down Expand Up @@ -160,11 +160,18 @@ protected virtual async Task<bool> HandleRemoteSignOutAsync()
return true;
}

public async Task SignOutAsync(AuthenticationProperties properties)
{
properties = properties ?? new AuthenticationProperties();
await HandleSignOutAsync(properties);
Logger.SignedOut(Scheme.Name);
}

/// <summary>
/// Redirect user to the identity provider for sign out
/// </summary>
/// <returns>A task executing the sign out procedure</returns>
protected override async Task HandleSignOutAsync(AuthenticationProperties properties)
protected virtual async Task HandleSignOutAsync(AuthenticationProperties properties)
{
Logger.EnteringOpenIdAuthenticationHandlerHandleSignOutAsync(GetType().FullName);

Expand Down
29 changes: 0 additions & 29 deletions src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,35 +178,6 @@ protected async Task<AuthenticateResult> HandleAuthenticateOnceSafeAsync()

protected abstract Task<AuthenticateResult> HandleAuthenticateAsync();

public async Task SignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
{
if (user == null)
{
throw new ArgumentNullException(nameof(user));
}

properties = properties ?? new AuthenticationProperties();
await HandleSignInAsync(user, properties);
Logger.AuthenticationSchemeSignedIn(Scheme.Name);
}

protected virtual Task HandleSignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
{
return Task.CompletedTask;
}

public async Task SignOutAsync(AuthenticationProperties properties)
{
properties = properties ?? new AuthenticationProperties();
await HandleSignOutAsync(properties);
Logger.AuthenticationSchemeSignedOut(Scheme.Name);
}

protected virtual Task HandleSignOutAsync(AuthenticationProperties properties)
{
return Task.CompletedTask;
}

/// <summary>
/// Override this method to handle Forbid.
/// </summary>
Expand Down
20 changes: 0 additions & 20 deletions src/Microsoft.AspNetCore.Authentication/LoggingExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ internal static class LoggingExtensions
private static Action<ILogger, string, Exception> _authSchemeAuthenticated;
private static Action<ILogger, string, Exception> _authSchemeNotAuthenticated;
private static Action<ILogger, string, string, Exception> _authSchemeNotAuthenticatedWithFailure;
private static Action<ILogger, string, Exception> _authSchemeSignedIn;
private static Action<ILogger, string, Exception> _authSchemeSignedOut;
private static Action<ILogger, string, Exception> _authSchemeChallenged;
private static Action<ILogger, string, Exception> _authSchemeForbidden;
private static Action<ILogger, string, Exception> _userAuthorizationFailed;
Expand Down Expand Up @@ -62,14 +60,6 @@ static LoggingExtensions()
eventId: 9,
logLevel: LogLevel.Debug,
formatString: "AuthenticationScheme: {AuthenticationScheme} was not authenticated.");
_authSchemeSignedIn = LoggerMessage.Define<string>(
eventId: 10,
logLevel: LogLevel.Information,
formatString: "AuthenticationScheme: {AuthenticationScheme} signed in.");
_authSchemeSignedOut = LoggerMessage.Define<string>(
eventId: 11,
logLevel: LogLevel.Information,
formatString: "AuthenticationScheme: {AuthenticationScheme} signed out.");
_authSchemeChallenged = LoggerMessage.Define<string>(
eventId: 12,
logLevel: LogLevel.Information,
Expand Down Expand Up @@ -107,16 +97,6 @@ public static void AuthenticationSchemeNotAuthenticatedWithFailure(this ILogger
_authSchemeNotAuthenticatedWithFailure(logger, authenticationScheme, failureMessage, null);
}

public static void AuthenticationSchemeSignedIn(this ILogger logger, string authenticationScheme)
{
_authSchemeSignedIn(logger, authenticationScheme, null);
}

public static void AuthenticationSchemeSignedOut(this ILogger logger, string authenticationScheme)
{
_authSchemeSignedOut(logger, authenticationScheme, null);
}

public static void AuthenticationSchemeChallenged(this ILogger logger, string authenticationScheme)
{
_authSchemeChallenged(logger, authenticationScheme, null);
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.AspNetCore.Authentication/Microsoft.AspNetCore.Authentication.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Authentication.Core\Microsoft.AspNetCore.Authentication.Core.csproj" />
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Http\Microsoft.AspNetCore.Http.csproj" />
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Http.Extensions\Microsoft.AspNetCore.Http.Extensions.csproj" />
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Http.Features\Microsoft.AspNetCore.Http.Features.csproj" />

<PackageReference Include="Microsoft.AspNetCore.DataProtection" Version="$(AspNetCoreVersion)" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(AspNetCoreVersion)" />
Expand Down
11 changes: 0 additions & 11 deletions src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Security.Claims;
using System.Security.Cryptography;
using System.Text.Encodings.Web;
using System.Threading.Tasks;
Expand Down Expand Up @@ -180,16 +179,6 @@ protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
return AuthenticateResult.Fail("Remote authentication does not directly support AuthenticateAsync");
}

protected override Task HandleSignOutAsync(AuthenticationProperties properties)
{
throw new NotSupportedException();
}

protected override Task HandleSignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
{
throw new NotSupportedException();
}

protected override Task HandleForbiddenAsync(AuthenticationProperties properties)
{
return Context.ForbidAsync(SignInScheme);
Expand Down
6 changes: 3 additions & 3 deletions test/Microsoft.AspNetCore.Authentication.Test/GoogleTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,15 +1024,15 @@ private static TestServer CreateServer(Action<GoogleOptions> configureOptions, F
}
else if (req.Path == new PathString("/signIn"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignInAsync("Google", new ClaimsPrincipal()));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignInAsync("Google", new ClaimsPrincipal()));
}
else if (req.Path == new PathString("/signOut"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignOutAsync("Google"));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignOutAsync("Google"));
}
else if (req.Path == new PathString("/forbid"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.ForbidAsync("Google"));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.ForbidAsync("Google"));
}
else if (testpath != null)
{
Expand Down
4 changes: 2 additions & 2 deletions test/Microsoft.AspNetCore.Authentication.Test/JwtBearerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,11 +738,11 @@ private static TestServer CreateServer(Action<JwtBearerOptions> options = null,
}
else if (context.Request.Path == new PathString("/signIn"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignInAsync(JwtBearerDefaults.AuthenticationScheme, new ClaimsPrincipal()));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignInAsync(JwtBearerDefaults.AuthenticationScheme, new ClaimsPrincipal()));
}
else if (context.Request.Path == new PathString("/signOut"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignOutAsync(JwtBearerDefaults.AuthenticationScheme));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignOutAsync(JwtBearerDefaults.AuthenticationScheme));
}
else
{
Expand Down
4 changes: 4 additions & 0 deletions .../Microsoft.AspNetCore.Authentication.Test/Microsoft.AspNetCore.Authentication.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
<ProjectReference Include="..\..\src\Microsoft.AspNetCore.Authentication.MicrosoftAccount\Microsoft.AspNetCore.Authentication.MicrosoftAccount.csproj" />
<ProjectReference Include="..\..\src\Microsoft.AspNetCore.Authentication.JwtBearer\Microsoft.AspNetCore.Authentication.JwtBearer.csproj" />
<ProjectReference Include="..\..\src\Microsoft.AspNetCore.Authentication.Twitter\Microsoft.AspNetCore.Authentication.Twitter.csproj" />
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Http\Microsoft.AspNetCore.Http.csproj" />
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Http.Abstractions\Microsoft.AspNetCore.Http.Abstractions.csproj" />
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Http.Extensions\Microsoft.AspNetCore.Http.Extensions.csproj" />
<ProjectReference Include="..\..\..\HttpAbstractions\src\Microsoft.AspNetCore.Http.Features\Microsoft.AspNetCore.Http.Features.csproj" />
</ItemGroup>

<ItemGroup>
Expand Down
6 changes: 3 additions & 3 deletions test/Microsoft.AspNetCore.Authentication.Test/MicrosoftAccountTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,15 +205,15 @@ private static TestServer CreateServer(Action<MicrosoftAccountOptions> configure
}
else if (req.Path == new PathString("/signIn"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignInAsync("Microsoft", new ClaimsPrincipal()));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignInAsync("Microsoft", new ClaimsPrincipal()));
}
else if (req.Path == new PathString("/signOut"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignOutAsync("Microsoft"));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignOutAsync("Microsoft"));
}
else if (req.Path == new PathString("/forbid"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.ForbidAsync("Microsoft"));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.ForbidAsync("Microsoft"));
}
else
{
Expand Down
4 changes: 0 additions & 4 deletions test/Microsoft.AspNetCore.Authentication.Test/OpenIdConnect/OpenIdConnectTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Net;
Expand All @@ -11,10 +10,7 @@
using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Authentication.OpenIdConnect;
using Microsoft.AspNetCore.DataProtection;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
using Xunit;

Expand Down
6 changes: 3 additions & 3 deletions test/Microsoft.AspNetCore.Authentication.Test/TwitterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,15 +177,15 @@ private static TestServer CreateServer(Action<TwitterOptions> options, Func<Http
var res = context.Response;
if (req.Path == new PathString("/signIn"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignInAsync("Twitter", new ClaimsPrincipal()));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignInAsync("Twitter", new ClaimsPrincipal()));
}
else if (req.Path == new PathString("/signOut"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.SignOutAsync("Twitter"));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.SignOutAsync("Twitter"));
}
else if (req.Path == new PathString("/forbid"))
{
await Assert.ThrowsAsync<NotSupportedException>(() => context.ForbidAsync("Twitter"));
await Assert.ThrowsAsync<InvalidOperationException>(() => context.ForbidAsync("Twitter"));
}
else if (handler == null || !handler(context))
{
Expand Down