Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow multiple values for OIDC attribute requirements #12488

Closed

Conversation

chugunov
Copy link

@chugunov chugunov commented Apr 17, 2022

I need to filter SSO logins by multiple domains (Google OpenID uses hd claim in response)
I've tried to extend the ability of filtering SSO attribute values for that use case, as described at #12238 (comment).

Signed-off-by: Andrey Chugunov [email protected]

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@chugunov chugunov marked this pull request as ready for review April 17, 2022 22:58
@chugunov chugunov requested a review from a team as a code owner April 17, 2022 22:58
@richvdh richvdh changed the title Add values to SSO attribute requirement ALlow multiple values to SSO attribute requirement Apr 20, 2022
@richvdh richvdh changed the title ALlow multiple values to SSO attribute requirement Allow multiple values for OIDC attribute requirements Apr 20, 2022
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Thanks very much for contributing!

I think this is a good start, but also there is quite a lot that needs cleaning up - see the comments below.

Regarding the documentation, let us know if you need a hand with the English, but please could you have a go yourself first?

Comment on lines +211 to +212
# `attribute_requirements` as shown below. All of the listed in `value` attributes and
# any of the listed in `values` attributes must match for the login to be permitted.
Copy link
Member

Choose a reason for hiding this comment

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

This new sentence isn't very clear - please could you expand on it somewhat?

Copy link
Member

Choose a reason for hiding this comment

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

In general this paragraph is very unclear to start with, so anything you can do to improve it would be much appreciated.

Comment on lines +216 to +217
# If you specify both `value` and `values` entries, then only `value` entry will be checked
# and `values` will be ignored.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# If you specify both `value` and `values` entries, then only `value` entry will be checked
# and `values` will be ignored.
# If you specify both `value` and `values` entries, then only the `value` entry will be checked
# and `values` will be ignored.

Copy link
Member

Choose a reason for hiding this comment

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

since you're using oneOf in the json schema, my understanding is that if you specify both values, the configuration will be rejected. Are you sure this sentence is correct?

@@ -36,13 +36,27 @@ class SsoAttributeRequirement:
"""Object describing a single requirement for SSO attributes."""

attribute: str
# If a value is not given, than the attribute must simply exist.
value: Optional[str]
# If a value or values are not given, than the attribute must simply exist.
Copy link
Member

Choose a reason for hiding this comment

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

according to the schema, one or both of value or values is required - so I don't understand this comment?

Comment on lines +40 to +41
value: Optional[str] = None
values: Optional[List[str]] = []
Copy link
Member

Choose a reason for hiding this comment

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

rather than having both of these, could we just have a values: List[str], and convert value into a single-item list?

#
# attribute_requirements:
# - attribute: given_name
# values: ["Andries", "Michael"]
Copy link
Member

Choose a reason for hiding this comment

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

I think a different name might make the behaviour here clearer - for example allowable_values ?

}
)
def test_attribute_requirements_exists(self) -> None:
"""The required attributes must exists."""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""The required attributes must exists."""
"""The required attributes must exist."""

elif req.values:
req_values.extend(req.values)
else:
# If the requirement is None or empty, the attribute existing is enough.
Copy link
Member

Choose a reason for hiding this comment

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

having special behaviour for an empty list feels extremely unintuitive. At the very least it should be documented, but I am not in favour.

}
)
def test_attribute_requirements_contains_any_values(self) -> None:
"""Test that auth succeeds if userinfo attribute CONTAINS ANY required values"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Test that auth succeeds if userinfo attribute CONTAINS ANY required values"""
"""Test that auth succeeds if userinfo attribute contains any of the required values"""

)
def test_attribute_requirements_mismatch_any_values(self) -> None:
"""
Test that auth fails if attributes exist but don't match ANY VALUE,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Test that auth fails if attributes exist but don't match ANY VALUE,
Test that auth fails if attributes exist but don't match any of the required values,

# match for the login to be permitted. Additional attributes can be added to
# userinfo by expanding the `scopes` section of the OIDC config to retrieve
# additional information from the OIDC provider.
# `attribute_requirements` as shown below. All of the listed in `value` attributes and
Copy link
Member

Choose a reason for hiding this comment

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

by the way, don't forget to update the manual at https://github.com/matrix-org/synapse/blob/develop/docs/usage/configuration/config_documentation.md - currently this has to be done manually. Maybe best to wait until we can agree wording though.

@reivilibre reivilibre added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Aug 5, 2022
@reivilibre
Copy link
Contributor

@chugunov As this has sat around for a bit now: are you still interested in working on this?

@clokep
Copy link
Member

clokep commented Aug 24, 2023

Closing due to lack of response.

@clokep clokep closed this Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants