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

Add support for login via cas - github.com/apereo/cas #8058

Merged
merged 4 commits into from May 17, 2021

Conversation

hdeadman
Copy link
Contributor

@hdeadman hdeadman commented Sep 26, 2020

This adds support for CAS as an OIDC login provider. CAS is supported by grant but there isn't a central CAS server so the subdomain parameter is required to be in the grant context so the URLs can be generated. If I ran a CAS server for a company at https://my.company.com/cas then the subdomain would be my.company.com/cas. The fact that subdomain includes the whole domain and possibly some extra non-domain stuff is because that is how it is done in grant (for CAS and several other providers). https://github.com/simov/grant/blob/master/config/oauth.json#L148

We are currently using this as a strapi extension by overriding these same files in the extensions\users-permissions folder.

Edited: removed some stuff that was no longer accurate. After this PR was made initially, another provider added support for subdomain which made this much simpler.

@codecov
Copy link

codecov bot commented Sep 26, 2020

Codecov Report

Merging #8058 (3b8f58e) into master (15c04d0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8058   +/-   ##
=======================================
  Coverage   57.84%   57.84%           
=======================================
  Files         184      184           
  Lines        6395     6395           
  Branches     1396     1396           
=======================================
  Hits         3699     3699           
  Misses       2233     2233           
  Partials      463      463           
Flag Coverage Δ
front ∅ <ø> (∅)
unit 57.84% <ø> (ø)

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 15c04d0...3b8f58e. Read the comment docs.

@hdeadman hdeadman changed the title add support for login via cas - github.com/apereo/cas Add support for login via cas - github.com/apereo/cas - fix #8095 Sep 29, 2020
@hdeadman hdeadman changed the title Add support for login via cas - github.com/apereo/cas - fix #8095 Add support for login via cas - github.com/apereo/cas Sep 30, 2020
@hdeadman
Copy link
Contributor Author

hdeadman commented Oct 9, 2020

It looks like now that AWS Cognito is merged with support for subdomain, I will need to update this to leverage that support.

@hdeadman
Copy link
Contributor Author

This has been updated to use the grant subdomain support added in the AWS Cognito PR. I updated strapi to 3.2.3 and used these changes in an extension and they worked.

@derrickmehaffy
Copy link
Member

@hdeadman in the future please don't assign people to review, we will assign specific people as needed.

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.

Will also need a PR to the react-login example within strapi/strapi-examples

https://github.com/strapi/strapi-examples/tree/master/login-react

@hdeadman
Copy link
Contributor Author

@hdeadman in the future please don't assign people

Sorry, I don't know how I did that, I was about to ping you (derrickmehaffy) but I didn't hit send (that I know of).

@derrickmehaffy
Copy link
Member

@hdeadman in the future please don't assign people

Sorry, I don't know how I did that, I was about to ping you (derrickmehaffy) but I didn't hit send (that I know of).

Some applications try to add it in automatically (GitKraken being one of them)

@hdeadman
Copy link
Contributor Author

I submitted a PR for the login-react app to add cas as a provider. I noticed that nobody added cognito and the linkedin PR still needs to be merged. Someone that uses CAS, especially if they are already using OIDC for other services (apps), ought to have little difficulty setting strapi up with CAS. It would be possible to create a small project that was pre-configured to let someone run a local CAS server that worked out of the box with a local strapi but I don't know where that would live and it probably isn't worth the maintenance to keep it up to date.

strapi/strapi-examples#170

Not that it is a big deal, but is it possible that someone else added reviewers to this PR? I don't even see a "request review" button in the browser (like I do on some projects) and I wasn't using any fancy git clients that would have done it for me. Just curious...

@hdeadman
Copy link
Contributor Author

I changed the scopes to be space delimited for the CAS provider b/c that is what CAS expects and the spec seems to call for them to be passed with space delimiter. - rfc6749#section-3.3 I had been getting away with passing the scopes in a way that CAS didn't recognize because CAS's fallback behavior was working for me in some cases.

@hdeadman
Copy link
Contributor Author

Will also need a PR to the react-login example within strapi/strapi-examples

https://github.com/strapi/strapi-examples/tree/master/login-react

I made this requested PR, trying to reply to this directly to see if it will make the "changes requested" go away.

@lauriejim
Copy link
Contributor

This pull request has been mentioned on Strapi Community. There might be relevant details there:

https://forum.strapi.io/t/add-custom-oauth-provider/737/3

@hdeadman
Copy link
Contributor Author

hdeadman commented Mar 5, 2021

I am still working on instructions for setting this up. I briefly went down the road of fully automating a test and when that didn't look like it was going to work I had a temporary loss of motivation but now I have a fairly simple test working and I will write it up soon. I also realized my current deployment is working due to a non-default setting I have in that CAS deployment so I will need to modify this PR.

@hdeadman
Copy link
Contributor Author

hdeadman commented Mar 6, 2021

@alexandrebodin I made a project that should make it fairly easy to test this locally. The Github Actions in the project are running a full test so you could look at that rather than run it locally.

https://github.com/hdeadman/strapi-cas-example

Rather than doing the steps manually you can see the output from starting CAS, starting Strapi and running a puppeteer login tests:
https://github.com/hdeadman/strapi-cas-example/runs/2109137129?check_suite_focus=true
There are artifacts with screenshots showing the successful logins.

The example runs the "CAS Intializr" as a container. The "CAS Initializr" is fairly new way to create a CAS "overlay" project and it doesn't support letting you specify a CAS version so I am using a Docker image of the initializr published at the time CAS 6.3 came out. (The latest release) I could make the example so the "cas-server" project generated by the initializr was just checked in with the server ready to start via gradle build and run, but I thought this showed the whole process.

Let me know if you have any questions or concerns.

@hdeadman
Copy link
Contributor Author

hdeadman commented Apr 5, 2021

Time for monthly ping on this issue. The https://github.com/hdeadman/strapi-cas-example should demonstrate that this works and not require anything to be setup locally for testing (you can look at the Actions/CI logs and artifacts). Let me know if there is anything else I can do on this PR.

@hdeadman
Copy link
Contributor Author

Here are some screenshots from the workflow that starts cas, starts strapi and does login: https://github.com/hdeadman/strapi-cas-example/actions/runs/749775243

image

Screenshot from puppeteer test showing jwt token.
image

@derrickmehaffy
Copy link
Member

Sorry @hdeadman we have been swamped with i18n and the StrapiConf. Let me ping the PO/M team again on this and see if we can review it.

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.

Hello @hdeadman 👋
Thank you for opening this PR & maintaining it over all those months.
I just tested it through your example repository and it worked like a charm, nice job & thanks for this easy to use test env!

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 tests to pass before merging.
Thanks again for this PR and for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: feature request Issue suggesting a new feature source: plugin:users-permissions Source is plugin/users-permissions package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants