SSP-2066_add_routes_and_controllers

[ioigoume]

Closes #26

[codecov]

Codecov Report

Attention: Patch coverage is 90.39548% with 51 lines in your changes missing coverage. Please review.

Project coverage is 67.86%. Comparing base (2b946a9) to head (416f2b1).
Report is 82 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##             master      #45       +/-   ##
=============================================
+ Coverage     41.43%   67.86%   +26.42%     
- Complexity      204      340      +136     
=============================================
  Files            14       26       +12     
  Lines           654     1229      +575     
=============================================
+ Hits            271      834      +563     
- Misses          383      395       +12     

[pradtke]

Thanks @ioigoume for all the work on this major update and improvement. I did some QA but have not yet read through the code changes. Most things worked great. I ran into the below issues

1. Overriding the CAS configuration for a specific service url did not work for me. See

   simplesamlphp-module-casserver/config/module_casserver.php.dist

   Line 31 in 81b6b53

   'attrname' => 'uid',

   for an example of how that looks in the config. My expectation is that I would be able to adjust various settings (such as attrname, attributes_to_transfer , and authproc) for each CAS service url. That wasn't working for me.

2. I couldn't get the Cas1 validate to work. It would always return no and have this error casserver:validate: internal server error. Missing user name attribute: 'uid'

3. I think this is a pre-existing issue. Basically HttpUtils changes the service url in ways that can be unexpected by the CAS client. For example if the service url was https%3A%2F%2Flocalhost%2Fexample%3Fval%3Db%26val%3Dc which has a query parameter ?val=a&val=b then SSP is dropping one of the those values. There are other similar issues with '+' and a couple other ambiguous parts of url spec. I think we can look at this in a separate PR (I think Option to redirect to exact service url #25 was my previous workaround).

4. Spurious 'Header already sent' error in logs. If I do something like not send a ticket param to validate, I get the expected CAS error, but in the logs I also see ERR [TRdeaf66cb] Error creating session: Headers already sent..

I'll look through the code and unit tests during my next free block.

[pradtke]

Thanks @ioigoume . I did a read through of the code. I've added some comments for things.

I think the main issue is that the per service CAS configs are not being used in Cas20Controller since it instantiates all the objects before the service url is resolved. I think that also means the test coverage for per service urls is not working. For us the things that change per service url are the attrname, attributes to transfer and authprocs.

Secondary would be proxy validate doesn't seem like it doesn't work. I think it is hard to know what working looks like (it probably won't be till the new year that I have a real world example)

Last bit is deciding if returning null from a controller to make tests work is good or to use RunnableResponse. @tvdijen I wonder what your thoughts are on RunnableResponse - is that something you recommend we use to wrap SSP calls (like http urils or authsource->logout()) that won't return? or stick with using mocks on those and have the controller return 'null'?

[tvdijen]

I think the RunnableResponse is the right way @pradtke, because in the future our utilities will return a response-object .. This will also make it easier to find these cases once that happens.