Problem/Motivation

Now it is not possible to use multiple connectors for Office 365, it is will be helpful for users from different organisations.

These changes are based on https://www.drupal.org/project/o365/issues/3240633, but tested with Drupal 9.x, additionally I have added a dependency to PHP 7.4 as a minimum viable version because it's a minimum supportable version of Drupal 9.3.x.

I kept all the existing features:

  • moving client, secret, and tenant IDs to the settings file
  • generation scopes via hooks
  • etc..

What comes via this ticket/PR?

  • Multiple SSO connectors
  • A new config entity (o365_connector)
  • Some things were moved to the entity (scopes, redirect URL)
  • Code refactoring a bit

Proposed resolution

1. Create config entity type to make possible create multiple connectors.
2. Existing configurations set as "default" entity.

Data model changes

See implementation for 2.x -> https://www.drupal.org/project/o365/issues/3240633

Issue fork o365-3295745

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:

Comments

rolki created an issue. See original summary.

rolki’s picture

rolki’s picture

Long story short.
These changes are based on https://www.drupal.org/project/o365/issues/3240633, but tested with Drupal 9.x, additionally I have added a dependency to PHP 7.4 as a minimum viable version because it's a minimum supportable version of Drupal 9.3.x.

I kept all the existing features:

  • moving client, secret, and tenant IDs to the settings file
  • generation scopes via hooks
  • etc..

What comes via this ticket/PR?

  • Multiple SSO connectors
  • A new config entity (o365_connector)
  • Some things were moved to the entity (scopes, redirect URL)
  • Code refactoring a bit
rolki’s picture

Issue summary: View changes
rolki’s picture

Status: Active » Needs review
fabianderijk’s picture

Status: Needs review » Needs work

@rolki could you rebase the branch against 3.0.x so the MR gets mergable, then I can test your fix.

Thanks!

  • fabianderijk committed 719869d on feature/issue-3295745
    Issue #3295745 - Started work on the MR, created by @rolki
    
rolki’s picture

Hi @fabianderijk, sorry, missed your message.
I still want these changes to be released, so I'm starting to update all these changes to the new version.

codebymikey made their first commit to this issue’s fork.

codebymikey’s picture

Status: Needs work » Needs review
StatusFileSize
new71.79 KB
codebymikey’s picture

StatusFileSize
new71.86 KB
new1008 bytes

vnech made their first commit to this issue’s fork.

vnech’s picture

Hi @fabianderijk,
Just wondering if you still want to merge this PR as we are actively use it and would be nice to see it as a part of your module.

fabianderijk’s picture

Hi @vnech, yes i would like to have this added to the module. I just haven't had a lot of time to check this.

In the meantime, the 5.0.x branch has been created, and I think this should be added there as well as all new features will be added there. I've c opied all changes from the MR to a MR for the 5.0.x branch. Everything seems to be working there. I've tested it, and I can login using multiple Azure apps.

Can someone check as well: https://git.drupalcode.org/project/o365/-/merge_requests/33

codebymikey’s picture

Title: Make it possible to use multiple Office 365 connectors (3.x) » Make it possible to use multiple Office 365 connectors (5.x)
Version: 3.0.x-dev » 5.0.x-dev

Planning to move https://git.drupalcode.org/project/o365/-/merge_requests/33 into an issue fork so that others may also contribute and work on it.

I noticed the o365_requirements() was still attempting to read from $settings['api_settings'] which is no longer the case.

codebymikey’s picture

StatusFileSize
new77.3 KB
new11.08 KB
batigolix’s picture

Issue tags: +finalist-sprint
fabianderijk’s picture

I will do a check on this code today.

When this works we will have to come up with a strategy on how to release this because this is a major change and may (better yet, will) break working sites. So we probably need to make a 6.0.x branch first before so we can merge this MR into that branch, write documentation on how to upgrade, etcetera.

fabianderijk’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed everything on a working intranet we use the module on. Everything is working when using the code in de MR.

I will create a 6.0.x branch to merge this into and start writing documentation on how to upgrade.

Great work everyone!

codebymikey’s picture

StatusFileSize
new80.64 KB
new41.62 KB

Attached a static patch of the current MR supporting the latest 5.0.4 version.

codebymikey’s picture

Attached a version which uses a post update hook to avoid collision with the existing #3405024: logout update hook.

codebymikey changed the visibility of the branch feature/3295745-multiple-connectors-5.0.4 to hidden.

codebymikey changed the visibility of the branch 3295745 to hidden.

bramtenhove’s picture

Great work everyone, this would be a nice addition indeed!

What are the next steps to resolve this issue? Anything I or we can help with @fabianderijk?

fabianderijk’s picture

@bramtenhove the new 6.0.x branch has been created. Can you point your MR to this branch? See https://git.drupalcode.org/project/o365/-/tree/6.0.x

vnech’s picture

StatusFileSize
new139.15 KB
vnech’s picture

Hi @fabianderijk
Could you please change the target branch to 6.x in MR #34? I can't do this (seems like I don't have permissions)...

I tested the changes from MR #34 and everything works as expected.

HTT:

  1. Apply MR #34 patch
  2. Run drush updb -y; drush cr
  3. In you settings.php add following settings:
    // Specific settings for "o365" authentication connectors.
    $settings['o365']['default']['client_id'] = '{PLACEHOLDER}';
    $settings['o365']['default']['client_secret'] = '{PLACEHOLDER}';
    $settings['o365']['default']['tenant_id'] = '{PLACEHOLDER}';
      
fabianderijk’s picture

The target branch has now changed to the 6.0.x branch.

fabianderijk’s picture

Title: Make it possible to use multiple Office 365 connectors (5.x) » Make it possible to use multiple Office 365 connectors (6.x)

uber_denis made their first commit to this issue’s fork.

uber_denis’s picture

StatusFileSize
new61.56 KB
nkoporec’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new64.21 KB

Added a new patch that adds a bit more defensive code, as we were seeing errors like:

TypeError: Drupal\\o365\\AuthenticationService::redirectToAuthorizationUrl(): Argument #1 ($o365_connector) must be of type Drupal\\o365\\O365ConnectorInterface, null given,

nkoporec’s picture

StatusFileSize
new0 bytes

Attaching a new one as the prev was based of 5.x

nkoporec’s picture

StatusFileSize
new64.21 KB

codebymikey changed the visibility of the branch feature/3295745-multiple-connectors-5.0.22 to hidden.

fabianderijk’s picture

I've tested the MR and everything works as expected. I fixed some phpcs and cspell issues and will merge this in the 6.x branch AND finally create a 6.0.0-beta1 release so we can let other people test as well.

We still need to update some of the documentation, for instance how the structure of the data in settings.php needs to be. I will, for now, add this to the release notes and the homepage of the module.

Thanks for all the good work!

fabianderijk’s picture

Version: 5.0.x-dev » 6.0.x-dev
Status: Needs review » Fixed

  • fabianderijk committed ad36da80 on 6.0.x
    Issue #3295745 by codebymikey, vnech, fabianderijk, rolki, nkoporec,...
fabianderijk’s picture

I've just added a extra commit with a small change in cspell to credit some extra people that helped in this issue

Status: Fixed » Closed (fixed)

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