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

Added a POST route to the SSO routes to support SAML login callbacks #10690

Merged
merged 2 commits into from
Aug 19, 2021

Conversation

kasonde
Copy link
Contributor

@kasonde kasonde commented Aug 4, 2021

What does it do?

This PR creates a route that accepts a POST request for SAML login to work with Strapi.

Why is it needed?

TL;DR

We need to add an additional route to this file in the Strapi SSO code to allow SAML login. Instead of sending a GET request to the route:

/connect/:provider

Services like OKTA and OneLogin, i.e SAML SSO login strategies send a POST request there. This results in a 405 error being thrown. Method not allowed .

Details

It turns out that the passport-saml auth strategy is sending a POST request to the route /connect/:provider instead of the expected GET request. This was causing all login attempts to fail, throwing a 405 error. The issue was tracked to this routes.js file. After adding the code below to the array:

{
  method: 'POST',
  path: '/connect/:provider',
  handler: 'authentication.providerLogin',
}

the SSO feature worked again.

How to test it?

Attempt to perform login with a provider like OneLogin or Okta. This currently exists on Strapi v3.x.x and v4.x-next

@kasonde kasonde requested a review from Convly August 4, 2021 15:44
@Convly Convly mentioned this pull request Aug 4, 2021
@Convly Convly added this to the 3.6.7 milestone Aug 4, 2021
@Convly Convly added source: core:strapi Source is core/strapi package issue: bug Issue reporting a bug labels Aug 4, 2021
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #10690 (8e57f8b) into master (e63fb9d) will not change coverage.
The diff coverage is n/a.

❗ Current head 8e57f8b differs from pull request most recent head 41759d3. Consider uploading reports for the commit 41759d3 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10690   +/-   ##
=======================================
  Coverage   58.01%   58.01%           
=======================================
  Files         185      185           
  Lines        6431     6431           
  Branches     1398     1398           
=======================================
  Hits         3731     3731           
  Misses       2236     2236           
  Partials      464      464           
Flag Coverage Δ
front ∅ <ø> (∅)
unit 58.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e63fb9d...41759d3. Read the comment docs.

Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, waiting for the checks to pass 🎉

Copy link
Member

@derrickmehaffy derrickmehaffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well, will prob have to wait for next release since we just did v3.6.6 earlier today

@derrickmehaffy
Copy link
Member

Tested using passport-saml for another customer and adding this route fixed the issue. Looking to get this merged ASAP and pushed out in a release.

@alexandrebodin alexandrebodin merged commit fb7f280 into master Aug 19, 2021
@alexandrebodin alexandrebodin deleted the saml_support_sso_v3 branch August 19, 2021 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug source: core:strapi Source is core/strapi package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants