EMAIL-163 Support for OAuth2 authentication #354
Conversation
…s/EMAIL-163 # Conflicts: # src/changes/changes.xml
commons-email2-jakarta/src/main/java/org/apache/commons/mail2/jakarta/Email.java
Outdated
Show resolved
Hide resolved
commons-email2-jakarta/src/main/java/org/apache/commons/mail2/jakarta/Email.java
Outdated
Show resolved
Hide resolved
commons-email2-javax/src/main/java/org/apache/commons/mail2/javax/Email.java
Outdated
Show resolved
Hide resolved
commons-email2-javax/src/main/java/org/apache/commons/mail2/javax/Email.java
Outdated
Show resolved
Hide resolved
|
@garydgregory Merged your changes and resolved all issues |
|
@garydgregory Reminder, thank you 😅 |
|
@garydgregory Are you happy with the PR? |
There was a problem hiding this comment.
Hi @sgoeschl ,
Please accept my apologies for the delay in reviewing.
I've added some comments.
The biggest problem I see is: How do we know this works? It can't be "trust me".
In other Java projects, I've used KeyCloak (with and without Docker, but using Docker is best) to test OAUth2 support. Why not do this here?
See the libraries com.nimbusds:oauth2-oidc-sdk, org.testcontainers:testcontainers, org.testcontainers:junit-jupiter, com.github.dasniko:testcontainers-keycloak and the Maven plugin io.fabric8:docker-maven-plugin.
| public static final String MAIL_SMTP_AUTH = "mail.smtp.auth"; | ||
|
|
||
| /** If set to true, tries to authenticate the user using an OAuth2 token. */ | ||
| public static final String MAIL_SMTP_AUTH_MECHANISMS = "mail.smtp.auth.mechanisms"; |
There was a problem hiding this comment.
The Javadoc since tag is missing.
|
|
||
| // tests | ||
| assertNotNull(mailSession); | ||
| assertEquals("XOAUTH2", mailSession.getProperties().getProperty("mail.smtp.auth.mechanisms")); |
|
|
||
| // tests | ||
| assertNotNull(mailSession); | ||
| assertEquals("XOAUTH2", mailSession.getProperties().getProperty("mail.smtp.auth.mechanisms")); |
| } | ||
|
|
||
| if (isOAuth2Required()) { | ||
| properties.put(EmailConstants.MAIL_SMTP_AUTH_MECHANISMS, "XOAUTH2"); |
There was a problem hiding this comment.
Refactor "XOAUTH2" into a constant.
|
Hi everyone, I just wanted to bring this PR back into focus, as it seems to be stuck. The functionality here is (IMHO) quite important for users, and leaving it unmerged might risk discouraging adoption or pushing people away who rely on this feature. From what I can see, the change is essentially just forwarding a parameter to Jakarta Mail. That’s a very low-risk adjustment, and while of course thorough tests are always valuable, in this specific case the cost–benefit trade-off of waiting for a full test suite may not justify the delay. I'm not sure @sgoeschl is prepared to invest this kind of time, since he is no longer really active. Would it be possible to merge this with at least a minimal test that checks whether the parameter has been set (or even with a clear TODO for additional tests), so that the community can start benefiting from it? Alternatively, maybe a committer or contributor closer to the project could help add the tests, if they are considered strictly necessary. This way the feature doesn’t stay blocked, users get what they need, and the code quality expectations of the project can still be met in a collaborative way. What do you think? |
3 participants
Thanks for your contribution to Apache Commons! Your help is appreciated!
Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.