diff --git a/CHANGELOG.md b/CHANGELOG.md index d48c4506..8a198ebe 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 00000000..dfc0899c --- /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 db5d2806..e1ae606f 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 d58cb7b4..65caca11 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 009b5f79..35af7b94 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 32f4c2fa..9f78b23f 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 45961cc5..9f5dd768 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