✨(oidc) people as an identity provider #638

qbey · 2025-01-14T14:52:29Z

Purpose

We want the people user database to also be an Identity Provider.

Ref:

Proposal

This is the very first step toward this goal:

jblagadec · 2025-02-11T13:38:51Z

COMMENTAIRE ORAL QUENTIN
Boîte mail permet de s'authentifier sur la Régie. Donne accès, couplé avec un mdp.
En passant par Proconnect : crée un user Django.

qbey · 2025-02-12T13:53:05Z

Changelog will be added when review is done (to ease the rebase process...)

sylvinus · 2025-02-12T19:48:30Z

src/backend/mailbox_oauth2/tests/test_backends.py

+    assert user is None
+
+
+def test_get_username_domain_from_email_valid():


Perhaps here we could use @pytest.mark.parametrize and include more test cases? Ideas:

Unicode only in username or domain

Spaces

Very long strings

weird characters (\0 ?)

empty username

empty domain

quotes, commas, ..

Definitely 👍

sylvinus · 2025-02-12T19:52:13Z

src/backend/mailbox_manager/models.py

+        Organization,
+        on_delete=models.PROTECT,
+        related_name="mail_domains",
+        null=True,


Is the plan to later switch this to null=False ?

A domain doesn't really require an Organization today, it's only mandatory for ProConnect for now... But when we will improve permissions, it may become:

easy to determine the organization, or at least propose a set of organization to the end user

mandatory to have an organization to be able to determine who can manage mailboxes

So yeah, it may become null=False in a near future :) While the "near future" is not clear, I did not mention it in the code ^^'

sylvinus · 2025-02-12T19:55:28Z

src/backend/mailbox_manager/utils/dimail.py

@@ -341,6 +342,7 @@ def import_mailboxes(self, domain):
                            # secondary email is mandatory. Unfortunately, dimail doesn't
                            # store any. We temporarily give current email as secondary email.
                            status=enums.MailboxStatusChoices.ENABLED,
+                            password=make_password(None),


This could use a comment. Are we absolutely sure it can't lead to successful logins with empty passwords?

Yes, when None it starts with the "invalid password" prefix.

See https://github.com/django/django/blob/5.1.6/django/contrib/auth/hashers.py#L103-L106
and https://github.com/django/django/blob/5.1.6/django/contrib/auth/hashers.py#L28-L42

I will add a comment. I don't like testing the framework but I can test "None" password cannot login, just in case.

sylvinus · 2025-02-12T20:01:20Z

src/backend/mailbox_manager/api/client/serializers.py

@@ -34,7 +36,7 @@ def create(self, validated_data):
        """
        Override create function to fire a request on mailbox creation.
        """
-        mailbox = super().create(validated_data)
+        mailbox = super().create(validated_data | {"password": make_password(None)})


same feedback here

sylvinus · 2025-02-12T20:22:44Z

src/backend/mailbox_manager/models.py

@@ -202,6 +218,9 @@ class Mailbox(BaseModel):
        choices=MailboxStatusChoices.choices,
        default=MailboxStatusChoices.PENDING,
    )
+    dn_email = models.EmailField(_("email"), blank=True, unique=True, editable=False)


what is dn_ ? isn't it a potential issue to store both this and the local_part ?

"dn_" stands for "denormalized". In the first place, I tried to add a generated field but it does not work with "join" (it was a concat of "local_part", "@" and "domain__name" 💥 ), so I used a field which is computed on save to store the denormalized email value.

For sure, if you make Mailbox.objects.filter().update(local_part="blah"), the dn_email will be broken...

The dn_email is required to make the Django admin work for mailbox with password...

I can add a comment to say we should only use this field for the Django admin.

Could this be a reason to do the reverse and compute local_part while only storing the full email?

We could, but it implies a bit of work, if we need it we may do it in another PR if it's ok for you.

qbey

@sylvinus Thanks for the review, I will improve that :)

qbey · 2025-02-12T21:14:54Z

src/backend/mailbox_manager/utils/dimail.py

@@ -341,6 +342,7 @@ def import_mailboxes(self, domain):
                            # secondary email is mandatory. Unfortunately, dimail doesn't
                            # store any. We temporarily give current email as secondary email.
                            status=enums.MailboxStatusChoices.ENABLED,
+                            password=make_password(None),


Yes, when None it starts with the "invalid password" prefix.

See https://github.com/django/django/blob/5.1.6/django/contrib/auth/hashers.py#L103-L106
and https://github.com/django/django/blob/5.1.6/django/contrib/auth/hashers.py#L28-L42

I will add a comment. I don't like testing the framework but I can test "None" password cannot login, just in case.

qbey · 2025-02-12T21:15:26Z

src/backend/mailbox_oauth2/tests/test_backends.py

+    assert user is None
+
+
+def test_get_username_domain_from_email_valid():


Definitely 👍

qbey · 2025-02-12T21:22:57Z

src/backend/mailbox_manager/models.py

+        Organization,
+        on_delete=models.PROTECT,
+        related_name="mail_domains",
+        null=True,


A domain doesn't really require an Organization today, it's only mandatory for ProConnect for now... But when we will improve permissions, it may become:

easy to determine the organization, or at least propose a set of organization to the end user

mandatory to have an organization to be able to determine who can manage mailboxes

So yeah, it may become null=False in a near future :) While the "near future" is not clear, I did not mention it in the code ^^'

qbey · 2025-02-12T21:30:31Z

src/backend/mailbox_manager/models.py

@@ -202,6 +218,9 @@ class Mailbox(BaseModel):
        choices=MailboxStatusChoices.choices,
        default=MailboxStatusChoices.PENDING,
    )
+    dn_email = models.EmailField(_("email"), blank=True, unique=True, editable=False)


"dn_" stands for "denormalized". In the first place, I tried to add a generated field but it does not work with "join" (it was a concat of "local_part", "@" and "domain__name" 💥 ), so I used a field which is computed on save to store the denormalized email value.

For sure, if you make Mailbox.objects.filter().update(local_part="blah"), the dn_email will be broken...

The dn_email is required to make the Django admin work for mailbox with password...

I can add a comment to say we should only use this field for the Django admin.

sylvinus · 2025-02-12T21:49:14Z

src/backend/mailbox_oauth2/backends.py

+        # Check if the account is locked
+        if self._check_login_attempts(email):
+            logger.warning("Account locked due to too many failed attempts: %s", email)
+            Mailbox().set_password(password)


could use the same comment as below (moreover, it's the first). great technique btw!

sylvinus · 2025-02-12T22:00:36Z

src/backend/mailbox_oauth2/tests/test_middleware.py

+    assert response.status_code == 200
+
+    # assert the user has a session
+    assert not client.session.is_empty()


Random thought, is there an expiration time on this session? it should always be short-lived, right?

You are right, even if the session cannot be used in any other context, I reduced it to one minute.

sylvinus · 2025-02-12T22:20:32Z

Great patch congrats!

Given the sensitive nature of providing OIDC identities, I think we need to do a careful security review.

For instance, we should look into security headers on the login page (https://www.cerberauth.com/blog/openid-connect-provider-security-headers/) Potential list (for next and/or django?)

{
    'X-Frame-Options': 'DENY',
    'X-Content-Type-Options': 'nosniff',
    'X-XSS-Protection': '1; mode=block',
    'Strict-Transport-Security': 'max-age=31536000; includeSubDomains',
    'Content-Security-Policy': "default-src 'self'; frame-ancestors 'none'",
    'Referrer-Policy': 'strict-origin-when-cross-origin'
}

qbey · 2025-02-12T23:31:48Z

@sylvinus In production, the Django headers are already quite well set, see for instance on /api/v1.0/authenticate/:

{
    'Date': 'Wed, 12 Feb 2025 23:25:20 GMT', 
    'Content-Type': 'text/html; charset=utf-8', 
    'Content-Length': '0',
    'Connection': 'keep-alive', 
    'Allow': 'GET', 
    'Vary': 'origin, Accept-Language, Cookie', 
    'X-Frame-Options': 'DENY',
    'Content-Language': 'en-us', 
    'Strict-Transport-Security': 'max-age=31536000; includeSubDomains',
    'X-Content-Type-Options': 'nosniff', 
    'Referrer-Policy': 'same-origin', 
    'Cross-Origin-Opener-Policy': 'same-origin',
}

sampaccoud · 2025-02-18T13:17:28Z

docs/identityProvider.md

+
+### Password strength
+
+There is currently no constraint on the password strength has it can only be defined by Django administrators,


Suggested change

There is currently no constraint on the password strength has it can only be defined by Django administrators,

There is currently no constraint on the password strength as it can only be defined by Django administrators,

sampaccoud · 2025-02-18T13:22:45Z

src/backend/mailbox_manager/admin.py

    """Admin for mailbox model."""

    list_display = ("__str__", "domain", "status", "updated_at")
    list_filter = ("status",)
    search_fields = ("local_part", "domain__name")
-    readonly_fields = ["updated_at", "local_part", "domain"]
+    # readonly_fields = ["updated_at", "local_part", "domain"]


dead code or should be readonly?

sampaccoud · 2025-02-18T13:32:56Z

src/backend/mailbox_oauth2/views.py

+        email = request.data.get("email")
+        password = request.data.get("password")


Why ot use a serializer? It could validate that what is submitted is an email and validate password format, returning proper errors etc.?

To be able to provide a SIRET in the ProConnect IdP process we need to be able to link a mail domain to its organization. For now this is not mandatory, as we can't detect the organization and need a frontend process to clarify it.

This allows to use `people` as an identity provider using OIDC and local users. This commit is partial, because it does not manage a way to create "local" users and the login page is the admin one, which can't be used for non staff users or login with email.

This configures local environment to test login through people: - Keycloak configuration of the IdP (people) - Add Keycloak Application in people The only user who can login for now is "admin".

To have a better user experience, we want the login page to in the frontend.

Few fixes to allow the keycloak dev stack to use people as an Identity Provider. This requires the update of the bitnami keycloak chart we use.

This is making too much noise when developing using the tilt stack...

This is a simple test to assert a user can login via people when setup as an identity provider.

This provides a light documentation about the way to configure people as an IdentityProvider.

qbey self-assigned this Jan 14, 2025

qbey added frontend Relative to the frontend backend feature labels Jan 14, 2025

qbey force-pushed the qbey/people-as-identity-provider branch 2 times, most recently from 799ef24 to 3a2f5a4 Compare January 14, 2025 15:13

qbey force-pushed the qbey/people-as-identity-provider branch 15 times, most recently from fba081c to 35a46ae Compare February 11, 2025 11:24

qbey force-pushed the qbey/people-as-identity-provider branch 8 times, most recently from 18a46da to 89ac911 Compare February 12, 2025 10:41

qbey requested review from sampaccoud, sylvinus and sdemagny February 12, 2025 13:51

qbey force-pushed the qbey/people-as-identity-provider branch from 15126b7 to 205f12c Compare February 12, 2025 15:55

sylvinus reviewed Feb 12, 2025

View reviewed changes

qbey commented Feb 12, 2025

View reviewed changes

sylvinus reviewed Feb 12, 2025

View reviewed changes

qbey force-pushed the qbey/people-as-identity-provider branch from 205f12c to 52fcb60 Compare February 12, 2025 23:33

sampaccoud reviewed Feb 18, 2025

View reviewed changes

qbey force-pushed the qbey/people-as-identity-provider branch 3 times, most recently from 775f3bf to 17bdd09 Compare February 20, 2025 09:29

qbey requested a review from lunika February 20, 2025 09:30

qbey added 8 commits February 21, 2025 11:34

🗃️(mailbox_manager) add organization to domain

ee984e8

To be able to provide a SIRET in the ProConnect IdP process we need to be able to link a mail domain to its organization. For now this is not mandatory, as we can't detect the organization and need a frontend process to clarify it.

🧑‍💻(demo) configure people as an IdP

288237b

This configures local environment to test login through people: - Keycloak configuration of the IdP (people) - Add Keycloak Application in people The only user who can login for now is "admin".

💄(oidc) add login page in the frontend

37a615d

To have a better user experience, we want the login page to in the frontend.

🧑‍💻(tilt) allow use of people as an IdP

a56e191

Few fixes to allow the keycloak dev stack to use people as an Identity Provider. This requires the update of the bitnami keycloak chart we use.

🔇(helm) disable sentry on local stack

48ed361

This is making too much noise when developing using the tilt stack...

✅(oidc) add IdP e2e test for login

eb71ad0

This is a simple test to assert a user can login via people when setup as an identity provider.

📝(oidc) describe the IdP aspect of people

d7bfdc4

This provides a light documentation about the way to configure people as an IdentityProvider.

qbey force-pushed the qbey/people-as-identity-provider branch from 17bdd09 to d7bfdc4 Compare February 21, 2025 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨(oidc) people as an identity provider #638

✨(oidc) people as an identity provider #638

qbey commented Jan 14, 2025 •

edited

Loading

jblagadec commented Feb 11, 2025

qbey commented Feb 12, 2025

sylvinus Feb 12, 2025

qbey Feb 12, 2025

sylvinus Feb 12, 2025

qbey Feb 12, 2025

sylvinus Feb 12, 2025 •

edited

Loading

qbey Feb 12, 2025

sylvinus Feb 12, 2025

sylvinus Feb 12, 2025

qbey Feb 12, 2025

sylvinus Feb 12, 2025

qbey Feb 13, 2025

qbey left a comment

qbey Feb 12, 2025

qbey Feb 12, 2025

qbey Feb 12, 2025

qbey Feb 12, 2025

sylvinus Feb 12, 2025

sylvinus Feb 12, 2025

qbey Feb 12, 2025

sylvinus commented Feb 12, 2025

qbey commented Feb 12, 2025

sampaccoud Feb 18, 2025

sampaccoud Feb 18, 2025

sampaccoud Feb 18, 2025

		assert user is None


		def test_get_username_domain_from_email_valid():


		### Password strength

		There is currently no constraint on the password strength has it can only be defined by Django administrators,

		email = request.data.get("email")
		password = request.data.get("password")

✨(oidc) people as an identity provider #638

Are you sure you want to change the base?

✨(oidc) people as an identity provider #638

Conversation

qbey commented Jan 14, 2025 • edited Loading

Purpose

Proposal

jblagadec commented Feb 11, 2025

qbey commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvinus Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvinus commented Feb 12, 2025

qbey commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbey commented Jan 14, 2025 •

edited

Loading

sylvinus Feb 12, 2025 •

edited

Loading