SSP-2030_OIDC_module_switch_to_ProcessingChain_for_authproc_support_consent_mod

[ioigoume]

#179

[ioigoume]

@cicnavi
the last commit on the wip-version-6 branch broke the dependencies injected from the modules/oidc/src/Services/Container.php class.

[cicnavi]

@cicnavi the last commit on the wip-version-6 branch broke the dependencies injected from the modules/oidc/src/Services/Container.php class.

What exactly, how should I reproduce this?

[ioigoume]

@cicnavi ,
before the commit that changed the routes this is how the authorize flow worked:

• public/authorize.php
• RoutingService initialized the ContainerService and created and injected all the dependencies to all the other services
• Called the controller

After the change the module uses the Symfony routing service. As a result, the old ContainerService is never called and the dependencies are not created/injected in the controller as before. For example the $requestRuleManager dependency is null.

In order to reproduce I am running the authorization flow.

[cicnavi]

Well yes, we are moving to Symfony routes and container. In v6, I was thinking to have both Symfony routes and "old" public/*.php routes available. The reason for both routes is that some RPs will have "old" configuration fetched from the configuration endpoint, and in this way they will still be able to authenticate using old routes and have time to fetch new configuration with new endpoints.

So, it's important that the old routes still function if they are used. I thought that I messed something up with them, but if I understand correctly, the problem for you now is that they are not used by default any more.

So, yes, if you are going to implement new stuff, please use Symfony routes and container... this is how SSP now handles requests, and we are actually pretty late to implement this...

[cicnavi]

Oh, and if I can help in any way, please let me now. Feel free to ping me on Slack (join group SimpleSAMLphp if you haven't already).

[ioigoume]

@cicnavi can you please provide some feedback on this PR. I refactored the existing tests so that they run correctly. As soon as we have a consensus about the new code I will extend the tests.

[cicnavi]

Will do as soon as I can

…

On Mon, Jun 17, 2024, 11:48 AM Ioannis Igoumenos ***@***.***> wrote: @cicnavi <https://github.com/cicnavi> can you please provide some feedback on this PR. I refactored the existing tests so that they run correctly. As soon as we have a consensus about the new code I will extend the tests. — Reply to this email directly, view it on GitHub <#228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYHTDGUZRHPDPAHLJN3ZP3ZH25HNAVCNFSM6AAAAABJEZQ5EGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZTGA3DENBSGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[pradtke]

I think it looks pretty good. I'd like to remove the redirect that happens from calling ProcessingChain::resumeProcessing($state);

I think for tests, I think you need one for AuthorizationControllerTest that invokes the code path that calls authenticationService->processRequest and a test in AuthenticationServiceTest to test processRequest

[cicnavi]

There are some conformance test issues, but are not visible here since no action runs were started. @tvdijen what is the policy on running actions in PRs?

[cicnavi]

@ioigoume You can also try to run conformance tests locally as per https://github.com/simplesamlphp/simplesamlphp-module-oidc/blob/wip-version-6/CONFORMANCE_TEST.md

[tvdijen]

[..] @tvdijen what is the policy on running actions in PRs?

We do this on any other repository, but this repo's test-suite has diverged over the years and I stopped keeping it in sync

[pradtke]

I think we also still need tests.

[cicnavi]

[..] @tvdijen what is the policy on running actions in PRs?

We do this on any other repository, but this repo's test-suite has diverged over the years and I stopped keeping it in sync

I don't follow. When I do a pull request, actions do run. Why they didn't run for ioigoumes PR?

[tvdijen]

I don't follow. When I do a pull request, actions do run. Why they didn't run for ioigoumes PR?

Ah! Because they only run against master or branches called release-*..

https://github.com/simplesamlphp/simplesamlphp-module-oidc/blob/master/.github/workflows/test.yaml#L7

We could add bugfix/* and feature/* but it will require some discipline to keep the naming-convention

[pradtke]

There is the line about $authState being a string that I don't understand.

I also don't see a test for AuthorizationControllerTest that results in $this->authenticationService->processRequest being called.

Thanks

[pradtke]

I did some more local testing with the authprocs and things worked as I expected.
I made a minor commit to add some info on running a docker image with preprodwarning authproc enabled to test redirects, and I enabled one basic authproc for the conformance tests (just to confirm that invoking authproc doesn't make anything explode, there is no validation of what the authproc does in the conformance test)

@cicnavi I'm ready to merge this. If you agree then I can do a squash and merge. Thanks

[cicnavi]

I haven't looked at it yet, but if you say it's OK, then go ahead. Please consider noting it in the upgrade log. For example, will this change now also run any "global" authproc from config.php? If so, someone could have SAML only authproc set globally which will then break because it won't have expected state.

…

On Thu, Jul 11, 2024, 12:59 AM Patrick ***@***.***> wrote: I did some more local testing with the authprocs and things worked as I expected. I made a minor commit to add some info on running a docker image with preprodwarning authproc enabled to test redirects, and I enabled one basic authproc for the conformance tests (just to confirm that invoking authproc doesn't make anything explode, there is no validation of what the authproc does in the conformance test) @cicnavi <https://github.com/cicnavi> I'm ready to merge this. If you agree then I can do a squash and merge. Thanks — Reply to this email directly, view it on GitHub <#228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYHTDFLOA4Q3D5TMIHPC4TZLW4ETAVCNFSM6AAAAABJEZQ5EGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRRGY3DGOJWGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[pradtke]

Good call out on the documentation. I updated it to reflect that we can now support doing redirects in the authproc.
The ProcessinChain does try to load additional authproc filters from the main config.php but it will be looking for them under the key authproc.oidc rather than authproc.idp that saml uses. I made a note of that behavior changes in the upgrade notes incase someone, for some unknown reason, has that key already defined.

[cicnavi]

Great, thanks!

…

On Fri, Jul 12, 2024, 1:23 AM Patrick ***@***.***> wrote: Merged #228 <#228> into wip-version-6. — Reply to this email directly, view it on GitHub <#228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYHTDANDGRA7A5IZYOGUSDZL4HV3AVCNFSM6AAAAABJEZQ5EGVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGQ3TSMJSGIZTOOI> . You are receiving this because you were mentioned.Message ID: <simplesamlphp/simplesamlphp-module-oidc/pull/228/issue_event/13479122379@ github.com>