Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

SignedOut Callback not invoked with exising samples #6

Closed
clairernovotny opened this issue Dec 5, 2016 · 4 comments
Closed

SignedOut Callback not invoked with exising samples #6

clairernovotny opened this issue Dec 5, 2016 · 4 comments

Comments

@clairernovotny
Copy link

clairernovotny commented Dec 5, 2016

When using multiple policies and the non-default values for CallbackPath on the OIDC middlewhere, at least with version 1.1, it seems like you also need to specify overrides of SignedOutCallbackPath and RemoteSignOutPath for the post logout callbacks to be invoked correctly:

I currently have the following in my CreateOptionsFromPolicy, which seems to work.

                CallbackPath = new PathString($"/{policy}"),
                SignedOutCallbackPath = new PathString($"/signout-callback-{policy}"),
                RemoteSignOutPath = new PathString($"/signout-{policy}"),
@KevinOrtman
Copy link

Thank you much for that comment - that had me stumped!

@alobakov
Copy link

alobakov commented Jan 6, 2017

Just spent the entire day troubleshooting and debugging this issue.
It boils down to the fact that when SignedOutCallbackPath is left unassigned and uses its default value, the OIDC middleware has no criteria to select the right policy when processing a sign-out callback, and simply picks the first declared one.
As a result, when any policy other than the first declared one is being signed out of, the following method fails to re-hydrate the properties previously sent to (and echoed by) the identity provider in the encrypted "state" parameter, which contains the ultimate PostLogoutRedirectUri.

OpenIdConnectHandler.cs (v1.1.0) :

        protected virtual Task<bool> HandleSignOutCallbackAsync()
        {
            StringValues protectedState;
            if (Request.Query.TryGetValue(OpenIdConnectParameterNames.State, out protectedState))
            {
                var properties = Options.StateDataFormat.Unprotect(protectedState);
                if (!string.IsNullOrEmpty(properties?.RedirectUri))
                {
                    Response.Redirect(properties.RedirectUri);
                    return Task.FromResult(true);
                }
            }

            return Task.FromResult(true);
        }

The reason for the failure is that Options.StateDataFormat.Unprotect() internally uses its own KeyRingBasedDataProtector which is policy-specific, resulting in the following exception being thrown by the latter:

The payload was invalid.

Obviously, it cannot decrypt data previously encrypted by another protector which most certainly used a different encryption key.
Notably, the exception is then silently suppressed in SecureDataFormat.cs for security reasons:

        public TData Unprotect(string protectedText, string purpose)
        {
            try
            {
                if (protectedText == null)
                {
                    return default(TData);
                }

                var protectedData = Base64UrlTextEncoder.Decode(protectedText);
                if (protectedData == null)
                {
                    return default(TData);
                }

                var protector = _protector;
                if (!string.IsNullOrEmpty(purpose))
                {
                    protector = protector.CreateProtector(purpose);
                }

                var userData = protector.Unprotect(protectedData);
                if (userData == null)
                {
                    return default(TData);
                }

                return _serializer.Deserialize(userData);
            }
            catch
            {
                // TODO trace exception, but do not leak other information
                return default(TData);
            }
        }

Therefore, no redirect is sent to the user agent.
So, it seems to work as expected and, as @onovotny pointed out, we just need to give it a hint by specifying distinct per-policy signed-out callback paths.

@nhwilly
Copy link

nhwilly commented Mar 20, 2017

Man, oh, man. Thank you. Where do I send the beer? This was making me nuts.

@gsacavdm
Copy link
Contributor

I don't believe this is applicable anymore with the updated 1.1 sample. I'll close the issue. Let me know if this is still and issue and we can reopen/revisit.

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

No branches or pull requests

5 participants