IdP initiated SSO - Githubissues

ghost commented 6 years ago

I'd like to use IdP initated SSO with Nextcloud. The reason for this is that we have different servers/URLs for different customers and those should all be able to access Nextcloud.

Steps to reproduce

Do NOT go to Nextcloud
POST a SAML response to /index.php/apps/user_saml/saml/acs

Expected behaviour

I'd like the user to be logged in to Nextcloud, whatever the state of the user previously was (not logged in, logged in, expired session).

Problems and my fixes

Normally you'd get "null" as a response. Fix: comment out the following lines in assertionConsumerService, so it looks like this:

                // if(is_null($AuthNRequestID) || $AuthNRequestID === '') {
                //       return;
                // }

Then, logging in works. However, when you perform the SAML login again, you get an error "The user is already logged in" (or something like that). This is fixed by commenting out:

// @OnlyUnauthenticatedUsers

The code now looks like this:

         /**
         * @PublicPage
         * @NoCSRFRequired
         * @UseSession
         * // @OnlyUnauthenticatedUsers
         * @NoSameSiteCookieRequired
         *
         * @return Http\RedirectResponse|void
         */
        public function assertionConsumerService() {
                $AuthNRequestID = $this->session->get('user_saml.AuthNRequestID');
                // if(is_null($AuthNRequestID) || $AuthNRequestID === '') {
                //       return;
                // }

This seems to work fine, however I sometimes get an internal error, with in the logs:

Doctrine\DBAL\Exception\UniqueConstraintViolationException: An exception occurred while executing 'INSERT INTO "oc_authtoken"("uid","login_name","name","token","type","remember","last_activity","last_check") VALUES(?,?,?,?,?,?,?,?)' with params ["<SNIP>", "<SNIP>", "<SNIP>", "<SNIP>", 0, 0, 1523868098, 1523868098]: SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint "authtoken_token_index" DETAIL: Key (token)=(<SNIP>) already exists.

I expect this has to do with an expired session.

Questions

Now my actual question: is this a good way to add IdP initiated login to the user_saml plugin? Any security implications? How should I handle the expired session?

When I've got everything figured out, would you be interested in me adding a "Allow IdP initiated SSO" setting?

jagland commented 6 years ago

I'm assuming by "different URLs/Servers", you mean different SAML entities? Each has it's own entityID? Why not use "Authentication via Environment Variable" part of user_saml and a different SP, such as Shibboleth SP which will support multiple IdPs i.e. Federated. Might need to add the Shibboleth EDS to do your discovery (pick which IdP to use)

jwg18 commented 5 years ago

@gkrol-zermelo I am having the same issue. iDP initiated login gives me a 'null' response. Would you mind telling me the file location of the assertionConsumerService. I can't seem to find it, as I would like to implement your fix as well. Thanks.

Edit: Never mind found it. For anyone else it's at /var/www/nextcloud/apps/user_saml/lib/Controller then you can open "SAMLController.php" on lines 236 - 248

nbitouze commented 5 years ago

We have a similar need and explored this as well.

We also started by commenting out the @OnlyUnauthenticatedUsers annotation to allow an already logged-in user to come back to nextcloud from the IDP without getting a "User is already logged in error".

However, it creates an issue where two users share the same machine, Alice is logged in on Nextcloud (e.g., via IDP-initiated SSO), then Bob tries an IDP-initiated SSO login. In that scenario, what happens is that Bob arrives on Nextcloud, but still logged in as Alice (he has to manually disconnect Alice from Nextcloud, and login to Nextcloud again).

The quick&dirty fix we implemented was to logout and log back at the start of the "assertionConsumerService" method (with a hardcoded IDP equal to 1):

    public function assertionConsumerService() {
        $this->userSession->logout();
        $this->login(1);

        ...

There is a pull request related to this issue at #297. It does not remove the @OnlyUnauthenticatedUsers though, but adds a couple functionalities.