dev/core#2559 Support tenancy in OAuth providers #25214
Conversation
|
No issue was found matching the number given in the pull request title. Please check the issue number. |
|
(Standard links)
|
|
@JKingsnorth test failures appear to be related. |
|
It looks like mostly undefined array key warnings. I'm also reviewing the code, and will do an |
|
Yes, should be a quick fix for the tests, my local rig wasn't working so I let the test bot at it. I'll fix em up shortly :) The test should just be: after updating and running the extension updates, you can create/edit an ms-exchange provider oauth client, and enter the tenant id. I plan to update the documentation if the PR is accepted. |
| */ | ||
| public function upgrade_5581(): bool { | ||
| $this->ctx->log->info('Applying oauth-client update 5581. Adding tenant column to the civicrm_oauth_client table.'); | ||
| CRM_Upgrade_Incremental_Base::addColumn($this->ctx, 'civicrm_oauth_client', 'tenant', 'varchar(128) NULL COMMENT "Tenant ID" AFTER guid'); |
There was a problem hiding this comment.
@JKingsnorth I believe you can change this to be
| CRM_Upgrade_Incremental_Base::addColumn($this->ctx, 'civicrm_oauth_client', 'tenant', 'varchar(128) NULL COMMENT "Tenant ID" AFTER guid'); | |
| $this->addTask('Add Tenant ID to OauthClient', 'addColumn', 'civicrm_oauth_client', 'tenant', 'varchar(128) NULL COMMENT "Tenant ID" AFTER guid'); |
There was a problem hiding this comment.
Thanks @seamuslee001, that makes more sense. Done.
|
I've altered the PR to add the logic into the CiviGenericProvider class, rather than creating a new one just for the ms-exchange. This should make it easier for other providers to add support for tenancy where required, whilst not causing bloat or breaking existing providers. But would be great to get an r-run test on sites that do use existing gmail providers [= I've also added tests (ooh, ahh). EDIT: I'll squash the commits before merge - but wanted to keep my working visible for now... |
JKingsnorth
force-pushed
the
dev/core#2559
branch
from
e5795e4
to
8e454e9
Compare
|
Squashed. Ready for any more review needed. |
|
I've applied this patch on a ms-exchange site where tenant ID is required and it works. Also applied on a gmail site and it carries on working |
|
Thanks @mattwire , and thanks for the review and merge. |
|
Wonderful. Just tried it on my development system running 5.59.alpha1 and it works! Just one comment, there didn't seem to be a confirmation that the information was saved after clicking Save. |
See https://lab.civicrm.org/dev/core/-/issues/2559