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
| Comment | File | Size | Author |
|---|---|---|---|
| #36 | 3295745--PR-36-PHP8.3.patch | 64.21 KB | nkoporec |
| #33 | 3295745--PR-34-PHP8.3.patch | 61.56 KB | uber_denis |
| #28 | 3295745--PR-34.patch | 139.15 KB | vnech |
Issue fork o365-3295745
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
Comment #3
rolki commentedComment #4
rolki commentedLong 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:
What comes via this ticket/PR?
Comment #5
rolki commentedComment #6
rolki commentedComment #7
fabianderijk@rolki could you rebase the branch against 3.0.x so the MR gets mergable, then I can test your fix.
Thanks!
Comment #9
rolki commentedHi @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.
Comment #11
codebymikey commentedComment #12
codebymikey commentedComment #14
vnech commentedHi @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.
Comment #15
fabianderijkHi @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
Comment #16
codebymikey commentedPlanning 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.Comment #18
codebymikey commentedComment #19
batigolixComment #20
fabianderijkI 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.
Comment #21
fabianderijkI'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!
Comment #22
codebymikey commentedAttached a static patch of the current MR supporting the latest 5.0.4 version.
Comment #23
codebymikey commentedAttached a version which uses a post update hook to avoid collision with the existing #3405024: logout update hook.
Comment #26
bramtenhove commentedGreat 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?
Comment #27
fabianderijk@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
Comment #28
vnech commentedComment #29
vnech commentedHi @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:
drush updb -y; drush crsettings.phpadd following settings:Comment #30
fabianderijkThe target branch has now changed to the 6.0.x branch.
Comment #31
fabianderijkComment #33
uber_denis commentedComment #34
nkoporecAdded 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,
Comment #35
nkoporecAttaching a new one as the prev was based of 5.x
Comment #36
nkoporecComment #38
fabianderijkI'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!
Comment #40
fabianderijkComment #42
fabianderijkI've just added a extra commit with a small change in cspell to credit some extra people that helped in this issue