subfission / cas

Simple CAS Authentication for Laravel 5 - 10.
MIT License
151 stars 70 forks source link

Support for apereo/phpcas 1.6.0 #115

Closed dstepe closed 1 year ago

dstepe commented 1 year ago

After some more digging, I think supporting apereo/phpcas 1.6.0 is going to require much more work. (I really wish they would follow semver, this is not a minor release.)

Here are two examples of what I've found so far.

The CAS::client() method takes a new required argument defined as:

$service_base_url the base URL (protocol, host and the
     *                                                  optional port) of the CAS client; pass
     *                                                  in an array to use auto discovery with
     *                                                  an allowlist; pass in
     *                                                  CAS_ServiceBaseUrl_Interface for custom
     *                                                  behavior. Added in 1.6.0. Similar to
     *                                                  serverName config in other CAS clients.

The value of that argument cannot be empty. The CasManager class will need to allow it to be configured, and if not configured, must use a reasonable default.

After working through that, I was told:

this version of CAS (`SAML_VERSION_1_1') is not supported by phpCAS 1.6.0

Since the CasManager class defaults to cas_enable_saml = true, this is likely to require much more review and testing.

I've started working on PHPUnit tests on a fork. I have a lot of ideas and am happy to keep working on this if that's something you're interested in. However, given the nature of the apereo/phpcas changes, I'd like to settle on a plan for implementing changes. I propose to not support apereo/phpcas > 1.5.0 at this time. We first work to get tests in place on what is currently working. Only after we are comfortable with test coverage should we tackle dependency upgrades. Thoughts?

dstepe commented 1 year ago

After an exchange with the apereo/phpcas maintainer and further review, I realize the SAML issue is of my own making. It's related to something that came while testing and can be disregarded.

I do suggest reading the security advisory related to the 1.6.0 release, however. There is more work required to fully prepare this package and users for the change.

subfission commented 1 year ago

After review, I tend to agree. This will take some work to revise.

For testing purposes and goals, this projects needs to maintain these two targets:

  1. Backwards compatibility down to Laravel 5.x.
  2. Security

In a perfect world, we could just use the latest dependencies and be done, but breaking changes are frequent in open-source projects and updates are highly unpredictable. Therefore, conditional logic can be added in to the code to adjust the dataflow to the correct method for the dependent CAS package version as needed. The unit testing can be implemented to cover these different conditional flows and to test integration with new versions of Laravel and phpCAS. I'm also inclined to look for alternatives to apereo as their code quality is subpar.

I'll close this for now, as food for thought.

My time towards maintaining this package is quite limited. If you are interested in being a collaborator, let me know.

dstepe commented 1 year ago

@subfission My institution currently relies heavily on this package and I'm willing to help with maintenance. I've started a draft PR (#117) with some ideas for testing. Please start a conversation on the PR to let me know how you see this fitting in with your goals.

I think your 4.3.0 release will work, it just requires additional configuration which I had missed (repercussions of upstream breaking changes). I'm testing that with our apps now.