Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support of OAUTHBEARER #79

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

EdouardVanbelle
Copy link
Contributor

Hello please find the implementation of the OAUTHBEARER authentification
(https://www.rfc-editor.org/rfc/rfc7628.html)

@schengawegga
Copy link
Contributor

Thank you. I am very busy at the moment. I will have a look next week.

Signed-off-by: Edouard Vanbelle <[email protected]>
@EdouardVanbelle
Copy link
Contributor Author

Hello @schengawegga, no worries. Let me know if you need explanations

@Neustradamus
Copy link

@schengawegga: It has been merged in Roundcube!

Now, I hope that SCRAM will be added in Roundcube soon!
I hope help from @EdouardVanbelle...

@EdouardVanbelle
Copy link
Contributor Author

Hello @schengawegga I added a new commit to this PR: when specifying the fake 'OAUTH' method,
library will elect the best method between 'XOAUTH2' and 'OAUTHBEARER'

Happy new year !

@schengawegga
Copy link
Contributor

Hey @EdouardVanbelle ,

thanks for your contribution. I will review and test this soon. But maybe not in this year, because my family wants some time with me, too ;-)

I wish you a happy new year.

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Jan 6, 2024

Hello @Neustradamus I do not work anymore in OVHCloud
By the way you can contact me via edouard at vanbelle dot fr

@Neustradamus
Copy link

@EdouardVanbelle: Thanks, I will do it, you can remove your comment :)

Net/SMTP.php Outdated Show resolved Hide resolved
@@ -1112,17 +1124,51 @@ protected function authGSSAPI($uid, $pwd, $authz = '')
public function authXOAuth2($uid, $token, $authz, $conn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All auth* methods are protected, so this one should too, I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not touch on previous methods already in place, I can correct it to move it into protected, no clue if it is already used outside of this library, I don't think so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected it in favor of a protected method
@schengawegga let me know your preferences

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is like @alecpl, to protect all auth* methods, to be straight on this kind of methods.
But, i as you said @EdouardVanbelle, if anybody uses this method from outside, it will break backwards compatibility, and have to be released on a new major version. I also do not think, that anybody uses this from outside, but it is possible.

@alecpl @jparise what do you think about that? can we do a bugfix to change authXOAuth2 method to protected without releasing this under a new major version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While waiting any feedback I kept the original signature and added a FIXME in comment

Net/SMTP.php Outdated Show resolved Hide resolved
Net/SMTP.php Outdated Show resolved Hide resolved
Net/SMTP.php Show resolved Hide resolved
@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Jan 15, 2024

Thank you @alecpl for your review !

@schengawegga, I hope you will find the time to review this small change ;)

   * OAUTH pseudo method will elect either XOAUTH2 or OAUTHBEARER according to server's capabilities

Signed-off-by: Edouard Vanbelle <[email protected]>
@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Feb 3, 2024

Hello @schengawegga, apologies for the delay, busy time...
I applied your suggestions

@schengawegga schengawegga merged commit 57c969e into pear:master Feb 9, 2024
9 checks passed
@schengawegga schengawegga added this to the 1.12.0 milestone Feb 9, 2024
@Neustradamus
Copy link

@EdouardVanbelle: Good job!

If you can help @alecpl to have SCRAM supports into Roundcube, it will be nice...

@schengawegga
Copy link
Contributor

Thank you very much @EdouardVanbelle @alecpl
I will do a release of 1.12.0 in the next days.

@EdouardVanbelle
Copy link
Contributor Author

Hello @schengawegga do you know when you can arrange time to release your new version ? :)

@schengawegga
Copy link
Contributor

@EdouardVanbelle Version 1.12.0 released

@Neustradamus
Copy link

@schengawegga: Thanks, good job too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants