Issue 53534: Support Microsoft's Graph Protocol API for authentication via OAuth2 token#7381
Issue 53534: Support Microsoft's Graph Protocol API for authentication via OAuth2 token#7381labkey-bpatel wants to merge 32 commits intodevelopfrom
Conversation
… a provider-based abstraction for email transport. Refactor the existing SMTP logic into its own provider. Manage OAuth2 token caching. Enforce that only one transport method can be configured.
…o fb_mail_transport_via_graph
…test, update emailTest.jsp to manually test Graph credentials, add an action to test uploads >= 4MB.
|
WARNING: This PR appears to have the default title generated by GitHub. Please use something more descriptive. |
…date testing. Add comments.
…h external URL images and data URI images.
…test around this change. Other minor updates.
labkey-adam
left a comment
There was a problem hiding this comment.
Code looks good, generally speaking. No required changes, but please review my questions & comments.
My biggest concern is the fact that this adds 70MB of JAR files to API. This is an extraordinary amount of new code for a single feature that will be used rarely. Were there any other options that would reduce the dependencies? Can any dependencies be excluded? If we can't slim it down, we may need to consider pushing the Graph API transport and dependencies into a separate module.
We could trim ~10MB by cutting azure-identity, but that would require us to handle token management - not a significant saving. I am leaning toward pushing these changes to a standalone module for now, then we can revisit moving it into premiumModules down the road. Thoughts? |
…er class to a new module in premiumModules. Rename test action to 'GraphEmailTestWithAttachmentAction'. Remove setting activeProvider in setSession. Use StringUtilsLabKey.joinWithConjunction()
labkey-adam
left a comment
There was a problem hiding this comment.
Please see a few suggestions and questions. I'm approving and no further review is needed at this stage.
I do think we should consider some refactoring to make key classes less tied to SMTP and Jakarta Mail. MailHelper special cases SMTP in a few places, making some of the methods applicable to SMTP only. It's now odd that ViewMessage and MultipartMessage extend Jakarta Mail classes and hold an SMTP session, even when used with Graph. Ideally, MailHelper and the message classes would be completely generic to the provider. Dumbster could construct & configure an SmtpTransportProvider, and set that as MailHelper's active provider. However, I don't think any of this is required right now. I'd prefer to get this tested and merged, and get the client to test it. Then circle back with some improvements as a follow-on.
| { | ||
| _session = DEFAULT_SESSION; | ||
| } | ||
| return _smtpProvider.getSession(); |
There was a problem hiding this comment.
Should this return null if _smtpProvider is not configured?
There was a problem hiding this comment.
_smtpProvider is a static final field initialized at class load time, and _smtpProvider.getSession() returns null if the smtp properties are not set (otherwise returns a session if smtp properties are set).
| public static Session getSession() | ||
| { | ||
| return _session; | ||
| return new MultipartMessage(getSession()); |
There was a problem hiding this comment.
What happens here if getSession() returns null?
There was a problem hiding this comment.
getSession() returns null when SMTP is not configured, which would cause a failure at send time.
The Graph transport doesn't call this method since it doesn't have a concept of Jakarta Mail Session. Since this only relevant for SMTP transport, renaming the method to getSmtpSession() as you suggested clarifies things.
| return session; | ||
| public static void setSession(Session session) | ||
| { | ||
| _smtpProvider.setSession(session); |
There was a problem hiding this comment.
Should this throw if called when Graph transport is active?
| } | ||
|
|
||
| return session; | ||
| public static void setSession(Session session) |
There was a problem hiding this comment.
Clarify method name? setSmtpSession()
| * Returns the SMTP session for creating messages | ||
| */ | ||
| @Nullable | ||
| public static Session getSession() |
There was a problem hiding this comment.
Clarify? getSmtpSession()
Yup, that makes sense! I thought about decoupling SMTP specific things out MailHelper and Dumbster; however the scope kept increasing so left it as-is, but a follow up improvements sounds like a fine plan! |
Rationale
Microsoft plans to retire SMTP Basic Authentication in March 2026. In response, one of our clients’ IT department has requested a shift to OAuth-based authentication
Since SMTP Oauth 2.0 client credential flow with non-interactive sign-in (for email notifications) is not supported we’ll need to use Microsoft’s Graph API instead of SMTP protocol
This PR adds a Graph transport method as an alternative to SMTP, which enables mail delivery via OAuth2 authentication method.
Spec
Related Pull Requests
Changes
Tasks 📍