Skip to content

Conversation

@johanib
Copy link
Contributor

@johanib johanib commented Mar 3, 2025

List of things to discuss before this can be merged:

  • Can we add a better description to docs/filter_commands.md?
  • The part below * Continue to Consent/StepUp in library/EngineBlock/Corto/Module/Service/SRAMInterrupt.php is largely duplicated, but not 100%. I'm concerned this code has many scenario's that are tested via AssertionConsumer.php. The duplicated code here has no such coverage. So we should probably centralize it. But how?
  • How should the merging of the attributes from SBS fuction? See tests/unit/OpenConext/EngineBlockBundle/Sbs/SbsAttributeMergerTest.php

@johanib johanib force-pushed the feature/sram-interrupt_rebase branch 4 times, most recently from 7f78ee5 to 246205b Compare March 4, 2025 13:51
@johanib johanib linked an issue Mar 4, 2025 that may be closed by this pull request
5 tasks
@johanib johanib force-pushed the feature/sram-interrupt_rebase branch 2 times, most recently from b6f27bf to 228cdb0 Compare March 4, 2025 14:00
@johanib johanib force-pushed the feature/sram-interrupt_rebase branch 4 times, most recently from 68b8c60 to da272f3 Compare March 13, 2025 09:03
@johanib johanib marked this pull request as ready for review March 13, 2025 09:04
Copy link
Contributor

@pablothedude pablothedude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an initial review mainly focusing on the code. This feature however should be thoroughly reviewed functionally before it's merged back and after all open todo's are addressed.

feature_enable_consent: true
feature_stepup_sfo_override_engine_entityid: false
feature_enable_idp_initiated_flow: true
feature_enable_sram_interrupt: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be the default?

@baszoetekouw baszoetekouw requested a review from mrvanes March 17, 2025 09:02
@mrvanes
Copy link
Contributor

mrvanes commented Mar 18, 2025

Ik ben aan het testen, maar loop tegen deze conditie aan in SRAMTestFilter:

if ($this->_serviceProvider->getCoins()->collabEnabled() === false) {
    return;
}

Hoe krijg ik die in manage, onder welke versie van manage (ik draai nu 9.0.0.1)?
Mist er misschien een row in de database voor collab_enabled?

@mrvanes
Copy link
Contributor

mrvanes commented Mar 19, 2025

Manage collabEnabled() problem has been solved. However, EB now gives me a "Session Lost" on returning to https://engine.dev.openconext.local/authentication/idp/process-sraminterrupt?ID=...
I see you have tested with sram.base_url served by EB itself so you may have lifted on more session information than you can count on in real life?

I suggest you use the SBS stub server that was originally included in my branch to test your redirects:
https://github.com/OpenConext/OpenConext-engineblock/tree/feature/sram-interrupt/sbs-stub

@johanib
Copy link
Contributor Author

johanib commented Mar 24, 2025

Manage collabEnabled() problem has been solved. However, EB now gives me a "Session Lost" on returning to https://engine.dev.openconext.local/authentication/idp/process-sraminterrupt?ID=... I see you have tested with sram.base_url served by EB itself so you may have lifted on more session information than you can count on in real life?

I suggest you use the SBS stub server that was originally included in my branch to test your redirects: https://github.com/OpenConext/OpenConext-engineblock/tree/feature/sram-interrupt/sbs-stub

The mock SBS does/should not need or rely on any session state. It receives the continue_url on the authz call, like in the example python script, and redirects the browser to that url when the interrupt is performed.
As it's only there for facilitating the functional tests and development, it does not support concurrent requests etc. It just stores the last received authz payload on filesystem.

When testing, does the ID=... contain an actual value?
If so, it seems like that value cannot be matched to a valid message request ID in the browser session.

@mrvanes
Copy link
Contributor

mrvanes commented Mar 24, 2025

Yes, the ?ID= parameter contains everything in the original continue_url. EngineBlock fails to continue on the contents of this continue_url. That's the bug. I would advice against testing this against an internal EB redirect but externalising, like this will be done in the real deploy.

@mrvanes mrvanes force-pushed the feature/sram-interrupt_rebase branch 7 times, most recently from ae05ebe to 6eccd69 Compare March 31, 2025 11:15
@mrvanes mrvanes force-pushed the feature/sram-interrupt_rebase branch 5 times, most recently from 612494e to 157ebdb Compare April 7, 2025 14:51
@mrvanes mrvanes force-pushed the feature/sram-interrupt_rebase branch 3 times, most recently from 4b1347a to 3d2cb21 Compare April 17, 2025 15:41
@mrvanes mrvanes force-pushed the feature/sram-interrupt_rebase branch from 3d2cb21 to 49c5eaf Compare April 17, 2025 15:46
@mrvanes mrvanes marked this pull request as ready for review April 22, 2025 07:00
@mrvanes mrvanes force-pushed the feature/sram-interrupt_rebase branch 2 times, most recently from 264f496 to 6aa50ac Compare May 6, 2025 14:14
@mrvanes mrvanes force-pushed the feature/sram-interrupt_rebase branch from 6aa50ac to df1777a Compare May 6, 2025 14:21
@baszoetekouw baszoetekouw self-assigned this May 12, 2025
@johanib johanib mentioned this pull request Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Connect SBS as external authorization engine to Engineblock

5 participants