Uploaded image for project: 'Single Sign On'
  1. Single Sign On
  2. MGNLSSO-231

Improve error messages when pac4j configuration is not valid and/or add validation and/or add default values where it is reasonable

    XMLWordPrintable

Details

    • Improvement
    • Resolution: Unresolved
    • Neutral
    • None
    • 3.0.1
    • None
    • None

    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)

      Checklists

        Acceptance criteria

        Attachments

          Issue Links

            Activity

              People

                Unassigned Unassigned
                mgeljic Mikaël Geljić
                AdminX
                Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                  Created:
                  Updated:

                  Checklists

                    Task DoD