Merge pull request #1142 from lsst-sqre/tickets/DM-47148

DM-47148: Separate bot users in auth events

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Change log
 
-Gafaelfawr is versioned with [semver](https://semver.org/). Dependencies are updated to the latest available version during each release. Those changes are not noted here explicitly.
+Gafaelfawr is versioned with [semver](https://semver.org/). Changes to metrics and logging are not considered backwards-incompatible changes.
+
+Dependencies are updated to the latest available version during each release. Those changes are not noted here explicitly.
 
 Find changes for the upcoming release in the project's [changelog.d directory](https://github.com/lsst-sqre/gafaelfawr/tree/main/changelog.d/).
 

changelog.d/20241028_150742_rra_DM_47148.md

@@ -0,0 +1,3 @@
+### New features
+
+- Separate `auth` metrics into `auth_bot` and `auth_user` metrics, where the former are authentications to services from bot users and the latter are authentications from non-bot users. Stop excluding mobu bot users now that they can be included in the `auth_bot` metric instead.

docs/user-guide/metrics.rst

@@ -15,8 +15,13 @@ Frontend metrics
 
 The following events are logged by the Gafaelfawr frontend:
 
-auth
-    A user was successfully authenticated to a service.
+auth_bot
+    A bot user was successfully authenticated to a service.
+    The username is present as the ``username`` tag.
+    The service name is present as the ``service`` tag, if known.
+
+auth_user
+    A non-bot user was successfully authenticated to a service.
     The username is present as the ``username`` tag.
     The service name is present as the ``service`` tag, if known.
 

src/gafaelfawr/events.py

@@ -9,7 +9,8 @@
 __all__ = [
     "ActiveUserSessionsEvent",
     "ActiveUserTokensEvent",
-    "AuthEvent",
+    "AuthBotEvent",
+    "AuthUserEvent",
     "FrontendEvents",
     "LoginAttemptEvent",
     "LoginEnrollmentEvent",
@@ -78,10 +79,24 @@ async def initialize(self, manager: EventManager) -> None:
         )
 
 
-class AuthEvent(EventPayload):
-    """An authentication to a service.
+class AuthBotEvent(EventPayload):
+    """An authentication to a service by a bot user."""
 
-    Authentications from mobu bot users are not logged via this event.
+    username: str = Field(
+        ..., title="Username", description="Username of bot user"
+    )
+
+    service: str | None = Field(
+        None,
+        title="Service",
+        description="Service to which the user was authenticated",
+    )
+
+
+class AuthUserEvent(EventPayload):
+    """An authentication to a service by a user.
+
+    Bot users are not included in this metric.
     """
 
     username: str = Field(
@@ -161,7 +176,12 @@ class FrontendEvents(EventMaker):
     """
 
     async def initialize(self, manager: EventManager) -> None:
-        self.auth = await manager.create_publisher("auth", AuthEvent)
+        self.auth_bot = await manager.create_publisher(
+            "auth_bot", AuthBotEvent
+        )
+        self.auth_user = await manager.create_publisher(
+            "auth_user", AuthUserEvent
+        )
         self.login_attempt = await manager.create_publisher(
             "login_attempt", LoginAttemptEvent
         )

src/gafaelfawr/handlers/ingress.py

@@ -26,7 +26,7 @@
 from ..constants import MINIMUM_LIFETIME
 from ..dependencies.auth import AuthenticateRead
 from ..dependencies.context import RequestContext, context_dependency
-from ..events import AuthEvent
+from ..events import AuthBotEvent, AuthUserEvent
 from ..exceptions import (
     ExternalUserInfoError,
     InsufficientScopeError,
@@ -37,7 +37,7 @@
 )
 from ..models.auth import AuthType, Satisfy
 from ..models.token import TokenData
-from ..util import is_mobu_bot_user
+from ..util import is_bot_user
 
 router = APIRouter(route_class=SlackRouteErrorHandler)
 
@@ -359,11 +359,16 @@ async def get_auth(
     headers = await build_success_headers(context, auth_config, token_data)
     for key, value in headers:
         response.headers.append(key, value)
-    if not is_mobu_bot_user(token_data.username):
-        event = AuthEvent(
+    if is_bot_user(token_data.username):
+        bot_event = AuthBotEvent(
             username=token_data.username, service=auth_config.service
         )
-        await context.events.auth.publish(event)
+        await context.events.auth_bot.publish(bot_event)
+    else:
+        user_event = AuthUserEvent(
+            username=token_data.username, service=auth_config.service
+        )
+        await context.events.auth_user.publish(user_event)
     return {"status": "ok"}
 
 

src/gafaelfawr/util.py

@@ -25,7 +25,6 @@
     "base64_to_number",
     "group_name_for_github_team",
     "is_bot_user",
-    "is_mobu_bot_user",
     "normalize_ip_address",
     "normalize_scopes",
     "normalize_timedelta",
@@ -88,21 +87,6 @@ def is_bot_user(username: str) -> bool:
     return re.search(BOT_USERNAME_REGEX, username) is not None
 
 
-def is_mobu_bot_user(username: str) -> bool:
-    """Return whether the given username is a mobu bot user.
-
-    mobu is the integration testing bot. Its actions should not be included in
-    usage metrics, since they could swamp the measurements for regular users
-    or distort the metrics.
-
-    Parameters
-    ----------
-    username
-        Username to check.
-    """
-    return is_bot_user(username) and username.startswith("bot-mobu")
-
-
 def group_name_for_github_team(organization: str, team: str) -> str:
     """Convert a GitHub organization and team to a group name.
 

tests/util_test.py

@@ -12,7 +12,6 @@
     add_padding,
     base64_to_number,
     is_bot_user,
-    is_mobu_bot_user,
     normalize_timedelta,
     number_to_base64,
 )
@@ -53,16 +52,6 @@ def test_is_bot_user() -> None:
     assert not is_bot_user("bot-in!valid")
 
 
-def test_is_mobu_bot_user() -> None:
-    assert is_mobu_bot_user("bot-mobu-weekly")
-    assert is_mobu_bot_user("bot-mobu")
-    assert not is_mobu_bot_user("bot-user")
-    assert not is_mobu_bot_user("some-user")
-    assert not is_mobu_bot_user("bot")
-    assert not is_mobu_bot_user("botuser")
-    assert not is_mobu_bot_user("bot-in!valid")
-
-
 def test_normalize_timedelta() -> None:
     class TestModel(BaseModel):
         delta: timedelta | None