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 https://github.com/simplesamlphp/simplesamlphp-module-oidc/pull/228#issuecomment-2173062420, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYHTDGUZRHPDPAHLJN3ZP3ZH25HNAVCNFSM6AAAAABJEZQ5EGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZTGA3DENBSGA . You are receiving this because you were mentioned.Message ID: @.***>

[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

[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

[cicnavi]

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

Oh, right. But it wouldn't make a difference since he is having those branches in its own fork. I would rather run them on any PR

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

But again, that wouldn't run them here since he has them in his own fork I guess... Would it be ok for you that we run those on all PRs? Something like:

on:
  pull_request:
    types: [opened, synchronize, reopened, closed]

[ioigoume]

@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

@cicnavi thank you for the hint. I managed to run the tests manually directly from the https://staging.certification.openid.net site

[tvdijen]

But again, that wouldn't run them here since he has them in his own fork I guess... [..]

No, no, it is because of the name of the target branch!

Would it be ok for you that we run those on all PRs

Like I said, the testsuite here is much different than the rest of the repositories, and I consider this repo yours... do as you please!

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.33333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 53.32%. Comparing base (354b4ca) to head (d5e7997).

Files	Patch %	Lines
src/Services/Container.php	0.00%	6 Missing :warning:
src/Services/StateService.php	33.33%	4 Missing :warning:
src/Services/AuthenticationService.php	98.18%	1 Missing :warning:
src/Utils/Checker/Rules/MaxAgeRule.php	0.00%	1 Missing :warning:
src/Utils/Checker/Rules/PromptRule.php	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## wip-version-6 #228 +/- ## =================================================== + Coverage 52.99% 53.32% +0.32% - Complexity 1101 1105 +4 =================================================== Files 117 118 +1 Lines 4029 4051 +22 =================================================== + Hits 2135 2160 +25 + Misses 1894 1891 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[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 https://github.com/simplesamlphp/simplesamlphp-module-oidc/pull/228#issuecomment-2221663962, 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 https://github.com/simplesamlphp/simplesamlphp-module-oidc/pull/228 into wip-version-6.

— Reply to this email directly, view it on GitHub https://github.com/simplesamlphp/simplesamlphp-module-oidc/pull/228#event-13479122379, 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>