Merge pull request #1013 from lsst-sqre/tickets/DM-44136

DM-44136: Simplify sources of user metadata

changelog.d/20240502_133255_rra_DM_44136.md

@@ -0,0 +1,5 @@
+### Backwards-incompatible changes
+
+- Drop support for getting user metadata from OpenID Connect token claims. LDAP, for both user metadata and group membership, is now required when using an OpenID Connect authentication, including CILogon.
+- Drop support for LDAP groups without GIDs. Either Firestore GID assignment must be enabled or LDAP must contain a GID for each group. Groups without GIDs in LDAP will be ignored if Firestore is not enabled.
+- Retrieval of the UID and primary GID from LDAP is now enabled by default unless Firestore is enabled.

docs/_rst_epilog.rst

@@ -6,6 +6,7 @@
 .. _Phalanx: https://phalanx.lsst.io/
 .. _pre-commit: https://pre-commit.com
 .. _pytest: https://docs.pytest.org/en/latest/
+.. _RFC 2307bis: https://datatracker.ietf.org/doc/html/draft-howard-rfc2307bis-02
 .. _Ruff: https://beta.ruff.rs/docs/
 .. _Safir: https://safir.lsst.io/
 .. _scriv: https://scriv.readthedocs.io/en/latest/

docs/dev/index.rst

@@ -14,6 +14,7 @@ The Gafaelfawr code structure follows the guidelines in :sqr:`072`.
    requirements
    configuration
    providers
+   userinfo
    scopes
    logging
 

docs/dev/internals.rst

@@ -77,6 +77,10 @@ Python internal API
    :include-all-objects:
    :inherited-members:
 
+.. automodapi:: gafaelfawr.models.userinfo
+   :include-all-objects:
+   :inherited-members:
+
 .. automodapi:: gafaelfawr.operator
    :include-all-objects:
 

docs/dev/userinfo.rst

@@ -0,0 +1,38 @@
+#############
+User metadata
+#############
+
+User metadata is supplemental information about a user, specifically their name, email address, UID, primary GID, and group membership.
+Gafaelfawr can obtain this information from several sources, depending on its configuration.
+
+Conceptually, user metadata is associated with the username and is the same across all of their tokens.
+Operationally, metadata from GitHub and from admin token requests is stored in the token, so Gafaelfawr starts from a token and its associated data when determining user metadata.
+
+UID
+===
+
+The UID is determined as follows.
+
+#. If the token was created by an admin request that specified a UID, or is the child token of a token with an associated UID, return that UID.
+#. If Firestore support is configured, allocate a UID from Firestore if necessary and return the UID in Firestore.
+#. If the token was created from a GitHub authentication, return the GitHub user ID as the UID.
+#. If LDAP is configured and the ``uidAttr`` setting is not null, search LDAP for the user's UID using the configured LDAP attribute.
+
+If the algorithm falls off the end of this list, Gafaelfawr is misconfigured or the data is missing from LDAP.
+Return an error.
+
+Primary GID
+===========
+
+The primary GID is determined as follows.
+Note that the UID must be determined first.
+
+#. If the token was created by an admin request that specified a GID, or is the child token of a token with an associated GID, return that GID.
+#. If the authentication mechanism is GitHub, return the UID as the primary GID.
+#. If LDAP is configured and the ``gidAttr`` setting is not null, search LDAP for the user's primary GID using the configured LDAP attribute.
+#. If ``addUserGroup`` is enabled, search the LDAP group tree for a group whose name matches the username, and use the GID of that group as the primary GID.
+#. Return `None` as the primary GID.
+
+Gafaelfawr does not require users have a primary GID.
+Bot users often do not, for example.
+Some services, such as `Nublado <https://nublado.lsst.io/>`__, may reject users who do not have a primary GID, however.

examples/gafaelfawr-oidc.yaml

@@ -55,6 +55,16 @@ oidc:
   issuer: "https://cilogon.org"
   audience: "cilogon:/client_id/..."
 
+# LDAP must be configured if OpenID Connect is configured.  This example is
+# for the CILogon-managed COmanage.
+ldap:
+  url: "ldaps://ldap.cilogon.org"
+  groupBaseDn: "ou=groups,o=Example,o=CO,dc=example,dc=org"
+  groupObjectClass: "eduMember"
+  groupMemberAttr: "hasMember"
+  userBaseDn: "ou=people,o=Example,o=CO,dc=example,dc=org"
+  userSearchAttr: "voPersonApplicationUID"
+
 # Replace this with a list of users who should have admin rights when
 # bootstrapping a fresh database.
 initialAdmins:

src/gafaelfawr/config.py

@@ -31,7 +31,7 @@
 )
 from pydantic_core import Url
 from safir.logging import LogLevel, configure_logging
-from safir.pydantic import CamelCaseModel, validate_exactly_one_of
+from safir.pydantic import CamelCaseModel
 
 from .constants import SCOPE_REGEX, USERNAME_REGEX
 from .keypair import RSAKeyPair
@@ -135,15 +135,6 @@ class OIDCSettings(CamelCaseModel):
     username_claim: str = "uid"
     """Name of claim to use as the username."""
 
-    uid_claim: str = "uidNumber"
-    """Name of claim to use as the UID."""
-
-    gid_claim: str | None = None
-    """Name of claim to use as the primary GID."""
-
-    groups_claim: str = "isMemberOf"
-    """Name of claim to use for the group membership."""
-
 
 class LDAPSettings(CamelCaseModel):
     """pydantic model of LDAP configuration."""
@@ -204,12 +195,12 @@ class LDAPSettings(CamelCaseModel):
     If set to `True`, ``user_base_dn`` must be set.
     """
 
-    user_base_dn: str | None = None
+    user_base_dn: str
     """Base DN to use to search for user information.
 
-    If set, the base DN used to search for the user record, from which other
-    information such as full name, email, and (if configured) numeric UID will
-    be retrieved.
+    The base DN used to search for the user record, from which other
+    information such as full name, email, numeric UID, and (if configured)
+    numeric GID will be retrieved.
     """
 
     user_search_attr: str = "uid"
@@ -226,8 +217,8 @@ class LDAPSettings(CamelCaseModel):
     """LDAP full name attribute.
 
     The attribute from which the user's full name will be taken, or `None` to
-    not look up full names.  This should normally be ``displayName``, but
-    sometimes it may be desirable to use a different name attribute.  This
+    not look up full names. This should normally be ``displayName``, but
+    sometimes it may be desirable to use a different name attribute. This
     should hold the whole name that should be used by the Science Platform,
     not just a surname or family name (which are not universally valid
     concepts anyway).
@@ -237,28 +228,24 @@ class LDAPSettings(CamelCaseModel):
     """LDAP email attribute.
 
     The attribute from which the user's email address should be taken, or
-    `None` to not look up email addresses.  This should normally be
-    ``mail``.
+    `None` to not look up email addresses. This should normally be ``mail``.
     """
 
-    uid_attr: str | None = None
+    uid_attr: str | None = "uidNumber"
     """LDAP UID attribute.
 
-    If set, the user's UID will be taken from this sttribute.  If UID lookups
-    are desired, this should usually be ``uidNumber``, as specified in
-    :rfc:`2307` and `RFC 2307bis
-    <https://datatracker.ietf.org/doc/html/draft-howard-rfc2307bis-02>`__.
+    If set, the user's UID will be taken from this sttribute. This should
+    usually be ``uidNumber``, as specified in :rfc:`2307` and `RFC 2307bis`_.
+    If not set, Firestore must be configured.
     """
 
-    gid_attr: str | None = None
+    gid_attr: str | None = "gidNumber"
     """LDAP GID attirbute.
 
-    If set, the user's primary GID will be taken from this sttribute.  If GID
-    lookups are desired, this should usually be ``gidNumber``, as specified in
-    :rfc:`2307` and `RFC 2307bis
-    <https://datatracker.ietf.org/doc/html/draft-howard-rfc2307bis-02>`__.  If
-    not set, the primary GID will match the UID if ``add_user_group`` is true,
-    and otherwise will not be set.
+    If set, the user's primary GID will be taken from this sttribute. This
+    should usually be ``gidNumber``, as specified in :rfc:`2307` and `RFC
+    2307bis`_. If not set, the primary GID will match the UID if
+    ``add_user_group`` is true, and otherwise will not be set.
     """
 
     add_user_group: bool = False
@@ -269,13 +256,6 @@ class LDAPSettings(CamelCaseModel):
     requiring it to appear in LDAP.
     """
 
-    @model_validator(mode="after")
-    def _validate_group_search_by_dn(self) -> Self:
-        if self.group_search_by_dn and not self.user_base_dn:
-            msg = "user_base_dn must be set if group_search_by_dn is true"
-            raise ValueError(msg)
-        return self
-
     @model_validator(mode="after")
     def _validate_password_file(self) -> Self:
         """Ensure fields are non-empty if url is non-empty."""
@@ -510,9 +490,24 @@ def _valid_known_scopes(cls, v: dict[str, str]) -> dict[str, str]:
                 raise ValueError(f"required scope {required} missing")
         return v
 
-    _validate_provider = model_validator(mode="after")(
-        validate_exactly_one_of("github", "oidc")
-    )
+    @model_validator(mode="after")
+    def _validate_userinfo(self) -> Self:
+        """Ensure user information sources are configured properly."""
+        if not self.github and not self.oidc:
+            msg = "One of GitHub or OpenID Connect must be configured"
+            raise ValueError(msg)
+        if self.github and self.oidc:
+            raise ValueError("GitHub and OpenID Connect cannot both be used")
+        if self.github and self.ldap:
+            raise ValueError("LDAP cannot be used with GitHub authentication")
+        if self.oidc and not self.ldap:
+            msg = "LDAP must be configured if OpenID Connect is used"
+            raise ValueError(msg)
+        if self.ldap:
+            if not self.ldap.uid_attr and not self.firestore:
+                msg = "ldap.uidAttr must be set unless Firestore is used"
+                raise ValueError(msg)
+        return self
 
 
 @dataclass(frozen=True, slots=True)
@@ -580,15 +575,6 @@ class OIDCConfig:
     username_claim: str
     """Token claim from which to take the username."""
 
-    uid_claim: str
-    """Token claim from which to take the UID."""
-
-    gid_claim: str | None
-    """Token claim from which to take the primary GID."""
-
-    groups_claim: str
-    """Token claim from which to take the group membership."""
-
 
 @dataclass(frozen=True, slots=True)
 class LDAPConfig:
@@ -639,7 +625,7 @@ class LDAPConfig:
     containing that DN.
     """
 
-    user_base_dn: str | None
+    user_base_dn: str
     """Base DN to use to search for user information.
 
     If set, the base DN used to search for the user record, from which other
@@ -694,14 +680,6 @@ class LDAPConfig:
     and otherwise will not be set.
     """
 
-    add_user_group: bool
-    """Whether to synthesize a user private group with GID matching UID.
-
-    If set to `True`, synthesize a group for the user whose name and GID
-    matches the username and UID, adding it to the group list without
-    requiring it to appear in LDAP.
-    """
-
 
 @dataclass(frozen=True, slots=True)
 class FirestoreConfig:
@@ -858,6 +836,14 @@ class Config:
     cadc_base_uuid: UUID | None
     """Namespace UUID used to generate UUIDs for CADC-compatible auth."""
 
+    add_user_group: bool
+    """Whether to synthesize a user private group with GID matching UID.
+
+    If set to `True`, synthesize a group for the user whose name and GID
+    matches the username and UID, adding it to the group list without
+    requiring it to appear in LDAP.
+    """
+
     github: GitHubConfig | None
     """Configuration for GitHub authentication."""
 
@@ -932,14 +918,12 @@ def from_file(cls, path: Path) -> Self:  # noqa: PLR0912,PLR0915,C901
                 issuer=settings.oidc.issuer,
                 audience=settings.oidc.audience,
                 username_claim=settings.oidc.username_claim,
-                uid_claim=settings.oidc.uid_claim,
-                gid_claim=settings.oidc.gid_claim,
-                groups_claim=settings.oidc.groups_claim,
             )
 
         # Build LDAP configuration if needed.
+        add_user_group = settings.github is not None
         ldap_config = None
-        if settings.ldap and settings.ldap.url:
+        if settings.ldap:
             ldap_password = None
             if settings.ldap.password_file:
                 path = settings.ldap.password_file
@@ -959,8 +943,8 @@ def from_file(cls, path: Path) -> Self:  # noqa: PLR0912,PLR0915,C901
                 email_attr=settings.ldap.email_attr,
                 uid_attr=settings.ldap.uid_attr,
                 gid_attr=settings.ldap.gid_attr,
-                add_user_group=settings.ldap.add_user_group,
             )
+            add_user_group = settings.ldap.add_user_group
 
         # Build Firestore configuration if needed.
         firestore_config = None
@@ -1068,6 +1052,7 @@ def from_file(cls, path: Path) -> Self:  # noqa: PLR0912,PLR0915,C901
             error_footer=settings.error_footer,
             slack_webhook=slack_webhook,
             cadc_base_uuid=settings.cadc_base_uuid,
+            add_user_group=add_user_group,
             github=github_config,
             oidc=oidc_config,
             ldap=ldap_config,