flipboxfactory / saml-sp

SAML Service Provider (SP) Plugin for Craft CMS
https://saml-sp.flipboxfactory.com/
Other
19 stars 5 forks source link

Login page default RelayState being set to base URL #119

Closed ampmtm closed 3 years ago

ampmtm commented 3 years ago

When accessing our login page directly at /admin, the RelayState is set to the base URL causing the user to be redirected to the homepage of our website versus the desired location of the Dashboard. However, if we try to access a direct URL within the admin when logged out such as an entry or even /admin/entries, that referring URL is successfully captured in the RelayState and the user is redirected to the proper place after authentication.

Any ideas what may be causing this?

dsmrt commented 3 years ago

👋 @ampmtm, What version of craft are you on?

ampmtm commented 3 years ago

Hi, @dsmrt! We're currently on 3.6.16.

dsmrt commented 3 years ago

Great! Let me check something.

dsmrt commented 3 years ago

I'm not able to replicate what you are seeing. I get taken back to the admin/dashboard as expected. If you look at the code below, do you think there's a config that could be messing with the below?

/vendor/flipboxfactory/saml-sp/src/templates/_cp/login.twig

                {% if providers %}
                    {% for provider in providers.all %}
                        <div class="buttons">
                            <a href="{{ provider.getLoginRequestPath }}?RelayState={%- if craft.app.user.returnUrl() == siteUrl -%}{{ cpUrl('dashboard') }}{%- else -%}{{ craft.app.user.returnUrl() }}{% endif %}" class="btn submit">
                                Via {{ provider.label ? provider.label : provider.entityId }}
                            </a>
                        </div>
                    {% endfor %}
                {% endif %}
ampmtm commented 3 years ago

Thanks for pointing me in this direction! I reviewed our config files and noticed we may have had a competing siteUrl variable held over from our recent upgrade. That seemed to solve the issue for hitting the page directly via /admin.

In further testing, I was able to replicate the issue again by logging out of the control panel and checking the RelayState which seems to revert it back to the base URL. Much more of an edge case and far less of a concern for us specifically, but wondering if that happens in your testing as well.

It seems Line 95 in your above referenced file should prevent the base URL from ever being the RelayState, no?

ampmtm commented 3 years ago

Looks like when we login via the Okta interface we are being redirected to the base URL as well. If we define the Default Relay State in Okta, we get an Internal Server Error from Craft. The authentication is successful if you manually type in the admin URL and reload.

Header may not contain more than a single header, new line detected in /vendor/yiisoft/yii2/web/Response.php at line 381:

header("$name: $value", $replace);
if (headers_sent($file, $line)) {
        throw new HeadersAlreadySentException($file, $line);
    }
    if ($this->_headers) {
        foreach ($this->getHeaders() as $name => $values) {
            $name = str_replace(' ', '-', ucwords(str_replace('-', ' ', $name)));
            // set replace for first occurrence of header but false afterwards to allow multiple
            $replace = true;
            foreach ($values as $value) {
                header("$name: $value", $replace);
                $replace = false;
            }
        }
    }
    $statusCode = $this->getStatusCode();
    header("HTTP/{$this->version} {$statusCode} {$this->statusText}");
    $this->sendCookies();
}

Happy to move this to a separate issue if unrelated.

dsmrt commented 3 years ago

As for the last comment, if you think this is a bug and unrelated to the other issues, break it off into an different issue if you don't mind. Before you do that though, can you make sure there isn't any debug output (like var_dumps/etc) that could be causing this? I feel like that is common with this error.

As for the possible logout issue...

I was able to replicate the issue again by logging out of the control panel and checking the RelayState which seems to revert it back to the base URL.

I wasn't able to replicate this either. I just tested this by:

  1. going to the /admin cp login page and using SSO to log me
  2. I was placed at the /admin/dashboard page.
  3. From there I logged out which sent me to the /admin cp login page and logged back in using SSO
  4. I was again placed on the /admin/dashboard page.

Let me know if I'm missing something there but it seems to be working for me.

dsmrt commented 3 years ago

I think we resolved this issue. Let me know if you're still having issues and we can open this one back up.

Thanks!

ampmtm commented 3 years ago

Thanks for your help, @dsmrt!

In case anyone comes across this and is having similar issues, base 64 encoding our Default Relay State in Okta solved the Internal Server Error issue we were getting upon successful authentication redirect.

dsmrt commented 3 years ago

Thanks @ampmtm ... good point on mentioning the base 64 issue (ref #120)