Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-47148: Separate bot users in auth events #1142

Merged
merged 1 commit into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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/).

Expand Down
3 changes: 3 additions & 0 deletions changelog.d/20241028_150742_rra_DM_47148.md
Original file line number Diff line number Diff line change
@@ -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.
9 changes: 7 additions & 2 deletions docs/user-guide/metrics.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
30 changes: 25 additions & 5 deletions src/gafaelfawr/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
__all__ = [
"ActiveUserSessionsEvent",
"ActiveUserTokensEvent",
"AuthEvent",
"AuthBotEvent",
"AuthUserEvent",
"FrontendEvents",
"LoginAttemptEvent",
"LoginEnrollmentEvent",
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
)
Expand Down
15 changes: 10 additions & 5 deletions src/gafaelfawr/handlers/ingress.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)

Expand Down Expand Up @@ -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"}


Expand Down
16 changes: 0 additions & 16 deletions src/gafaelfawr/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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.

Expand Down
11 changes: 0 additions & 11 deletions tests/util_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
add_padding,
base64_to_number,
is_bot_user,
is_mobu_bot_user,
normalize_timedelta,
number_to_base64,
)
Expand Down Expand Up @@ -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
Expand Down