Repository navigation
Sending Mails via SMTP and XOAUTH2 authentication mechanism - #36665
Conversation
2c30746 to
765ca32
Compare
bc00a5f to
4d73f47
Compare
|
Added a test that waits for a merge of greenmail-mail-test/greenmail#838. Testing with a real SMTP service providing token based auth follows. |
fe0b22e to
b1171b6
Compare
7c8b16d to
17abcd7
Compare
e399184 to
c9fd818
Compare
|
This PR is IMHO already in a good shape: The functionality looks good, the are tests, the tests are green (the one failure is the external link check that is unrelated). After talking to @srose, may someone from the @keycloak/core-iam teams have a look? If this should be a @keycloak/core-clients issue instead, please let me know. |
|
+1 considering the upvotes from the community on the issue we should consider this for review with a higher priority. I am going to triage the original issue as such and assign it to a milestone so it is handled before release of the next minor. |
c9fd818 to
cb9b2de
Compare
|
@srose mind if we mark this one as ready for review, or do you believe more work is needed? |
|
@jonkoops thank you and @ahus1 for looking into it, feedback would be great! Despite having a GreenMail way of testing I was not able to find a working SMTP + XOAUTH2 service to have a real world test. Thats the reason for leaving it at the draft-state. Please proceed, Review feedback would be great and we can go back to draft if necessary? |
f507752 to
be671d3
Compare
|
@srose could you do another rebase? It seems some conflicts have popped up. |
be671d3 to
d7642df
Compare
|
Rebase done 👍 |
|
TimeOffsetTest might be unstable, I created issue #38180 for that. |
d7642df to
95bf84e
Compare
95bf84e to
15b2a91
Compare
|
Improved the documentation to help configuration of existing providers and rebased with conflicts once more. |
There was a problem hiding this comment.
@srose Nice! Sorry it took longer to review, but I hope we can manage to make it for Keycloak 26.2.0 (preliminary planned for end of March).
I've added few minor comments inline (no big deals). Besides that, it can be needed to rebase as there is conflict in release notes (Hint: Better to add your release notes somewhere in the middle rather than to the bottom as it decrease the probability of conflict :-)
There was a problem hiding this comment.
This constructor public DefaultEmailSenderProvider(KeycloakSession session) { can be removed entirely in this PR. We don't need to keep backwards compatibility for the code inside keycloak-services module as those are internal classes. There are some subclasses of DefaultEmailSenderPRovider in the testsuite, but I see you already updated them, so this constructor could be simply deleted as it is not used anywhere AFAIK.
There was a problem hiding this comment.
AFAIK not so preferred to use var in the Keycloak Java codebase (my vote is to update this var in here as in other places in this PR, but not strictly needed as we don't have policy yet for Keycloak to not using var, so if you strongly prefer to keep it, feel free to keep...).
There was a problem hiding this comment.
As I think consistency is a good thing I tried to remove all 'var' introduced by me. Thanks for the hint.
@srose / @mposolda - Stian updated the release notes by priority as a preparation for the release. That being said, add your release note updates rather to the end of the file when you rebase the PR (at least this time). |
Closes keycloak#17432 Signed-off-by: Sebastian Rose <sebastian.rose@gmail.com>
15b2a91 to
14094e2
Compare
Closes #17432
First draft, gathering tokens from additional settings, make existing tests work.
Find a way for testing looks compilcated after some hours.