[MGNLSSO-231] Improve error messages when pac4j configuration is not valid and/or add validation and/or add default values where it is reasonable Created: 23/Jan/23  Updated: 07/Mar/23

Status: Open
Project: Single Sign On
Component/s: None
Affects Version/s: 3.0.1
Fix Version/s: None

Type: Improvement Priority: Neutral
Reporter: Mikaël Geljić Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
causality
caused by MGNLSSO-206 Upgrade Pac4j to latest version 5.7.x Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Epic Link: SSO maintenance
Team: AdminX

 Description   

Steps to reproduce

On behalf of fmangold and partner trying the update, with same config as on 3.0.0

...
clients: 
  oidc.id: {...}
  oidc.secret: {...}
  oidc.scope: openid profile email
  oidc.discoveryUri: {...}/auth/realms/mgnl/.well-known/openid-configuration
  oidc.preferredJwsAlgorithm: RS256
  oidc.authorizationGenerators: groupsAuthorization

Expected results

  • Customer expected a routine patch update.
  • Changelog barely mentions a new property, but not that it is now required and needs action!
  • At least expected an error or warning in logs if something needs to be set, not an NPE.

Actual results

Webapp starts successfully, but editors cannot log in. NullPointerException found in logs.

java.lang.NullPointerException
	org.pac4j.core.client.BaseClient.lambda$retrieveCredentials$0(BaseClient.java:75)
	java.base/java.util.Optional.ifPresent(Optional.java:183)
	org.pac4j.core.client.BaseClient.retrieveCredentials(BaseClient.java:72)
	org.pac4j.core.client.IndirectClient.getCredentials(IndirectClient.java:145)
	org.pac4j.core.engine.DefaultCallbackLogic.perform(DefaultCallbackLogic.java:75)
	info.magnolia.sso.SsoCallbackServlet.doGet(SsoCallbackServlet.java:71)
	...
	info.magnolia.sso.SsoLoginFilter.lambda$doFilter$1(SsoLoginFilter.java:81)
	org.pac4j.core.engine.DefaultSecurityLogic.perform(DefaultSecurityLogic.java:160)
	info.magnolia.sso.SsoLoginFilter.doFilter(SsoLoginFilter.java:79)

Workaround

Docs now mention the oidc.clientAuthenticationMethod property, but without much guidance as for allowed values. The following did the trick:

oidc.clientAuthenticationMethod: client_secret_basic

Development notes

See thread in the Pac4j upgrade PR: https://git.magnolia-cms.com/projects/ENTERPRISE/repos/magnolia-sso/pull-requests/181/overview?commentId=107268

  • I guess we might need to consider some "dirty" code to post-process the Pac4j Config after all, and add a default value?
    • Would need to apply to both MP and Yaml config
    • Applying it only to first/main OIDC config is fine (vs. http bearer auth)

Generated at Mon Feb 12 10:52:19 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.