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

✨(backend) add option to configure list of required OIDC claims #525

Merged
merged 1 commit into from
Dec 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ and this project adheres to

## Added

🔧(backend) add option to configure list of required OIDC claims #525
🔧(helm) add option to disable default tls setting by @dominikkaminski #519

## Changed
Expand Down
12 changes: 12 additions & 0 deletions src/backend/core/authentication/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ def get_userinfo(self, access_token, id_token, payload):
_("Invalid response format or token verification failed")
) from e

# Validate required claims
missing_claims = [
claim
for claim in settings.USER_OIDC_REQUIRED_CLAIMS
if claim not in userinfo
]
if missing_claims:
raise SuspiciousOperation(
_("Missing required claims in user info: %(claims)s")
% {"claims": ", ".join(missing_claims)}
)

Comment on lines +60 to +71
Copy link
Collaborator

@lebaudantoine lebaudantoine Dec 27, 2024

Choose a reason for hiding this comment

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

according to the spec, these claims should be referred as essential claims. Please take a look here https://openid.net/specs/openid-connect-core-1_0.html

Essential Claim
Claim specified by the Client as being necessary to ensure a smooth authorization experience for the specific task requested by the End-User.

Copy link
Collaborator

@lebaudantoine lebaudantoine Dec 27, 2024

Choose a reason for hiding this comment

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

You might prefer to override the base method, and call it in get_or_create_user, as proposed in the initial implementation. get_userinfo should have a single responsibility, getting the data.

    def verify_claims(self, claims):
        """Verify the provided claims to decide if authentication should be allowed."""

        # Verify claims required by default configuration
        scopes = self.get_settings("OIDC_RP_SCOPES", "openid email")
        if "email" in scopes.split():
            return "email" in claims

        LOGGER.warning(
            "Custom OIDC_RP_SCOPES defined. "
            "You need to override `verify_claims` for custom claims verification."
        )

        return True

https://github.com/mozilla/mozilla-django-oidc/blob/2c2334fdc9b2fc72a492b5f0e990b4c30de68363/mozilla_django_oidc/auth.py#L84

Copy link
Collaborator

Choose a reason for hiding this comment

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

Capture d’écran 2024-12-27 à 16 40 05

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting! 🙏
Note that checking the presence of the "sub" claim was already wrong then 🤔
Let's fix that before it is released!

Copy link
Member Author

Choose a reason for hiding this comment

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

@lebaudantoine Here you go: #531

Copy link
Collaborator

@lebaudantoine lebaudantoine Dec 27, 2024

Choose a reason for hiding this comment

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

Kind of, checking the presence of "sub" was naive and simple.
As soon as we introduce more complexity (a list check, a new setting, …) I would stick with the Mozilla django oidc philosophy

return userinfo

def get_or_create_user(self, access_token, id_token, payload):
Expand Down
84 changes: 84 additions & 0 deletions src/backend/core/tests/authentication/test_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -365,3 +365,87 @@ def get_userinfo_mocked(*args):
klass.get_or_create_user(access_token="test-token", id_token=None, payload=None)

assert models.User.objects.count() == 1


# Required claims


@override_settings(
OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo",
USER_OIDC_REQUIRED_CLAIMS=["email", "sub", "address"],
)
@responses.activate
def test_authentication_get_userinfo_required_claims_missing():
"""Ensure SuspiciousOperation is raised if required claims are missing."""

responses.add(
responses.GET,
re.compile(r".*/userinfo"),
json={
"last_name": "Doe",
"email": "[email protected]",
},
status=200,
)

oidc_backend = OIDCAuthenticationBackend()

with pytest.raises(
SuspiciousOperation, match="Missing required claims in user info: sub, address"
):
oidc_backend.get_userinfo("fake_access_token", None, None)


@override_settings(
OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo",
USER_OIDC_REQUIRED_CLAIMS=["email", "Sub"],
)
@responses.activate
def test_authentication_get_userinfo_required_claims_case_sensitivity():
"""Ensure the system respects case sensitivity for required claims."""

responses.add(
responses.GET,
re.compile(r".*/userinfo"),
json={
"sub": "123",
"last_name": "Doe",
"email": "[email protected]",
},
status=200,
)

oidc_backend = OIDCAuthenticationBackend()

with pytest.raises(
SuspiciousOperation, match="Missing required claims in user info: Sub"
):
oidc_backend.get_userinfo("fake_access_token", None, None)


@override_settings(
OIDC_OP_USER_ENDPOINT="http://oidc.endpoint.test/userinfo",
USER_OIDC_REQUIRED_CLAIMS=["email", "sub"],
)
@responses.activate
def test_authentication_get_userinfo_required_claims_success():
"""Ensure user is authenticated when required claims are present."""

responses.add(
responses.GET,
re.compile(r".*/userinfo"),
json={
"sub": "123",
"last_name": "Doe",
"email": "[email protected]",
},
status=200,
)

oidc_backend = OIDCAuthenticationBackend()
result = oidc_backend.get_userinfo("fake_access_token", None, None)

assert result["sub"] == "123"
assert result.get("first_name") is None
assert result["last_name"] == "Doe"
assert result["email"] == "[email protected]"
3 changes: 3 additions & 0 deletions src/backend/impress/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,9 @@ class Base(Configuration):
environ_prefix=None,
)

USER_OIDC_REQUIRED_CLAIMS = values.ListValue(
default=[], environ_name="USER_OIDC_REQUIRED_CLAIMS", environ_prefix=None
)
USER_OIDC_FIELDS_TO_FULLNAME = values.ListValue(
default=["first_name", "last_name"],
environ_name="USER_OIDC_FIELDS_TO_FULLNAME",
Expand Down
Loading