Skip to content

Conversation

@AnaisPantheor
Copy link
Contributor

@AnaisPantheor AnaisPantheor commented Dec 8, 2025

New branch based on all the latest additions to develop (previously #423 with massive merge conflicts)

SimpleSAMLphp security warnings were displayed to users using the OneLogin/internal connector, even though these warnings are only relevant to SimpleSAMLphp users.

This PR adds stricter connection type checks to SimpleSAMLphp version warnings to prevent them from displaying to users who are not actually using SimpleSAMLphp authentication.

Behat tests are expected to fail until #447 is merged into develop, all tests on #447 are passing now.


SimpleSAMLphp version and connection type validation

  • SimpleSAMLphp is NOT bundled with the plugin (users must download/install separately)
  • Only OneLogin is bundled with the plugin.
  • We need to check BOTH library detection AND version status AND connection_type together (lines 758, 781, ~820) to ensure warnings ONLY show when SimpleSAMLphp is genuinely configured and in use.

Scenario 1: User is using OneLogin/internal connector with explicit config

  • SimpleSAMLphp may or may not be installed on server.
  • connection_type = 'internal' (explicitly set in filter) .
  • SimpleSAMLphp version not checked because connection_type !== 'simplesamlphp'
  • Result: Don't show SimpleSAMLphp warnings.

Scenario 2: User is trying to use SimpleSAMLphp

  • SimpleSAMLphp is not installed (library not detected)
  • connection_type = 'simplesamlphp' (explicitly set or default)
  • Result: Show SimpleSAMLphp error message.

Scenario 3: User is actively using SimpleSAMLphp

  • SimpleSAMLphp is installed (library detected)
  • connection_type = 'simplesamlphp' (explicitly set or default)
  • Result: Show appropriate SimpleSAMLphp version warnings (critical/warning/unknown)

Scenario 4: Ambiguous - we don't know if library is detected, but config says SimpleSAMLphp

  • connection_type = 'simplesamlphp' (could be explicitly set OR default value)
  • $simplesamlphp_version_status = 'unknown' (can't get library version number)
  • Something is up, but we CAN'T KNOW WHAT EXACTLY: a) User wants SimpleSAMLphp but installation is broken/incomplete b) User is using OneLogin but hasn't explicitly set connection_type='internal' (legacy default)
  • Result: Check && $connection_type === 'simplesamlphp' and show warning "unable to determine version"

TO DO:
Help users distinguish between cases a) and b) with better messaging.

@AnaisPantheor AnaisPantheor marked this pull request as ready for review December 9, 2025 20:43
@AnaisPantheor AnaisPantheor requested a review from a team as a code owner December 9, 2025 20:43
@AnaisPantheor AnaisPantheor changed the title [SITE-5067] Adapt warning message - December '25 version [SITE-5067] Adapt warning message Dec 9, 2025
@scottbuscemi
Copy link
Contributor

@jazzsequence any chance you could review this?

@pwtyler
Copy link
Member

pwtyler commented Dec 15, 2025

I'd like to see behat -> GHA move decoupled from the bug fix in this PR.

@AnaisPantheor AnaisPantheor marked this pull request as draft December 16, 2025 19:07
@AnaisPantheor AnaisPantheor mentioned this pull request Dec 16, 2025
@github-actions
Copy link

Hi from your friendly robot! 🤖 I fixed PHPCS issues with phpcbf on 0d66b6a. Please review the changes.

@AnaisPantheor AnaisPantheor changed the title [SITE-5067] Adapt warning message [SITE-5067] Fix SimpleSAMLphp warnings showing for OneLogin/internal connector users Dec 16, 2025
@AnaisPantheor
Copy link
Contributor Author

@jazzsequence any chance you could review this?

The ticket has been split into two: the Behat tests: #447 and the optimization of the warning message (this ticket).

@AnaisPantheor AnaisPantheor marked this pull request as ready for review December 17, 2025 18:41
@AnaisPantheor AnaisPantheor requested review from pwtyler and removed request for mklasen December 18, 2025 15:30
@AnaisPantheor AnaisPantheor merged commit 8159ede into develop Dec 19, 2025
38 of 44 checks passed
@AnaisPantheor AnaisPantheor deleted the newAdaptWarningMessage branch December 19, 2025 21:11
AnaisPantheor added a commit that referenced this pull request Jan 8, 2026
…connector users (#445)

* update readme and changelog
* refactor scenarios
* Add more early returns

---------

Co-authored-by: Pantheon Robot <[email protected]>
Co-authored-by: Phil Tyler <[email protected]>
AnaisPantheor added a commit that referenced this pull request Jan 8, 2026
…connector users (#445)

* update readme and changelog
* refactor scenarios
* Add more early returns

---------

Co-authored-by: Pantheon Robot <[email protected]>
Co-authored-by: Phil Tyler <[email protected]>
AnaisPantheor added a commit that referenced this pull request Jan 9, 2026
…connector users (#445)

* update readme and changelog
* refactor scenarios
* Add more early returns

---------

Co-authored-by: Pantheon Robot <[email protected]>
Co-authored-by: Phil Tyler <[email protected]>
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.

5 participants