-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
It looks like now that AWS Cognito is merged with support for subdomain, I will need to update this to leverage that support. |
4a494b4
to
735f69c
Compare
This has been updated to use the grant |
@hdeadman in the future please don't assign people to review, we will assign specific people as needed. |
There was a problem hiding this 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
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) |
I submitted a PR for the login-react app to add 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... |
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. |
I made this requested PR, trying to reply to this directly to see if it will make the "changes requested" go away. |
This pull request has been mentioned on Strapi Community. There might be relevant details there: |
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. |
@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: 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. |
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. |
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 |
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. |
There was a problem hiding this 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!
There was a problem hiding this 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.
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 athttps://my.company.com/cas
then the subdomain would bemy.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#L148We 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.