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

Single logout not working in asp.net core #1092

Closed
mirza21 opened this issue May 14, 2019 · 7 comments
Closed

Single logout not working in asp.net core #1092

mirza21 opened this issue May 14, 2019 · 7 comments

Comments

@mirza21
Copy link

mirza21 commented May 14, 2019

Non Security Issues

Single logout is not working in Asp.net core

Information needed

  1. What nuget packages are you using
    Sustainsys.Saml2.AspNetCore2 packages is being using to consume SAML from ADFS 4.0
  2. What is the expected behaviour
    When I initiate single logout by calling "Saml2/Logout".
    i)Saml2Handler>HandleRequestAsync method is called
    ii) Then in LogoutCommand>InitiateLogout is called at some point.
  if (request.User != null)
            {
                idpEntityId = request.User.FindFirst(AuthServicesClaimTypes.LogoutNameIdentifier)?.Issuer;
                sessionIndexClaim = request.User.FindFirst(AuthServicesClaimTypes.SessionIndex);
            }

Here request.User should not be null so that "AuthServicesClaimTypes.LogoutNameIdentifier" and
"AuthServicesClaimTypes.SessionIndex" claims can be parsed and used to create logout Saml response.

  1. What happens instead.
    request.User is always null it seems that code never populates it .

If you look at call to Saml2Hanlder> HandleRequestAsync. Here call to "context.ToHttpRequestData(dataProtector.Unprotect)" will never populate user.


public Task<bool> HandleRequestAsync()
     {
         if(context.Request.Path.StartsWithSegments(options.SPOptions.ModulePath, StringComparison.Ordinal))
         {
             var commandName = context.Request.Path.Value.Substring(
                 options.SPOptions.ModulePath.Length).TrimStart('/');

             var commandResult = CommandFactory.GetCommand(commandName).Run(
                 context.ToHttpRequestData(dataProtector.Unprotect), options);

             commandResult.Apply(context, dataProtector, options.SignInScheme);

             return Task.FromResult(true);
         }
         return Task.FromResult(false);
     }

I think this is breaking logout feature .

Additional info

  • Version of Asp.Net MVC / Asp.NET Core used.
@torronen
Copy link

I believe User is being populated by context.ToHttpRequestData( ... ) (source code in Sustainsys.Saml2.AspNetCore2 / HttpRequestExtensions.cs )

Were you able to debug further information or fix it?

@arthurmmedeiros
Copy link

Facing a similar issue here, but I believe this is because somewhere we need to set these two claim types:

  • LogoutNameIdentifier
  • SessionIndex

I can't figure how to add them.
Also, I don't understand why the login is so easy to implement, but the logout is so difficult.

@mklinke
Copy link

mklinke commented Nov 21, 2019

@arthurmmedeiros I happen to have implemented what you suggest today. Not sure yet, if it’s the intended way, but I have stored the claims in a cookie each and add them in a middleware to the user via a new identity. I have already been using a cookie authentication scheme to do the actual sign in and sign out because SAML2 support is optional in my app. Still, I’ve been wondering if there is an existing solution in ASP.NET Core to keep the claims in the session. Haven’t found any in the code of this project. Maybe this helps you or someone else to get going. Ideally, we all learn an even better way to do it here 😉
Cheers

@arthurmmedeiros
Copy link

Hey @mklinke , nice to know you were able to implement it. Is there any chance you could share how you managed to store the claims in a cookie? Also, how did you add a new identity? Is everything set in the StartUp?

@mklinke
Copy link

mklinke commented Nov 25, 2019

@arthurmmedeiros Sure. I've created two methods to convert between Claim and string (to store in the cookie), using the WriteTo method and the Claim constructor accepting a BinaryReader.

In addition, I'm using a cookie scheme to create the cookies on sign in. I'm setting the Saml2Options.SignInScheme to that cookie scheme. Then, in the cookie options (CookieAuthenticationOptions), I'm setting the Events.OnSignedIn Func to append a cookie to the request for the two claims that I retrieve from the context.Principal.

To restore the claims from the cookies, I've created a new middleware to read the cookie values and add an identity to the context.Principal. The middleware is registered after the authentication middleware so that the principal is populated. If there is no principal, you can directly create one with the identity, too. Otherwise, just add the identity to the existing principal.

As I said, it seems pretty basic and I'd hoped that this would be covered by some existing implementation, but I couldn't find any. Looking further into the ASP.NET Core Identity model might reveal some insights, but I don't have the capacity for that atm.

Let me know if you have further questions, I'll try to answer if I can. I was glad to find this project to support SAML2 in ASP.NET Core and I'm happy to return/forward some of the experience.

Of course, I'd be curious if there is a better/existing way to do this. Please let me know if you find one.

HTH and good luck.

@stukalin
Copy link

Huuge +1 here.

@AndersAbel would you mind to chime in and perhaps explain a little bit on how it was planned to save/restore claims info between requests?

In the sample app I can see this class which I assume should do it

        class Saml2ClaimsFactory : IUserClaimsPrincipalFactory<ApplicationUser>
        {
            IUserClaimsPrincipalFactory<ApplicationUser> _inner;
            ExternalLoginInfo _externalLoginInfo;

            public Saml2ClaimsFactory(
                IUserClaimsPrincipalFactory<ApplicationUser> inner,
                ExternalLoginInfo externalLoginInfo)
            {
                _inner = inner;
                _externalLoginInfo = externalLoginInfo;
            }

            public async Task<ClaimsPrincipal> CreateAsync(ApplicationUser user)
            {
                var principal = await _inner.CreateAsync(user);

                var logoutInfo = _externalLoginInfo.Principal.FindFirst(Saml2ClaimTypes.LogoutNameIdentifier);
                var sessionIndex = _externalLoginInfo.Principal.FindFirst(Saml2ClaimTypes.SessionIndex);

                var identity = principal.Identities.Single();
                identity.AddClaim(logoutInfo);
                identity.AddClaim(sessionIndex);

                return principal;
            }
        }

but that externalLoginInfo in the constructor just seems a little hacky for me...

@rjregenold
Copy link

rjregenold commented May 26, 2020

Maybe this will help someone that is having the same issue: if you're using the SignInManager then you can use SignInWithClaimsAsync method and pass the additional claims. That seems to be working for me.

https://github.com/dotnet/aspnetcore/blob/v3.1.4/src/Identity/Core/src/SignInManager.cs#L214-L222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants