search

eXist-db / existdb-saml

XQuery module that implements SAML v2 single sign-on
GNU Lesser General Public License v2.1
4 stars 3 forks source link

[BUG] Ordering of statements is wrong in example controller.xq #13

Closed adamretter closed 1 year ago

adamretter commented 1 year ago

In the README.md an example controller.xq file is given here, that follows the basic ordering of checks:

  1. if no valid token, redirect to SAML auth

    if (exsaml:is-enabled() and not(exsaml:check-valid-saml-token()))

  2. if logout, invalidate SAML token

    else if ($exist:path = '/logout')

  3. handle SP endpoint to process SAML response in HTTP POST

    else if($exist:path = "/SAML2SP")

  4. else, your controller code...

Unfortunately, this is the wrong ordering of checks. If the above is followed, a never-ending loop occurs between the SP and IdP (via the User Agent).

To explain the never ending loop:

  1. The SP (eXist-db) receives a HTTP request from the User Agent and fires the controller.xql.
  2. The check at position 1 of the controller.xql(above) causes the initial redirect of the User Agent from the SP to the IdP (e.g. Microsoft Azure).
  3. IdP authenticates the User Agent and provides them with an HTML Form that will HTTP POST the SAML Response to the SP.
  4. User Agent submits the HTML Form, and thus HTTP POST's the SAML Response to the SP.
  5. The SP receives the HTTP POST and fires the controller.xql
  6. The The check at position 1 of the controller.xql(above) AGAIN causes a redirect of the User Agent from the SP to the IdP (e.g. Microsoft Azure)... And so we go back to Step 1...

The example controller.xql in the README.md file is incorrect and needs to be refactored so that checks (1) and (3) of the controller.xq (at the top of this issue) are swapped. Ideally further checks should also be added to differentiate between HTTP GET and POST requests and the action that should be taken.

chakl commented 1 year ago

@adamretter You are completely right about this:

The example controller.xql in the README.md file is incorrect and needs to be refactored so that checks (1) and (3) of the controller.xq ... are swapped.

I checked some working installations, and of course they have step 3 first. Clearly a documentation bug, thanks for reporting. Noted for upcoming overhaul.