From 28ee3f8c4b7359f42c1c43db7a6392eb28dffaf1 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Mon, 28 Oct 2024 14:47:47 -0700 Subject: [PATCH] Separate bot users in auth events Log separate metrics for bot authentications and user authentications. Remove the special case for mobu bot users since they can not be counted as regular bot users. --- CHANGELOG.md | 4 ++- changelog.d/20241028_150742_rra_DM_47148.md | 3 +++ docs/user-guide/metrics.rst | 9 +++++-- src/gafaelfawr/events.py | 30 +++++++++++++++++---- src/gafaelfawr/handlers/ingress.py | 15 +++++++---- src/gafaelfawr/util.py | 16 ----------- tests/util_test.py | 11 -------- 7 files changed, 48 insertions(+), 40 deletions(-) create mode 100644 changelog.d/20241028_150742_rra_DM_47148.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d48c45060..8a198ebe2 100644 --- a/CHANGELOG.md +++ b/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/). diff --git a/changelog.d/20241028_150742_rra_DM_47148.md b/changelog.d/20241028_150742_rra_DM_47148.md new file mode 100644 index 000000000..dfc0899c0 --- /dev/null +++ b/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. diff --git a/docs/user-guide/metrics.rst b/docs/user-guide/metrics.rst index db5d2806a..e1ae606f7 100644 --- a/docs/user-guide/metrics.rst +++ b/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. diff --git a/src/gafaelfawr/events.py b/src/gafaelfawr/events.py index d58cb7b41..65caca11f 100644 --- a/src/gafaelfawr/events.py +++ b/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 ) diff --git a/src/gafaelfawr/handlers/ingress.py b/src/gafaelfawr/handlers/ingress.py index 009b5f797..35af7b94e 100644 --- a/src/gafaelfawr/handlers/ingress.py +++ b/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"} diff --git a/src/gafaelfawr/util.py b/src/gafaelfawr/util.py index 32f4c2fa7..9f78b23ff 100644 --- a/src/gafaelfawr/util.py +++ b/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. diff --git a/tests/util_test.py b/tests/util_test.py index 45961cc5b..9f5dd768c 100644 --- a/tests/util_test.py +++ b/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