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

Improve the parsing of the groups property

XMLWordPrintable

    • Icon: Task Task
    • Resolution: Done
    • Icon: Neutral Neutral
    • 1.1
    • None
    • None
    • None

      In version 1.0.x, the usergroups property lookup was hardcoded. This was a problem for a couple of reasons:

      • usergroups is, I believe, Microsoft terminology, that we inherited from the previous version of the module; OIDC tends to favor groups (see here, here or here)
      • the fact that we had to hardcode it meant the module had no flexibility
      • for an external user, the error message was unclear when setting things up

      Initial solution

      I suggested to lookup the groups property in the following PR: https://git.magnolia-cms.com/projects/ENTERPRISE/repos/magnolia-sso/pull-requests/22/overview?commentId=73403

      My hope was that by doing this while adding the groups OIDC claim, everything would become clearer, and work. But it didn't actually work. At least in Keycloak, adding the groups claim to the request doesn't add any data if no mapper is configured.

      Actual solution

      As suggested here: https://www.pac4j.org/docs/clients.html#2-compute-roles-and-permissions

      Before we map the OIDC user's groups into Magnolia groups, we have to convert the OIDC user's groups into something in his OIDC user profile. (OIDC user: lives in the IDP, OIDC profile: is shared between OIDC and Magnolia during the login process)

      I have:

      • opted to use OIDC profile roles instead of groups, because there is standard API for this (solving the hardcoded value problem)
      • decided to implement an AuthorizationGenerator to streamline the code as well as give us the chance to warn the user with a relevant message at every step of the way (see screenshot)
      • decided to stay with looking up the groups property. This is a convention and we would rather have users follow it (which can easily be done at least in Okta and Keycloak), rather than allow too many use cases
      • we can always revisit this in future iterations

        Acceptance criteria

              mmichel Maxime Michel
              mmichel Maxime Michel
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Created:
                Updated:
                Resolved:

                  Task DoR