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.

Sign up for GitHub

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

Jump to bottom

OAuth improvements (+ add user_create hook) #9274

Merged
merged 1 commit into from
Dec 26, 2023
Merged

OAuth improvements (+ add user_create hook) #9274

merged 1 commit into from
Dec 26, 2023

Conversation

EdouardVanbelle
Copy link
Contributor

@EdouardVanbelle EdouardVanbelle commented Dec 17, 2023 •
edited
Loading

Dear team, please find OAuth improvements

  • Core: Improv.: can specify an array on *_auth_type, lib will elect the best auth type according server's capabilities
  • OAuth: Refact.: migrate login flow into hooks (more evolutive code and less dependency to core code)
  • OAuth: Fix: logger prefix (include prefix during login phase)
  • OAuth: Feat: add user_create hook (map OIDC claims when creating a new user)
  • OAuth: Lean: prepare OAUTHBEARER or XOAUTH2 in Sieve, SMTP, IMAP

This pull request also prepare the support of OAUTHBEARER and can already be merged, I will add OAUTHBEARER once:

Many thanks for your time

@Neustradamus
Copy link

@EdouardVanbelle: Excellent!

@EdouardVanbelle EdouardVanbelle force-pushed the feat/hooks-and-oauth branch 2 times, most recently from 127feb2 to 67acba6 Compare December 18, 2023 07:58
@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Dec 18, 2023 •
edited
Loading

Hello I also have another commit that brings the PKCE support (security improvement)
more details: https://datatracker.ietf.org/doc/html/rfc7636

the pending change is here: https://github.com/EdouardVanbelle/roundcubemail/compare/feat/hooks-and-oauth...EdouardVanbelle:roundcubemail:feat/oauth-pkce?expand=1

Let me know if you want I add it in this PR

This will cover this request: #8757

@EdouardVanbelle
Copy link
Contributor Author

Hello, I also implemented the use of the nonce, which is a security complement to PKCE
will be pushed on another request : https://github.com/EdouardVanbelle/roundcubemail/compare/feat/hooks-and-oauth...EdouardVanbelle:roundcubemail:feat/oauth-pkce?expand=1 )

@alecpl
Copy link
Member

alecpl commented Dec 23, 2023

  • the smaller PRs are better, I'd prefer everything auth_type related in a separate PR
  • regarding auth_type, I thought it would be better to implement method auto-selection in Net_Sieve and Net_SMTP. I.e. I'd make "OAUTH" type label, when set, these packages would use "OAUTHBEARER", and if not available "XOAUTH2". Setting type as an array looks needlessly overcomplicated.

@EdouardVanbelle
Copy link
Contributor Author

Hello I am going to split PR per features

Regarding OAUTH in libraries, I took the option that these libraries are low level
I also opt for an array, I thought it was more evolutive, seems that I took the wrong decision
I will check deeper according your suggestions ;-)

@EdouardVanbelle
Copy link
Contributor Author

Here is the point @alecpl

This PR is now related to code refactorization to remove dependencies in core code with Oauth class and maximise use of hooks (better evolutivity + program/actions/login/oauth.php part is 95% similar to index.php > login's section)
Once validated I will send a PR per features:

  • oauth: user_create (code ready)
  • oauth: pkce (security improvement) (code ready)
  • oauth: nonce (security improvement) (code ready)

While waiting approval I will step back on XOAUTH2/OAUTHBEARER routing/decision that fit your suggestions

Happy end of year celebrations

alecpl
alecpl requested changes Dec 25, 2023
View reviewed changes
program/include/rcmail_oauth.php Outdated Show resolved Hide resolved
program/include/rcmail_oauth.php Outdated Show resolved Hide resolved
@EdouardVanbelle EdouardVanbelle force-pushed the feat/hooks-and-oauth branch 3 times, most recently from 954a3e0 to 755bc36 Compare December 25, 2023 18:08
@EdouardVanbelle
OAuth: code refactorization
fe9ca60 
 * OAuth: Refact.: migrate login flow into hooks (more evolutive code and less dependency to core code)
 * OAuth: Fix: logger prefix (include prefix during login phase)

Signed-off-by: Edouard Vanbelle <edouard@vanbelle.fr>
@EdouardVanbelle
Copy link
Contributor Author

Hello @alecpl I added this PR: #9284 in order to fix coding style not related to this change

@alecpl alecpl merged commit 320bdef into roundcube:master Dec 26, 2023
14 of 15 checks passed
@Neustradamus
Copy link

Neustradamus commented Dec 26, 2023 •
edited
Loading

@EdouardVanbelle: Nice, good job!

Linked to:

@Neustradamus Neustradamus mentioned this pull request Dec 26, 2023
Merged
@EdouardVanbelle
Copy link
Contributor Author

Hello @Neustradamus , why not, seems my dovecot supports SCRAM, will check it when I handle time to do it

@Neustradamus
Copy link

@EdouardVanbelle: Nice, thanks in advance :)

About Dovecot, please note that @stephanbosch is on SCRAM-SHA-1-PLUS/SCRAM-SHA-256-PLUS: dovecot/core@main...stephanbosch:dovecot-core:sasl-scram-plus

He has already done several years ago several lines about SCRAM: https://github.com/search?q=repo%3Adovecot%2Fcore+scram&type=commits&s=author-date&o=desc

For -PLUS variants, there are 3 possibilities:

  • tls-unique for TLS =< 1.2
  • tls-server-end-point
  • tls-exporter for TLS = 1.3

About Cyrus-SASL:

About Cyrus-Imapd:

This was referenced Dec 27, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@EdouardVanbelle EdouardVanbelle deleted the feat/hooks-and-oauth branch December 29, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecpl alecpl Awaiting requested review from alecpl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EdouardVanbelle @Neustradamus @alecpl