Skip to content

oauth: feat: use OIDC claims on user creation#9286

Merged
alecpl merged 1 commit intoroundcube:masterfrom
EdouardVanbelle:feat/oauth-user-create
Dec 29, 2023
Merged

oauth: feat: use OIDC claims on user creation#9286
alecpl merged 1 commit intoroundcube:masterfrom
EdouardVanbelle:feat/oauth-user-create

Conversation

@EdouardVanbelle
Copy link
Contributor

Hello please find the user_create hook that uses the OIDC's identify when creating a new user

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should validate the values coming from outside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, here what I did:

  1. ensure config setup only allowed keys (user & host are for example forbidden)
  2. normalizing user_email & language, then I check validity

I also added tests

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 just changed the language check, the list_languages() was too strict

-                            // check that language is supported
-                            $languages = $this->rcmail->list_languages();
-                            if (!array_key_exists($value, $languages)) {
+                            // check that language is at the correct format
+                            if (!preg_match('/^[a-z]{2}(\_[A-Z]{2,})?$/', $value)) {
                                 rcube::raise_error([

@alecpl
Copy link
Member

alecpl commented Dec 27, 2023

@Neustradamus Please, stop adding these references. It's misleading, not everything is related.

@EdouardVanbelle EdouardVanbelle force-pushed the feat/oauth-user-create branch 2 times, most recently from 960e77c to 6f11ec7 Compare December 28, 2023 17:59
Copy link
Member

Choose a reason for hiding this comment

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

That's not going to work with languages like "ast" or "es_419"

Copy link
Contributor Author

@EdouardVanbelle EdouardVanbelle Dec 28, 2023

Choose a reason for hiding this comment

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

What about this simplification?

   // check that language is in the correct format (over simplification of RFC5646)
   if (!preg_match('/^[a-z]{2,8}(\_[a-z0-9]{1,8})*$/i', $value)) {
  ...

Which already sanitize data and ensure you are not going to have temptative hacks like $language="../../../password" or similar

What is the requirement on the user's property language

  • does it need to be the wished language from the customer? (Permit respective translation to be added lately)
  • or does it need to be a truly and currently supported language by roundcube?

If it has to be a supported language, the best option I see is to change rcube->language_prop() from protected to public and use it in this hook

What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

In another place we're using /^[a-zA-Z0-9_-]+$/, but I'd add a length limit, lets say 8, and it will be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

please note, I saw that rcube->language_prop() is capturing browser's accepted language via

            if (preg_match('/^([a-z]+)[_-]([a-z]+)$/i', $lang, $m))

I added this: #9292

Signed-off-by: Edouard Vanbelle <edouard@vanbelle.fr>
@alecpl alecpl merged commit ffa298d into roundcube:master Dec 29, 2023
@EdouardVanbelle EdouardVanbelle deleted the feat/oauth-user-create branch December 29, 2023 20:36
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.

3 participants