Problem/Motivation

openid_connect has added suport for multiple clients of every provider. This means that there can be multiple keycloak configurations on a single site. Since the keycloak module provides the keycloak_sso functionality that alters the entire sites behavious it doesn't make sense to provide that setting on each keycloak client configuration.

Steps to reproduce

Install the keycloak 2.x-dev and the openid_connect 3.x modules on a site. Then go to the openid_connect list page and notice that it is possible to create multiple clients of the keycloak type.

Proposed resolution

I propose that we remove the hard coded client konfiguration static that determines that the machine name of the keycloak client must be keycloak and also that we lift out the keycloak_sso setting to a single keycloak settings page. On this page it should also be possible to select which of the keycloak clients on the site that should be used for the /user/login page.

CommentFileSizeAuthor
#9 3390391.diff25.46 KB_tarik_

Issue fork keycloak-3390391

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

auth created an issue. See original summary.

auth opened merge request !32

auth’s picture

auth commented
Status: Active » Needs review

The functionality mentioned in the proposed resolution is implemented in the linked issue fork

j-lee’s picture

j-lee
he/him
Primary language German
Location 🇩🇪🇪🇺
commented

I noticed the same problem when I tried a Keykloak implementation. The MR looks good so far in a quick review.

auth’s picture

auth commented

Would it be possible for one of the maintainers to review this issue and see if this can be accepted?

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Didn't test it out yet. Quickly went through the code and added a few remarks

auth’s picture

auth commented
Status: Needs work » Needs review

Thank you for the feedback.

I have provided a updatedb hook for the schema change and fixed the issue with settings.keycloak_i18n_mapping that should be settings.keycloak_i18n.mapping.

I left the initial config as it was since I belive that to be a reasonable default value.

auth’s picture

auth commented

If a maintainer have the chance to look at this I will try to be quick in my response time to move this patch forward towards acceptance.

_tarik_’s picture

_tarik_ commented
StatusFileSize
new 3390391.diff25.46 KB

Here diff from the MR 32 for those who won't have a list from commits in composer.json

Also, thanks @auth for his contribution it works like a charm for me.

j-lee’s picture

j-lee
he/him
Primary language German
Location 🇩🇪🇪🇺
commented
Status: Needs review » Needs work

The MR requires a rebase with the main branch to resolve merge conflicts.
Two issues with code quality are still open (src/Service/KeycloakService.php:371 and src/Service/KeycloakService.php:388).

I quickly looked over it and didn't see anything else.
Once the problems above are resolved, I can perform a manual test.

It would be great if we could get this into a release quickly.

j-lee’s picture

j-lee
he/him
Primary language German
Location 🇩🇪🇪🇺
commented

I performed a rebase locally and can confirm that the MR works.

The changes from #3382665: Replace Drupal login with Keycloak single sign-on (SSO) is not working will be overwritten.

A selection of Keycloak instances is offered, which can be used to replace the standard user login. This is then also used when logging in.

Unfortunately, I am currently unable to push. So, still NW.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

Unfortunately, I am currently unable to push. So, still NW.

Did you click the "get push access" button next to the issue fork name?

j-lee’s picture

j-lee
he/him
Primary language German
Location 🇩🇪🇪🇺
commented

Yup, maybe blocked or an network issue here.

j-lee’s picture

j-lee
he/him
Primary language German
Location 🇩🇪🇪🇺
commented
Assigned: auth » Unassigned
Status: Needs work » Needs review

Now i could push the rebased MR.
Added a commit with code quality fixes.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Are there configuration implications for those currently not using this? So, wondering, will the configurations carry over when upgrading or will this require configuration updates for existing users (say we released with this hypothetically).

I'm wondering what to expect, any possible disruptions for those adopting this without knowing? Or can we safely merge this and everyones configuration will continue to work regardless?

j-lee’s picture

j-lee
he/him
Primary language German
Location 🇩🇪🇪🇺
commented

I tested the behavior and came to the following conclusion:

1. Keycloak module installed without MR:

1.1 One Keycloak instance configured:

  • Login with Drupal works
  • Login with Keycloak works

1.2 Two Keycloak instances configured:

  • Login with Drupal works
  • Login with Keycloak works (depending on which button is clicked on both instances)

-> “Replace Drupal login with Keycloak” is enabled, but has no effect in 2.2.x.

2. Keycloak module installed with MR:

2.1 One Keycloak instance configured:

2.1.1 “Replace Drupal login with Keycloak single sign-on (SSO)” disabled:

  • Login with Drupal works
  • Login with Keycloak works

2.1.2 “Replace Drupal login with Keycloak single sign-on (SSO)” enabled:

  • Only one Keycloak instance can be selected.
  • Drupal login is replaced with Keycloak.

2.2 Two Keycloak instances are configured:

2.2.1 “Replace Drupal login with Keycloak single sign-on (SSO)” disabled:

  • Login with Drupal works
  • Login with Keycloak works (both instances)

2.2.2 “Replace Drupal login with Keycloak single sign-on (SSO)” enabled:

  • One of the two Keycloak instances can be selected.
  • Drupal login is replaced with Keycloak.

-> The “keycloak.settings” configuration is updated accordingly.

3. Keycloak module installed without MR, update to MR-Version:

  • drush updb installs hook_update_10000
  • The config key “openid_connect.client.XXX” still has the key “keycloak.sso.key”.
  • The new config key “keycloak.settings” is created and not activated. The first client is selected.
  • Login without further configuration as in Points 1.1 or 1.2
  • After configuration, it behaves as in Point 2.
j-lee’s picture

j-lee
he/him
Primary language German
Location 🇩🇪🇪🇺
commented

@joseph.olstad Users who do not replace the login are not affected and everything continues to work as usual.

Users who replace the login should check their configuration. The setting will not be transferred to the new configuration.
However, even if the setting is transferred, users should check the configuration.

  • joseph.olstad committed a44b6126 on 2.2.x authored by auth 
    feat: #3390391 Add keycloak support for multple instances from...
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Tagged and released 2.2.3-rc1

Please remind me in January to publish 2.2.3

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.