Feature #15664: Google OAuth plugin for emails #3054
Conversation
|
Seems great, but
For 1 : i ask some question and put some idea For 2 : i need to report as a new dev issue (we already have this vendor in Auth plugin for example) |
|
234 files changed? |
Lot of vendor files |
|
CI says you need to run composer install or update in web root folder. |
- Extend support for non-OAuth email plugins
gabrieljenik
added
Code review done
Unit tests added for isCurrentEmailPlugin, saveSettings and getCredentials functions.
Unit tests added.
Unit tests added for the listEmailPlugins event.
|
SonarCloud Quality Gate failed. |
|
Getting
when clicking on plugin details. |
| $this->setOAuthState($plugin, $event->get('state')); | ||
|
|
||
| header('Location: ' . $authUrl); | ||
| exit; |
|
|
||
| private function getHelpContent() | ||
| { | ||
| $this->subscribe('getPluginTwigPath'); |
There was a problem hiding this comment.
Subscribe? Is getPluginTwigPath an event?
There was a problem hiding this comment.
Where is it documented? I didn't find it on the plugin event manual page.
There was a problem hiding this comment.
Found it. I don't understand why it's needed tho, can't you just use $this->renderPartial() and it will look in the views/ folder inside the plugin automatically?
There was a problem hiding this comment.
Found it. I don't understand why it's needed tho, can't you just use
$this->renderPartial()and it will look in the views/ folder inside the plugin automatically?
Currently no. View folder are not added automatically.
There was a problem hiding this comment.
Found it. I don't understand why it's needed tho, can't you just use
$this->renderPartial()and it will look in the views/ folder inside the plugin automatically?Currently no. View folder are not added automatically.
$content = $this->api->renderTwig(__DIR__ . '/views/index.twig', $data);
https://github.com/olleharstedt/MassAction/blob/master/MassAction.php#L228
That's not enough?
There was a problem hiding this comment.
Found it. I don't understand why it's needed tho, can't you just use
$this->renderPartial()and it will look in the views/ folder inside the plugin automatically?Currently no. View folder are not added automatically.
$content = $this->api->renderTwig(__DIR__ . '/views/index.twig', $data);https://github.com/olleharstedt/MassAction/blob/master/MassAction.php#L228
That's not enough?
Oh !! You're right !
getPluginTwigPath isb only needed for Public page : to allow theme to replace the twig file !
Sorry !
Can be removed : you're right
| SettingGlobal::setSetting('emailplugin', $emailPlugin); | ||
| // If the email plugin has changed, dispatch an event to allow the new plugin to do any necessary setup. | ||
| if ($emailMethod == LimeMailer::MethodPlugin && $oldEmailPlugin != $emailPlugin) { | ||
| $event = new PluginEvent('afterSelectEmailPlugin', $this); |
There was a problem hiding this comment.
Is this new event needed, or can we use afterControllerAction? Oh wait, there's only beforeControllerAction... Should we add afterControllerAction instead? It's way more general than afterSelectEmailPlugin.
| @@ -604,6 +627,20 @@ public function Send() | |||
| $this->setError(gT('Email was not sent because demo-mode is activated.')); | |||
| return false; | |||
| } | |||
|
|
|||
| // If the email method is set to "Plugin", we need to dispatch an event to that specific plugin | |||
| // so it can perform it's logic without depending on the more generic "beforeEmail" event. | |||
There was a problem hiding this comment.
without depending on the more generic "beforeEmail" event.
Why not depend on this one? Not possible/easy?
There was a problem hiding this comment.
Should read some back and forth to remember, but I recall one of the issue was the priority among beforeEmail events.
|
This is really good stuff, I just have some concern regarding too many new events. |
Well many of them are needed by the oAuth protocol. Let's review on a call |
|
I am pretty sure we fixed this already, but in this branch I am getting 500 when I try to scan for plugins
Then when I zip and upload a plugin and click on the name to set it up
|
…Google-OAuth-plugin-3
There was a problem hiding this comment.
@gabrieljenik Binary files should not be part of PR. I already removed it in a fixing commit.
Done
Plugin Events Added
listSMTPOauthPlugins
When selecting the email method, the user should get a list of available plugins to select the active one.
This event allows plugins to enlist in the dropdown.
afterSelectSMTPOAuthPlugin
The OAuth mechanism is based on a refresh token, that the client uses to authenticate to the provider.
This should be triggered by the admin user. Afer setting the smtp provider, the admin user is invited to trigger the process with a custom message, set from this plugin event.
beforePrepareRedirectToAuthPage
Before showing the button for launching the provider AuthPage, we need to know specifics, as for example, what is the size of the popup open, or specific messages to appear on that screen with instructions. This event provides so.
beforeRedirectToAuthPage
Before redirecting to the provider AuthPage, we need to know specifics, as for example, the provider URL. Also, set the state (similar to a CSRF) for later validation of the response. This event provides so.
afterReceiveOAuthResponse
When receiving a response from OAuth, the plugin (through this event) will process the response (ideally using the provider methods), extract the refreshtoken and save the refreshtoken on the session settings.
newSMTPOAuthInitialization
When sending emails, PHPMailer needs a provider (class which extends AbstractProvider) to authenticate with the provider.
The provider needs to be set from the plugin, using the event newSMTPOAuthInitialization
Pendings:
Wish / Nice to Have
Some screenshots: