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

Move Active Authentication Mode into AutomaticAuthenticationHandler #164

Closed
wants to merge 20 commits into from

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Feb 24, 2015

  • Rename AuthenticationMode to AutomaticAuthentication bool on AutomaticAuthenticationOptions
  • Split old active behavior into a new base handler class : AutomaticAuthenticationHandler which derives from AuthenticationHandler<AutomaticAuthenticationOptions>
  • Remove SecurityHelper LookupChallenge and added two new virtuals to handle the old active authentication checks (which work when no scheme is specified)
  • Also attempted to address 401-403 with this change (needs more unit tests here)

cc @lodejard @Praburaj @blowdart @Tratcher

@ghost ghost added the cla-not-required label Feb 24, 2015
@@ -60,6 +60,8 @@ Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.AspNet.Authorizat
EndProject
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.AspNet.Authorization", "src\Microsoft.AspNet.Authorization\Microsoft.AspNet.Authorization.kproj", "{6AB3E514-5894-4131-9399-DC7D5284ADDB}"
EndProject
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Microsoft.Framework.WebEncoders", "..\HttpAbstractions\src\Microsoft.Framework.WebEncoders\Microsoft.Framework.WebEncoders.kproj", "{DD2CE416-765E-4000-A03E-C2FF165DA1B6}"
Copy link
Member

Choose a reason for hiding this comment

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

I assume you didn't mean to check in this relative reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'll revert the global.json and revert all the sln changes as part of the final squash/merge, they won't make it back to dev

@HaoK
Copy link
Member Author

HaoK commented Feb 25, 2015

Updated PR, moved ShouldConvertChallengeToForbidden to AutomaticHandler, added a test for cookie/bearer to ensure they don't touch 401's if they weren't authenticated.

@@ -334,7 +314,7 @@ protected virtual Task ApplyResponseGrantAsync()

public virtual void SignIn(ISignInContext context)
{
SignInIdentityContext = new SignInIdentityContext(context.Principal, new AuthenticationProperties(context.Properties));
SignInContext = new SignInContext(context.Principal, new AuthenticationProperties(context.Properties));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't SignIn and SignOut also check for an AuthScheme match before doing anything here?

Copy link
Member Author

@HaoK HaoK Feb 25, 2015 via email

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added tests verifying that exceptions are thrown (rather than sign in/sign out happening) when wrong auth type is specified

now correctly checks for auth scheme
@HaoK
Copy link
Member Author

HaoK commented Mar 2, 2015

Merged 775eb5e

@HaoK HaoK closed this Mar 2, 2015
@@ -566,7 +566,7 @@ public override Task<bool> InvokeAsync()
{
if (ticket.Principal != null)
{
Request.HttpContext.Response.SignIn(ticket.Properties, ticket.Principal.Identities);
Request.HttpContext.Response.SignIn(ticket.AuthenticationScheme, ticket.Principal, ticket.Properties);
Copy link
Contributor

Choose a reason for hiding this comment

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

An invalid scheme is used here and makes the OIDC client middleware totally buggy because it cannot delegate the principal persistence to the cookie middleware.

Use Option.SignInScheme instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah thanks, saved me from having to track this down today!

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened a ticket for that (#167) and submitted a PR (#168) that fixes it 😄

@HaoK HaoK deleted the 2-23-auto branch April 20, 2015 23:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants