[Mailer] Improve oauth setup #52585

dbu · 2023-11-14T15:54:11Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	-
License	MIT

Setting up the Symfony Mailer with XOAuth based authentication is possible but quite complicated. I hacked something together in https://gist.github.com/dbu/3094d7569aebfc94788b164bd7e59acc but with a few changes, I would only need to implement the token provider and configure it.

I guess this is too late for 7.0, so it should go to 7.1. I will adjust the target branch once 7.1 is created.

carsonbot · 2023-11-14T15:54:17Z

Hey!

Thanks for your PR. You are targeting branch "7.0" but it seems your PR description refers to branch "7.1".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

dbu · 2023-11-14T16:20:24Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+        foreach ($config['smtp']['authenticators'] ?? [] as $authenticator) {
+            $authenticators[] = new Reference($authenticator);
+        }
+        $container->getDefinition('mailer.transport_factory.smtp')->setArgument(3, $authenticators);


according to mailer_transport.php this service is the EsmtpTransportFactory, but the CI tells that we somehow end up setting an argument to "Symfony\Component\Mailer\Mailer" - any idea what i am messing up here?

the weird error message could be because of inlining the transport factory services. It might be inside one of the inlined services inside the mailer service.

is that a bug of the DI container, or should i be doing something differently?

I would not call it a bug. The hard thing is that inline definitions don't have an id. So that's hard to know how to mention them in error messages.

if it is not a bug, what do i need differently to set the parameter on the correct place?

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

nicolas-grekas · 2023-11-20T09:52:40Z

Did you consider adding the oauth token providen to core? Could this be done instead of the current extra interface? Is there an approach that wouldn't require adding this extra interface - or at least making XOAuth2Authenticator unaware of it? I don't really like the instanceof check there 😬

dbu · 2023-11-20T13:16:49Z

Did you consider adding the oauth token providen to core? Could this be done instead of the current extra interface? Is there an approach that wouldn't require adding this extra interface - or at least making XOAuth2Authenticator unaware of it?

oauth seems to be more of a concept than an actual standard. every provider is doing it differently. if symfony had a fully fledged oauth component with support for all the major players, we could maybe make this use that. but to me an interface is really the way to go here.

oauth means sending a post with some json body to some url. but which field names to use, whats required or optional and what values to use e.g. for the scope is totally provider dependent.

for office365/exchange, the values happen to be

    private const OAUTH_URL = 'https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token';
    private const SCOPE = 'https://outlook.office365.com/.default';
    private const GRANT_TYPE = 'client_credentials';

        $data = [
            'client_id' => $this->clientId,
            'client_secret' => $this->clientSecret,
            'scope' => self::SCOPE,
            'grant_type' => self::GRANT_TYPE,
        ];

    // post that as x-www-form-urlencoded to
    UriTemplate::expand(self::OAUTH_URL, [
            'tenant' => $this->tenant,
    ]));

I don't really like the instanceof check there 😬

i think the null check is required for BC.

nicolas-grekas · 2023-11-20T13:24:14Z

Sorry I wrote "instanceof" but that's inaccurate. I meant this line:
https://github.com/symfony/symfony/pull/52585/files#diff-3ac0365eb0e52c96210c4fc1848acd4be3eb71ceb86aed561b76b8d1d3626b6dR42

nicolas-grekas · 2023-11-20T13:26:11Z

src/Symfony/Component/Mailer/Transport/Smtp/Auth/XOAuth2Authenticator.php

@@ -32,6 +39,7 @@ public function getAuthKeyword(): string
     */
    public function authenticate(EsmtpTransport $client): void
    {
-        $client->executeCommand('AUTH XOAUTH2 '.base64_encode('user='.$client->getUsername()."\1auth=Bearer ".$client->getPassword()."\1\1")."\r\n", [235]);
+        $token = $this->tokenProvider ? $this->tokenProvider->getToken() : $client->getPassword();


Suggested change

$token = $this->tokenProvider ? $this->tokenProvider->getToken() : $client->getPassword();

$token = $this->tokenProvider?->getToken() : $client->getPassword();

But I'm wondering about this line as now we give two ways to pass that secret - what if both are set for example? I feel like it'd be better to provide only one way to pass that secret - the existing one if possible :) Does that make sense? (it doesn't have to :) )

the issue with the existing one is that oauth tokens tend to be short-lived. In your worker process, you cannot just set the token as the password in the Transport as this will not properly refresh it when it expires.

changing the client to fetch the password dynamically would be a large refactor, and would mix responsibilities for the auth method.

the thing is that an oauth token can not be statically configured. and in a long running process it might expire before the process ends, so each time we need it we need to be ready to re-fetch it.

we could inject some TokenProviderInterface into the client and use that for getPassword(), but i don't think that would be better.

…ticator.php Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>

nicolas-grekas

Got it for the geToken??getPassword logic, makes sense to me now.
Here are some suggestions to help move forward.

nicolas-grekas · 2023-11-30T20:15:47Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -2091,6 +2091,15 @@ private function addMailerSection(ArrayNodeDefinition $rootNode, callable $enabl
                                ->end()
                            ->end()
                        ->end()
+                        ->arrayNode('smtp')
+                            ->fixXmlConfig('authenticator')


instead of adding a new config entry, I'd suggest passing a tagged_locator in the service definition
we should define the default authenticators as services with low priority so that then adding a new authenticator is just a matter of creating a new class that implements AuthenticatorInterface, thanks to registerForAutoconfiguration.

nicolas-grekas · 2023-11-30T20:16:56Z

src/Symfony/Component/Mailer/Transport/Smtp/Auth/AuthTokenProviderInterface.php

+ * Usually with OAuth, you will need to do some web request to fetch the token.
+ * You also want to cache the token for as long as it is valid.
+ */
+interface AuthTokenProviderInterface


TokenProviderInterface?

nicolas-grekas · 2023-11-30T20:19:13Z

src/Symfony/Component/Mailer/Transport/Smtp/EsmtpTransportFactory.php

+    /**
+     * @param AuthenticatorInterface[]|null $authenticators
+     */
+    public function __construct(EventDispatcherInterface $dispatcher = null, HttpClientInterface $client = null, LoggerInterface $logger = null, array $authenticators = null)


I'm not sure we need the new argument: calling setAuthenticators on the resulting instance should be enough

nicolas-grekas · 2023-11-30T20:20:29Z

Note that it'd be cool to contribute the office365 bridge while at it :)

nicolas-grekas · 2023-11-30T20:21:09Z

Oh and maybe also an AbstractOauthTokenProvider?

dbu · 2023-12-13T13:03:20Z

thanks for the feedback. yeah that sounds good. i will try to find time, might be after christmas only 🙈

the office365 provider would go into a new Office365 namespace at https://github.com/symfony/symfony/tree/7.1/src/Symfony/Component/Mailer/Bridge and need a composer.json for the subtree split, right?

i will see if an abstract base class makes sense that could be shared in the messenger package.

nicolas-grekas · 2024-01-02T17:15:53Z

the office365 provider would go into a new Office365 namespace

yes!

pounard · 2024-02-15T14:23:01Z

Hello, @dbu @nicolas-grekas, I stumbled here while doing some research, because I am since a few days working a new component proposal, that does exactly the job of the introduced here AuthTokenProvider, and more. Would you be interested in participating instead of adding this new interface in this PR ?

dbu · 2024-02-15T19:34:11Z

i do not have much time at the moment, so more than glad if you put some brain energy into proposing a nice architecture for the auth token system. i can try to give my input on the proposal when you have a draft ready, and will be happy to rebase this PR when/if your proposal gets merged.

pounard · 2024-02-16T12:40:23Z

@dbu thanks, I will in few days I hope, open an issue for it.

pounard · 2024-02-20T17:20:23Z

@dbu if you have a few minutes to spare, I'd very much like to have your sentiment about #54013, thanks in advance !

pounard · 2024-02-20T20:01:02Z

Did you consider adding the oauth token providen to core?

@nicolas-grekas I did ! Please see my PR #54013 - your insights would be invaluable !

carsonbot added Status: Needs Review Feature labels Nov 14, 2023

carsonbot added this to the 7.0 milestone Nov 14, 2023

dbu force-pushed the mailer-oauth branch from 3da2c68 to c39f25a Compare November 14, 2023 16:03

dbu commented Nov 14, 2023

View reviewed changes

stof reviewed Nov 14, 2023

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd Outdated Show resolved Hide resolved

improve oauth setup for mailer

5106e21

dbu force-pushed the mailer-oauth branch from c39f25a to 5106e21 Compare November 15, 2023 07:29

nicolas-grekas modified the milestones: 7.0, 7.1 Nov 17, 2023

nicolas-grekas added the Mailer label Nov 17, 2023

carsonbot changed the title ~~improve oauth setup for mailer~~ [Mailer] improve oauth setup for mailer Nov 17, 2023

nicolas-grekas changed the title ~~[Mailer] improve oauth setup for mailer~~ [Mailer] Improve oauth setup Nov 17, 2023

nicolas-grekas changed the base branch from 7.0 to 7.1 November 17, 2023 15:56

nicolas-grekas reviewed Nov 20, 2023

View reviewed changes

Update src/Symfony/Component/Mailer/Transport/Smtp/Auth/XOAuth2Authen…

09acfdf

…ticator.php Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>

nicolas-grekas reviewed Nov 30, 2023

View reviewed changes

pounard mentioned this pull request Feb 20, 2024

[AccessToken] A component for fetching and caching remote services access tokens #54013

Open

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

	$token = $this->tokenProvider ? $this->tokenProvider->getToken() : $client->getPassword();
	$token = $this->tokenProvider?->getToken() : $client->getPassword();

Uh oh!

[Mailer] Improve oauth setup #52585

Are you sure you want to change the base?

[Mailer] Improve oauth setup #52585

Conversation

dbu commented Nov 14, 2023

Uh oh!

carsonbot commented Nov 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolas-grekas commented Nov 20, 2023

Uh oh!

dbu commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Nov 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu commented Dec 13, 2023

Uh oh!

nicolas-grekas commented Jan 2, 2024

Uh oh!

pounard commented Feb 15, 2024

Uh oh!

dbu commented Feb 15, 2024

Uh oh!

pounard commented Feb 16, 2024

Uh oh!

pounard commented Feb 20, 2024

Uh oh!

pounard commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dbu commented Nov 20, 2023 •

edited

Loading

nicolas-grekas commented Nov 30, 2023 •

edited

Loading

nicolas-grekas commented Nov 30, 2023 •

edited

Loading

pounard commented Feb 20, 2024 •

edited

Loading