From 449c921bf201dc1b65a302c4e00c6d6ff062a60b Mon Sep 17 00:00:00 2001 From: Seena Fallah Date: Sat, 11 Jul 2026 22:47:15 +0000 Subject: [PATCH 1/5] feat(api): add API-provisionable service accounts Add machine/service accounts so a self-hosted workspace can be provisioned entirely over the API: a distinct, active actor with an API-mintable token and no invite / email-verification / password flow. What each account is (created in one transaction): an active, email-verified User flagged as a bot (is_bot=True, bot_type=SERVICE) with an unusable password and a synthetic unique username/email; a WorkspaceMember at the requested role; and a workspace-scoped bot APIToken (user_type=Bot, is_service=True). is_bot blocks interactive login (BOT_USER_LOGIN_FORBIDDEN); the account acts only via its token, and its writes attribute to it via created_by. - create_service_account management command (--workspace --name --role [--email --description]) that prints the token. - Optional admin-scoped POST /api/v1/workspaces/{slug}/service-accounts/ (WorkspaceOwnerPermission), mirroring the command over HTTP. - Shared plane/utils/service_account.py helper so command and endpoint cannot drift. - Add SERVICE to BotTypeEnum. bot_type is a choice-less CharField, so no migration is required. - Security: redact secret-bearing response bodies from api_activity_logs so the minted token is never persisted in plaintext (response-body analogue of the existing SENSITIVE_HEADERS redaction). - docs/service-accounts.md and contract + unit tests (token authenticates as a distinct actor, writes attribute to it, non-admin gets 403, token not logged). Signed-off-by: Seena Fallah --- apps/api/plane/api/serializers/__init__.py | 1 + .../plane/api/serializers/service_account.py | 41 +++ apps/api/plane/api/urls/__init__.py | 2 + apps/api/plane/api/urls/service_account.py | 15 ++ apps/api/plane/api/views/__init__.py | 2 + apps/api/plane/api/views/service_account.py | 78 ++++++ .../commands/create_service_account.py | 76 ++++++ apps/api/plane/db/models/user.py | 1 + apps/api/plane/middleware/logger.py | 28 +- .../contract/api/test_service_account.py | 246 ++++++++++++++++++ .../tests/unit/middleware/test_logger.py | 27 +- .../tests/unit/utils/test_service_account.py | 31 +++ apps/api/plane/utils/service_account.py | 143 ++++++++++ docs/service-accounts.md | 126 +++++++++ 14 files changed, 812 insertions(+), 5 deletions(-) create mode 100644 apps/api/plane/api/serializers/service_account.py create mode 100644 apps/api/plane/api/urls/service_account.py create mode 100644 apps/api/plane/api/views/service_account.py create mode 100644 apps/api/plane/db/management/commands/create_service_account.py create mode 100644 apps/api/plane/tests/contract/api/test_service_account.py create mode 100644 apps/api/plane/tests/unit/utils/test_service_account.py create mode 100644 apps/api/plane/utils/service_account.py create mode 100644 docs/service-accounts.md diff --git a/apps/api/plane/api/serializers/__init__.py b/apps/api/plane/api/serializers/__init__.py index d0278eb1415..2c6365ef3cb 100644 --- a/apps/api/plane/api/serializers/__init__.py +++ b/apps/api/plane/api/serializers/__init__.py @@ -67,4 +67,5 @@ WorkspaceMemberLiteAPISerializer, ProjectMemberLiteAPISerializer, ) +from .service_account import ServiceAccountCreateSerializer, ServiceAccountSerializer from .sticky import StickySerializer diff --git a/apps/api/plane/api/serializers/service_account.py b/apps/api/plane/api/serializers/service_account.py new file mode 100644 index 00000000000..edb4bc38e99 --- /dev/null +++ b/apps/api/plane/api/serializers/service_account.py @@ -0,0 +1,41 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Third party imports +from rest_framework import serializers + +# Module imports +from plane.utils.service_account import DEFAULT_SERVICE_ACCOUNT_ROLE, SERVICE_ACCOUNT_ROLES + + +class ServiceAccountCreateSerializer(serializers.Serializer): + """Request body for provisioning a workspace service account.""" + + name = serializers.CharField(max_length=255, help_text="Display name for the service account") + role = serializers.ChoiceField( + choices=sorted(SERVICE_ACCOUNT_ROLES), + default=DEFAULT_SERVICE_ACCOUNT_ROLE, + help_text="Workspace role: admin, member, or guest", + ) + description = serializers.CharField( + required=False, + allow_blank=True, + default="", + help_text="Optional description stored on the API token", + ) + + +class ServiceAccountSerializer(serializers.Serializer): + """Response for a newly provisioned service account. + + ``token`` is the plaintext API key and is returned only once, at creation. + """ + + id = serializers.UUIDField(read_only=True, help_text="Service account user id") + username = serializers.CharField(read_only=True) + email = serializers.CharField(read_only=True) + display_name = serializers.CharField(read_only=True) + role = serializers.IntegerField(read_only=True, help_text="Workspace role value (20 admin, 15 member, 5 guest)") + workspace = serializers.UUIDField(read_only=True) + token = serializers.CharField(read_only=True, help_text="Plaintext API token — shown once") diff --git a/apps/api/plane/api/urls/__init__.py b/apps/api/plane/api/urls/__init__.py index 4a202431bc7..ffeaa64ce3b 100644 --- a/apps/api/plane/api/urls/__init__.py +++ b/apps/api/plane/api/urls/__init__.py @@ -9,6 +9,7 @@ from .member import urlpatterns as member_patterns from .module import urlpatterns as module_patterns from .project import urlpatterns as project_patterns +from .service_account import urlpatterns as service_account_patterns from .state import urlpatterns as state_patterns from .user import urlpatterns as user_patterns from .work_item import urlpatterns as work_item_patterns @@ -23,6 +24,7 @@ *member_patterns, *module_patterns, *project_patterns, + *service_account_patterns, *state_patterns, *user_patterns, *work_item_patterns, diff --git a/apps/api/plane/api/urls/service_account.py b/apps/api/plane/api/urls/service_account.py new file mode 100644 index 00000000000..9bc59f33ce9 --- /dev/null +++ b/apps/api/plane/api/urls/service_account.py @@ -0,0 +1,15 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +from django.urls import path + +from plane.api.views import ServiceAccountAPIEndpoint + +urlpatterns = [ + path( + "workspaces//service-accounts/", + ServiceAccountAPIEndpoint.as_view(http_method_names=["post"]), + name="service-accounts", + ), +] diff --git a/apps/api/plane/api/views/__init__.py b/apps/api/plane/api/views/__init__.py index 5e4660a7b2b..71ced15847f 100644 --- a/apps/api/plane/api/views/__init__.py +++ b/apps/api/plane/api/views/__init__.py @@ -71,4 +71,6 @@ from .invite import WorkspaceInvitationsViewset +from .service_account import ServiceAccountAPIEndpoint + from .sticky import StickyViewSet diff --git a/apps/api/plane/api/views/service_account.py b/apps/api/plane/api/views/service_account.py new file mode 100644 index 00000000000..349afde96ec --- /dev/null +++ b/apps/api/plane/api/views/service_account.py @@ -0,0 +1,78 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Third party imports +from rest_framework import status +from rest_framework.response import Response +from drf_spectacular.utils import extend_schema, OpenApiResponse, OpenApiRequest + +# Module imports +from plane.api.views.base import BaseAPIView +from plane.api.serializers.service_account import ( + ServiceAccountCreateSerializer, + ServiceAccountSerializer, +) +from plane.db.models import Workspace +from plane.middleware.logger import redact_response_body +from plane.utils.permissions import WorkspaceOwnerPermission +from plane.utils.openapi.parameters import WORKSPACE_SLUG_PARAMETER +from plane.utils.service_account import create_service_account + + +class ServiceAccountAPIEndpoint(BaseAPIView): + """Admin-scoped endpoint for provisioning workspace service accounts. + + A workspace admin can mint a machine identity — a distinct, active actor + with its own API token — without any invite, email-verification, or + password round-trip. This is the HTTP equivalent of the + ``create_service_account`` management command. + """ + + permission_classes = [WorkspaceOwnerPermission] + + @extend_schema( + summary="Create a service account", + description=( + "Create a machine/service account in the workspace and mint an API token for it. " + "The account is active and email-verified, is added as a workspace member with the " + "given role, and can authenticate only through the returned token. The token is " + "returned once and cannot be retrieved again. Requires the caller to be a workspace admin." + ), + request=OpenApiRequest(request=ServiceAccountCreateSerializer), + parameters=[WORKSPACE_SLUG_PARAMETER], + responses={ + 201: OpenApiResponse( + description="Service account created", + response=ServiceAccountSerializer, + ) + }, + ) + def post(self, request, slug): + workspace = Workspace.objects.get(slug=slug) + + serializer = ServiceAccountCreateSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + + service_account = create_service_account( + workspace=workspace, + name=data["name"], + role=data["role"], + description=data.get("description", ""), + ) + + response = ServiceAccountSerializer( + { + "id": service_account.user.id, + "username": service_account.user.username, + "email": service_account.user.email, + "display_name": service_account.user.display_name, + "role": service_account.member.role, + "workspace": workspace.id, + "token": service_account.token, + } + ) + # The response carries the plaintext token, so keep it out of the + # api_activity_logs table (APITokenLogMiddleware logs response bodies). + return redact_response_body(Response(response.data, status=status.HTTP_201_CREATED)) diff --git a/apps/api/plane/db/management/commands/create_service_account.py b/apps/api/plane/db/management/commands/create_service_account.py new file mode 100644 index 00000000000..d934f0ea50b --- /dev/null +++ b/apps/api/plane/db/management/commands/create_service_account.py @@ -0,0 +1,76 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Django imports +from django.core.management import BaseCommand, CommandError +from django.db import IntegrityError + +# Module imports +from plane.db.models import User, Workspace +from plane.utils.service_account import ( + DEFAULT_SERVICE_ACCOUNT_ROLE, + SERVICE_ACCOUNT_ROLES, + create_service_account, +) + + +class Command(BaseCommand): + help = ( + "Create a service (machine) account in a workspace and mint an API token for it. " + "The account is active and email-verified, needs no invite/accept flow, and can " + "authenticate only through the printed token." + ) + + def add_arguments(self, parser): + parser.add_argument("--workspace", type=str, required=True, help="Workspace slug") + parser.add_argument("--name", type=str, required=True, help="Display name for the service account") + parser.add_argument( + "--role", + type=str, + choices=list(SERVICE_ACCOUNT_ROLES), + default=DEFAULT_SERVICE_ACCOUNT_ROLE, + help=f"Workspace role (default: {DEFAULT_SERVICE_ACCOUNT_ROLE})", + ) + parser.add_argument( + "--email", + type=str, + default=None, + help="Optional email; a unique synthetic one is generated when omitted", + ) + parser.add_argument("--description", type=str, default="", help="Optional token description") + + def handle(self, *args, **options): + workspace = Workspace.objects.filter(slug=options["workspace"]).first() + if workspace is None: + raise CommandError(f"Workspace with slug '{options['workspace']}' does not exist") + + email = options.get("email") + if email and User.objects.filter(email=email).exists(): + raise CommandError(f"A user with email '{email}' already exists") + + try: + service_account = create_service_account( + workspace=workspace, + name=options["name"], + role=options["role"], + email=options["email"], + description=options["description"], + ) + except IntegrityError as exc: + # A concurrent insert (email race that slipped past the check above) + # or an extremely unlikely synthetic username/email collision surfaces + # here — report it readably instead of a raw traceback. The helper's + # @transaction.atomic has already rolled back, so no partial account + # remains. + raise CommandError(f"Could not create the service account — the email is already in use: {exc}") + + user = service_account.user + self.stdout.write(self.style.SUCCESS("Service account created successfully")) + self.stdout.write(f" user_id : {user.id}") + self.stdout.write(f" username : {user.username}") + self.stdout.write(f" email : {user.email}") + self.stdout.write(f" role : {options['role']}") + self.stdout.write(f" workspace: {workspace.slug}") + self.stdout.write(self.style.WARNING("API token (shown once — store it securely):")) + self.stdout.write(f" {service_account.token}") diff --git a/apps/api/plane/db/models/user.py b/apps/api/plane/db/models/user.py index 7f1ab162dab..bb7ab72e5e9 100644 --- a/apps/api/plane/db/models/user.py +++ b/apps/api/plane/db/models/user.py @@ -51,6 +51,7 @@ def get_default_product_tour(): class BotTypeEnum(models.TextChoices): WORKSPACE_SEED = "WORKSPACE_SEED", "Workspace Seed" + SERVICE = "SERVICE", "Service Account" class User(AbstractBaseUser, PermissionsMixin): diff --git a/apps/api/plane/middleware/logger.py b/apps/api/plane/middleware/logger.py index 343409c4e19..99d923aa7bf 100644 --- a/apps/api/plane/middleware/logger.py +++ b/apps/api/plane/middleware/logger.py @@ -77,6 +77,25 @@ def __call__(self, request): return response +# A view can set this attribute on its response to keep the (secret-bearing) +# body out of the api_activity_logs table — the response-body analogue of +# APITokenLogMiddleware.SENSITIVE_HEADERS. Use it for endpoints that return a +# credential (e.g. a freshly minted API token) which must never be persisted in +# plaintext. +REDACT_RESPONSE_BODY_ATTR = "_plane_redact_log_body" + + +def redact_response_body(response): + """Flag a response so APITokenLogMiddleware does not persist its body. + + Returns the response so it can be used inline, e.g.:: + + return redact_response_body(Response(data, status=201)) + """ + setattr(response, REDACT_RESPONSE_BODY_ATTR, True) + return response + + class APITokenLogMiddleware: """ Middleware to log External API requests to PostgreSQL. @@ -136,6 +155,13 @@ def process_request(self, request, response, request_body): return try: + # A secret-bearing response (e.g. a newly minted API token) must not + # be persisted in plaintext — mirrors the header redaction above. + if getattr(response, REDACT_RESPONSE_BODY_ATTR, False): + response_body = "[REDACTED]" + else: + response_body = self._safe_decode_body(response.content) if response.content else None + log_data = { # Tokenize the (high-entropy) API key into a stable, non-reversible # identifier so logs can be correlated to a token without ever @@ -149,7 +175,7 @@ def process_request(self, request, response, request_body): "query_params": request.META.get("QUERY_STRING", ""), "headers": self._redacted_headers(request), "body": self._safe_decode_body(request_body) if request_body else None, - "response_body": self._safe_decode_body(response.content) if response.content else None, + "response_body": response_body, "response_code": response.status_code, "ip_address": get_client_ip(request=request), "user_agent": request.META.get("HTTP_USER_AGENT", None), diff --git a/apps/api/plane/tests/contract/api/test_service_account.py b/apps/api/plane/tests/contract/api/test_service_account.py new file mode 100644 index 00000000000..5187ba358cd --- /dev/null +++ b/apps/api/plane/tests/contract/api/test_service_account.py @@ -0,0 +1,246 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +""" +Contract tests for workspace service (machine) accounts. + +Proves that a service account created without any invite/email/password flow is +a valid, distinct actor: it authenticates with its own API token and its writes +are attributed to it via ``created_by``. + +Covered surfaces: +- the ``create_service_account`` management command, and +- the admin-scoped ``POST /api/v1/workspaces/{slug}/service-accounts/`` endpoint. +""" + +from unittest.mock import patch + +import pytest +from django.core.management import call_command +from rest_framework import status +from rest_framework.test import APIClient + +from plane.db.models import APIToken, BotTypeEnum, Project, User, Workspace, WorkspaceMember +from plane.db.models.api import generate_token + +USERS_ME_URL = "/api/v1/users/me/" + + +def _client_for_token(token: str) -> APIClient: + client = APIClient() + client.credentials(HTTP_X_API_KEY=token) + return client + + +def _service_account_for(workspace: Workspace) -> WorkspaceMember: + return WorkspaceMember.objects.select_related("member").get( + workspace=workspace, + member__is_bot=True, + member__bot_type=BotTypeEnum.SERVICE, + ) + + +@pytest.mark.contract +class TestServiceAccountCommand: + """The management command provisions a valid, distinct, token-authing actor.""" + + @pytest.mark.django_db + def test_command_creates_active_verified_bot_member_with_token(self, workspace): + call_command( + "create_service_account", + workspace=workspace.slug, + name="CI Provisioner", + role="admin", + ) + + member = _service_account_for(workspace) + user = member.member + + # A valid actor: active + email verified, with no password round-trip. + assert user.is_active is True + assert user.is_email_verified is True + assert user.is_email_valid is True + assert user.is_bot is True + assert user.bot_type == BotTypeEnum.SERVICE + assert user.is_password_autoset is True + assert user.display_name == "CI Provisioner" + + # Added as a workspace member with the requested role. + assert member.role == 20 + assert member.is_active is True + + # A workspace-scoped bot API token was minted. + token = APIToken.objects.get(user=user) + assert token.user_type == 1 # Bot + assert token.is_service is True + assert token.workspace_id == workspace.id + assert token.token.startswith("plane_api_") + + @pytest.mark.django_db + def test_command_token_authenticates_as_distinct_actor(self, workspace): + call_command("create_service_account", workspace=workspace.slug, name="Robby", role="admin") + + user = _service_account_for(workspace).member + token = APIToken.objects.get(user=user) + + response = _client_for_token(token.token).get(USERS_ME_URL) + + assert response.status_code == status.HTTP_200_OK + # The token resolves to the service account itself — a distinct identity. + assert str(response.data["id"]) == str(user.id) + assert response.data["email"] == user.email + assert response.data["display_name"] == "Robby" + + @pytest.mark.django_db + def test_command_service_account_writes_are_attributed_to_it(self, workspace): + call_command("create_service_account", workspace=workspace.slug, name="Author Bot", role="admin") + + user = _service_account_for(workspace).member + token = APIToken.objects.get(user=user) + client = _client_for_token(token.token) + + response = client.post( + f"/api/v1/workspaces/{workspace.slug}/projects/", + {"name": "Provisioned Project", "identifier": "PROV"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + project = Project.objects.get(id=response.data["id"]) + # The write is attributed to the service account, not to any human. + assert project.created_by_id == user.id + + @pytest.mark.django_db + def test_command_supports_member_role(self, workspace): + call_command("create_service_account", workspace=workspace.slug, name="Member Bot", role="member") + + member = _service_account_for(workspace) + assert member.role == 15 + + @pytest.mark.django_db + def test_command_unknown_workspace_errors(self): + from django.core.management.base import CommandError + + with pytest.raises(CommandError): + call_command("create_service_account", workspace="does-not-exist", name="X", role="admin") + + @pytest.mark.django_db + def test_command_duplicate_email_errors_cleanly(self, workspace): + User.objects.create(username="taken", email="taken@plane.so") + + from django.core.management.base import CommandError + + with pytest.raises(CommandError): + call_command( + "create_service_account", + workspace=workspace.slug, + name="Dup", + role="admin", + email="taken@plane.so", + ) + + @pytest.mark.django_db + def test_command_integrity_error_becomes_command_error(self, workspace): + # A uniqueness collision that slips past the pre-check (e.g. an email + # race) surfaces as IntegrityError from the helper; the command must + # translate it into a readable CommandError, not a raw traceback. + from django.core.management.base import CommandError + from django.db import IntegrityError + + with patch( + "plane.db.management.commands.create_service_account.create_service_account", + side_effect=IntegrityError("duplicate key value violates unique constraint"), + ): + with pytest.raises(CommandError): + call_command("create_service_account", workspace=workspace.slug, name="Race", role="admin") + + +@pytest.mark.contract +class TestServiceAccountEndpoint: + """The admin-scoped HTTP endpoint mirrors the command.""" + + def _url(self, slug: str) -> str: + return f"/api/v1/workspaces/{slug}/service-accounts/" + + @pytest.mark.django_db + def test_admin_creates_service_account_and_token_works(self, api_key_client, workspace, create_user): + response = api_key_client.post( + self._url(workspace.slug), + {"name": "HTTP Bot", "role": "admin"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["role"] == 20 + assert response.data["display_name"] == "HTTP Bot" + token = response.data["token"] + assert token.startswith("plane_api_") + + # Provisioning is attributed to the acting admin (via crum current-user). + member = WorkspaceMember.objects.get(member_id=response.data["id"], workspace=workspace) + assert member.created_by_id == create_user.id + assert APIToken.objects.get(user_id=response.data["id"]).created_by_id == create_user.id + + # The returned token authenticates as the newly created distinct actor. + me = _client_for_token(token).get(USERS_ME_URL) + assert me.status_code == status.HTTP_200_OK + assert str(me.data["id"]) == str(response.data["id"]) + + # And its writes attribute to it. + created = _client_for_token(token).post( + f"/api/v1/workspaces/{workspace.slug}/projects/", + {"name": "HTTP Provisioned", "identifier": "HTTPP"}, + format="json", + ) + assert created.status_code == status.HTTP_201_CREATED, created.data + project = Project.objects.get(id=created.data["id"]) + assert str(project.created_by_id) == str(response.data["id"]) + + @pytest.mark.django_db + def test_minted_token_is_not_persisted_in_request_log(self, api_key_client, workspace): + # The admin calls this endpoint WITH their X-Api-Key, so APITokenLogMiddleware + # runs and would otherwise persist the response body (which carries the + # freshly minted token) into api_activity_logs in plaintext. The response is + # flagged for body redaction; assert the logged body never contains the token. + with patch("plane.middleware.logger.process_logs") as process_logs: + response = api_key_client.post( + self._url(workspace.slug), + {"name": "Secret Bot", "role": "admin"}, + format="json", + ) + assert response.status_code == status.HTTP_201_CREATED, response.data + token = response.data["token"] + assert process_logs.delay.called + log_data = process_logs.delay.call_args.kwargs["log_data"] + + assert log_data["response_body"] == "[REDACTED]" + assert token not in (log_data["response_body"] or "") + + @pytest.mark.django_db + def test_non_admin_member_is_forbidden(self, api_client, workspace): + # A guest member of the workspace is not a workspace admin. + guest = User.objects.create(username="guest_user", email="guest@plane.so") + guest.set_password("guest-pass") + guest.save() + WorkspaceMember.objects.create(workspace=workspace, member=guest, role=5, is_active=True) + guest_token = APIToken.objects.create(user=guest, label="guest-token", token=generate_token()) + + response = _client_for_token(guest_token.token).post( + self._url(workspace.slug), + {"name": "Should Fail", "role": "admin"}, + format="json", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + # No service account leaked into the workspace. + assert not WorkspaceMember.objects.filter(workspace=workspace, member__bot_type=BotTypeEnum.SERVICE).exists() + + @pytest.mark.django_db + def test_unauthenticated_is_rejected(self, api_client, workspace): + response = api_client.post( + self._url(workspace.slug), + {"name": "Anon", "role": "admin"}, + format="json", + ) + assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) diff --git a/apps/api/plane/tests/unit/middleware/test_logger.py b/apps/api/plane/tests/unit/middleware/test_logger.py index 5c13f53f6f7..d9b678f4041 100644 --- a/apps/api/plane/tests/unit/middleware/test_logger.py +++ b/apps/api/plane/tests/unit/middleware/test_logger.py @@ -20,7 +20,7 @@ from django.http import HttpResponse from django.test import RequestFactory -from plane.middleware.logger import APITokenLogMiddleware +from plane.middleware.logger import APITokenLogMiddleware, redact_response_body @pytest.fixture @@ -56,9 +56,7 @@ def _captured_log_data(self, middleware, request_factory): def test_token_identifier_is_hashed_not_plaintext(self, middleware, request_factory): log_data = self._captured_log_data(middleware, request_factory) - expected_hash = hmac.new( - settings.SECRET_KEY.encode(), self.API_KEY.encode(), hashlib.sha256 - ).hexdigest() + expected_hash = hmac.new(settings.SECRET_KEY.encode(), self.API_KEY.encode(), hashlib.sha256).hexdigest() assert log_data["token_identifier"] == expected_hash assert self.API_KEY not in log_data["token_identifier"] @@ -77,3 +75,24 @@ def test_no_log_without_api_key(self, middleware, request_factory): with patch("plane.middleware.logger.process_logs") as process_logs: middleware.process_request(request, HttpResponse(b"{}"), request_body=b"") assert not process_logs.delay.called + + def test_response_body_is_logged_by_default(self, middleware, request_factory): + request = request_factory.get("/api/v1/workspaces/", HTTP_X_API_KEY=self.API_KEY) + request.user = AnonymousUser() + response = HttpResponse(b'{"ok": true}') + with patch("plane.middleware.logger.process_logs") as process_logs: + middleware.process_request(request, response, request_body=b"") + log_data = process_logs.delay.call_args.kwargs["log_data"] + assert log_data["response_body"] == '{"ok": true}' + + def test_flagged_response_body_is_redacted(self, middleware, request_factory): + # A view that returns a secret (e.g. a minted API token) flags its response + # via redact_response_body so the body is never persisted in plaintext. + request = request_factory.post("/api/v1/workspaces/x/service-accounts/", HTTP_X_API_KEY=self.API_KEY) + request.user = AnonymousUser() + response = redact_response_body(HttpResponse(b'{"token": "plane_api_supersecret"}')) + with patch("plane.middleware.logger.process_logs") as process_logs: + middleware.process_request(request, response, request_body=b"") + log_data = process_logs.delay.call_args.kwargs["log_data"] + assert log_data["response_body"] == "[REDACTED]" + assert "plane_api_supersecret" not in log_data["response_body"] diff --git a/apps/api/plane/tests/unit/utils/test_service_account.py b/apps/api/plane/tests/unit/utils/test_service_account.py new file mode 100644 index 00000000000..cb991db5af3 --- /dev/null +++ b/apps/api/plane/tests/unit/utils/test_service_account.py @@ -0,0 +1,31 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Unit tests for service-account role resolution.""" + +import pytest + +from plane.utils.service_account import SERVICE_ACCOUNT_ROLES, resolve_service_account_role + + +@pytest.mark.unit +class TestResolveServiceAccountRole: + def test_valid_role_names_map_to_values(self): + assert resolve_service_account_role("admin") == 20 + assert resolve_service_account_role("member") == 15 + assert resolve_service_account_role("guest") == 5 + + def test_valid_integer_roles_pass_through(self): + for value in SERVICE_ACCOUNT_ROLES.values(): + assert resolve_service_account_role(value) == value + + def test_unknown_role_name_raises(self): + with pytest.raises(ValueError): + resolve_service_account_role("owner") + + def test_unsupported_integer_role_raises(self): + # An integer outside the allowed set (20/15/5) must not slip through and + # create a member with an invalid role. + with pytest.raises(ValueError): + resolve_service_account_role(99) diff --git a/apps/api/plane/utils/service_account.py b/apps/api/plane/utils/service_account.py new file mode 100644 index 00000000000..8b521852d68 --- /dev/null +++ b/apps/api/plane/utils/service_account.py @@ -0,0 +1,143 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Shared provisioning logic for workspace service (machine) accounts. + +A service account is a machine identity that acts *only* through an API token. +It is created directly (no invite, no email-verification, no password +round-trip) so it can be provisioned entirely programmatically, yet it is a +valid, distinct actor: it authenticates with its own token and its writes are +attributed to it via ``created_by``/``updated_by``. + +This module is the single source of truth for that flow. It is reused by both +the ``create_service_account`` management command and the admin-scoped +``/api/v1`` HTTP endpoint so the two can never drift. +""" + +# Python imports +import uuid +from dataclasses import dataclass + +# Django imports +from django.db import transaction + +# Module imports +from plane.db.models import APIToken, BotTypeEnum, User, Workspace, WorkspaceMember + +# Human-friendly role name -> WorkspaceMember.role value. +# Mirrors ROLE_CHOICES in plane/db/models/workspace.py ((20, Admin), (15, Member), (5, Guest)). +SERVICE_ACCOUNT_ROLES: dict[str, int] = {"admin": 20, "member": 15, "guest": 5} + +# Service accounts exist to provision a workspace end to end over the API, so an +# admin seat is the justified default. Narrow it with --role when a service +# account only needs member/guest scope. +DEFAULT_SERVICE_ACCOUNT_ROLE = "admin" + + +@dataclass +class ServiceAccount: + """Result of :func:`create_service_account`. + + ``token`` is the *plaintext* API key. Plane stores API tokens verbatim (the + authentication layer looks them up by exact match), so this is the value the + caller must persist — it is surfaced here exactly once at creation time. + """ + + user: User + member: WorkspaceMember + api_token: APIToken + + @property + def token(self) -> str: + return self.api_token.token + + +def resolve_service_account_role(role: str | int) -> int: + """Coerce a role name (or raw value) to a WorkspaceMember.role integer.""" + if isinstance(role, int): + if role in SERVICE_ACCOUNT_ROLES.values(): + return role + valid = ", ".join(str(v) for v in SERVICE_ACCOUNT_ROLES.values()) + raise ValueError(f"Invalid role '{role}'. Choose one of: {valid}.") + try: + return SERVICE_ACCOUNT_ROLES[role] + except KeyError: + valid = ", ".join(SERVICE_ACCOUNT_ROLES) + raise ValueError(f"Invalid role '{role}'. Choose one of: {valid}.") + + +@transaction.atomic +def create_service_account( + *, + workspace: Workspace, + name: str, + role: str | int = DEFAULT_SERVICE_ACCOUNT_ROLE, + email: str | None = None, + description: str = "", +) -> ServiceAccount: + """Create a service account in ``workspace`` and mint its API token. + + Creates, in a single transaction: + + * an ACTIVE, email-verified :class:`User` marked as a bot + (``is_bot=True``, ``bot_type=SERVICE``) with an unusable password, + * a :class:`WorkspaceMember` binding it to ``workspace`` at ``role``, + * an :class:`APIToken` (``user_type=Bot``, ``is_service=True``, + workspace-scoped) whose plaintext value is returned on the result. + + No email is sent and no password is ever round-tripped. ``is_bot=True`` + additionally blocks the interactive login/signup flow + (``BOT_USER_LOGIN_FORBIDDEN``), so the identity can be used *only* via its + token. + """ + role_value = resolve_service_account_role(role) + + # A service account never logs in, so username/email are internal, unique + # identifiers rather than human contact addresses. + unique = uuid.uuid4().hex + username = f"svc_{unique}" + if not email: + email = f"{username}@service.plane.local" + + user = User( + username=username, + email=email, + display_name=name, + first_name=name, + last_name="", + # Machine identity that acts only through API tokens (mirrors the + # WORKSPACE_SEED bot). is_bot=True forbids interactive login/signup. + is_bot=True, + bot_type=BotTypeEnum.SERVICE, + # Active + email verified so it is a valid actor with no accept flow: + # APIKeyAuthentication requires user__is_active, and downstream code + # treats a verified email as a fully onboarded account. + is_active=True, + is_email_verified=True, + is_email_valid=True, + is_password_autoset=True, + ) + # No password round-trip: the account authenticates only via its API token, + # so give it an unusable password that can never be used to log in. + user.set_unusable_password() + user.save() + + member = WorkspaceMember.objects.create( + workspace=workspace, + member=user, + role=role_value, + company_role="", + ) + + api_token = APIToken.objects.create( + label=name, + description=description or f"Service account token for {name}", + user=user, + # 1 == Bot (see APIToken.user_type choices). + user_type=1, + workspace=workspace, + is_service=True, + ) + + return ServiceAccount(user=user, member=member, api_token=api_token) diff --git a/docs/service-accounts.md b/docs/service-accounts.md new file mode 100644 index 00000000000..1d1939551c5 --- /dev/null +++ b/docs/service-accounts.md @@ -0,0 +1,126 @@ +# Service Accounts + +A **service account** is a machine identity for provisioning and automating a +workspace entirely over the API. Unlike a human member, a service account: + +- is created directly — **no invite, no email-verification, no password round-trip**; +- **cannot log in interactively** (email/password, magic code, or OAuth are all + rejected — it is a bot, see `BOT_USER_LOGIN_FORBIDDEN`); +- acts **only through its API token**; and +- is a **valid, distinct actor**: it authenticates as itself and everything it + creates is attributed to it (`created_by`). + +This makes a workspace fully provisionable by scripts and agents without a human +in the loop. + +## What gets created + +Creating a service account performs three writes in a single transaction: + +1. A `User` that is **active** (`is_active=True`) and **email-verified** + (`is_email_verified=True`, `is_email_valid=True`), flagged as a bot + (`is_bot=True`, `bot_type=SERVICE`) with an unusable password + (`is_password_autoset=True`). Its `username`/`email` are unique, synthetic + identifiers — no mail is ever sent to them. +2. A `WorkspaceMember` binding the user to the workspace at the requested role. +3. An `APIToken` (`user_type=Bot`, `is_service=True`, scoped to the workspace). + Its plaintext value is printed/returned **once** — store it securely, it + cannot be retrieved again. + +Because `is_bot=True`, the account is intentionally omitted from the human-facing +workspace **member list** (the same behaviour as the built-in workspace-seed bot). +It is still a full member for authorization and attribution: it passes permission +checks and appears as the `created_by`/`updated_by` actor on everything it writes, +and it is visible via the API (e.g. `GET /api/v1/workspaces/{slug}/members/`). + +## Roles + +| Name | Value | Capability | +| -------- | ----- | ------------------------------------------- | +| `admin` | 20 | Full workspace admin (default) | +| `member` | 15 | Create/update most entities | +| `guest` | 5 | Read-oriented, limited | + +The default is `admin`, since a service account usually needs to provision a +workspace end to end. Narrow it with the `role` argument when a token only needs +member/guest scope. + +## Creating one — management command + +```bash +python manage.py create_service_account \ + --workspace \ + --name "CI Provisioner" \ + --role admin +``` + +Arguments: + +- `--workspace` (required) — target workspace slug. +- `--name` (required) — display name for the account. +- `--role` — `admin` (default), `member`, or `guest`. +- `--email` — optional; a unique synthetic address is generated when omitted. +- `--description` — optional description stored on the token. + +The command prints the account details and the API token (shown once): + +```text +Service account created successfully + user_id : 1c2f...c7f3 + username : svc_9d89245e45164385a00e42ff6056cbce + email : svc_9d89245e45164385a00e42ff6056cbce@service.plane.local + role : admin + workspace: my-workspace +API token (shown once — store it securely): + plane_api_9d89245e45164385a00e42ff6056cbce +``` + +Use the token with the public API via the `X-Api-Key` header: + +```bash +curl -H "X-Api-Key: plane_api_..." https:///api/v1/users/me/ +``` + +## Creating one — admin HTTP endpoint (optional) + +The same flow is exposed over HTTP for callers that already hold a +**workspace-admin** API token: + +```http +POST /api/v1/workspaces/{slug}/service-accounts/ +X-Api-Key: +Content-Type: application/json + +{ "name": "CI Provisioner", "role": "admin", "description": "optional" } +``` + +Response `201 Created` (the `token` is returned once): + +```json +{ + "id": "1c2f...c7f3", + "username": "svc_...", + "email": "svc_...@service.plane.local", + "display_name": "CI Provisioner", + "role": 20, + "workspace": "8f0e...9ab1", + "token": "plane_api_..." +} +``` + +The caller must be an **admin** of `{slug}` (enforced by `WorkspaceOwnerPermission`); +any other caller receives `403 Forbidden`. This endpoint requires an existing +admin token, so the first service account in a fresh instance is typically minted +with the management command above. + +## Notes + +- Tokens are stored verbatim and matched exactly at authentication time; there is + no way to recover a lost token — mint a new account/token instead. +- The HTTP endpoint's response body is redacted from the API request log + (`api_activity_logs`), so the minted token is never persisted there in plaintext. +- Provisioning is attributed to the acting admin: the created `WorkspaceMember` + and `APIToken` carry that admin as `created_by` (the management command has no + acting user, so those rows are created with no `created_by`). +- The token authenticates only while the account stays active; deactivating the + `User` (`is_active=False`) immediately revokes access. From 85dd5a46be38f41c013b077c2d4ff0d095e4898f Mon Sep 17 00:00:00 2001 From: Seena Fallah Date: Thu, 23 Jul 2026 19:52:43 +0000 Subject: [PATCH 2/5] feat(api): accept caller-chosen username and display_name for service accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let an external provisioner assign stable, readable identity to a service account instead of the server-generated defaults, so accounts can be created idempotently (looked up by a known username) and show a readable name in the workspace members UI. - POST /api/v1/workspaces/{slug}/service-accounts/ and the create_service_account command accept optional username and display_name (--username/--display-name). - username is globally unique and bounded only by length (max 128) — matching regular Plane usernames, which have no charset validator (a random uuid handle). A collision returns 409 {"code": "USERNAME_ALREADY_EXISTS"} (or a CommandError), never a silently mutated name. A race past the pre-check is caught and mapped the same way; any other IntegrityError is re-raised rather than mislabeled. - display_name lands on User.display_name; both fields fall back to synthetic / name when omitted, and a blank value normalizes to the same fallback on both paths. The synthetic email is always a fresh unique svc_ address. - Response echoes the effective username + display_name. - Docs + contract/unit tests: custom identity, collision, faithful race, genuine atomic rollback, blank normalization, and command flag round-trip. Token semantics are unchanged (token lifecycle is a separate change). Signed-off-by: Seena Fallah --- .../plane/api/serializers/service_account.py | 23 ++- apps/api/plane/api/views/service_account.py | 44 ++++- .../commands/create_service_account.py | 43 +++- .../contract/api/test_service_account.py | 186 ++++++++++++++++++ apps/api/plane/utils/service_account.py | 24 ++- docs/service-accounts.md | 58 ++++-- 6 files changed, 342 insertions(+), 36 deletions(-) diff --git a/apps/api/plane/api/serializers/service_account.py b/apps/api/plane/api/serializers/service_account.py index edb4bc38e99..d32ea1018b3 100644 --- a/apps/api/plane/api/serializers/service_account.py +++ b/apps/api/plane/api/serializers/service_account.py @@ -12,12 +12,33 @@ class ServiceAccountCreateSerializer(serializers.Serializer): """Request body for provisioning a workspace service account.""" - name = serializers.CharField(max_length=255, help_text="Display name for the service account") + name = serializers.CharField(max_length=255, help_text="Name for the service account (used as the token label)") role = serializers.ChoiceField( choices=sorted(SERVICE_ACCOUNT_ROLES), default=DEFAULT_SERVICE_ACCOUNT_ROLE, help_text="Workspace role: admin, member, or guest", ) + # Optional caller-chosen identity. username must be globally unique and, like + # every Plane username, is bounded only by max_length (128); a collision is + # rejected, never silently mutated. Omitting either falls back to a synthetic + # username / to `name` for the display name. + # allow_blank so a "" from a caller is normalized to the synthetic/fallback + # value (same as omitting it, and identical to the management command), rather + # than 400-ing where the command would accept it. + username = serializers.CharField( + required=False, + allow_null=True, + allow_blank=True, + max_length=128, + help_text="Optional globally-unique username; a synthetic svc_ is generated when omitted", + ) + display_name = serializers.CharField( + required=False, + allow_null=True, + allow_blank=True, + max_length=255, + help_text="Optional display name shown in the members UI; falls back to name when omitted", + ) description = serializers.CharField( required=False, allow_blank=True, diff --git a/apps/api/plane/api/views/service_account.py b/apps/api/plane/api/views/service_account.py index 349afde96ec..f57edc66de5 100644 --- a/apps/api/plane/api/views/service_account.py +++ b/apps/api/plane/api/views/service_account.py @@ -2,6 +2,9 @@ # SPDX-License-Identifier: AGPL-3.0-only # See the LICENSE file for details. +# Django imports +from django.db import IntegrityError + # Third party imports from rest_framework import status from rest_framework.response import Response @@ -13,12 +16,15 @@ ServiceAccountCreateSerializer, ServiceAccountSerializer, ) -from plane.db.models import Workspace +from plane.db.models import User, Workspace from plane.middleware.logger import redact_response_body from plane.utils.permissions import WorkspaceOwnerPermission from plane.utils.openapi.parameters import WORKSPACE_SLUG_PARAMETER from plane.utils.service_account import create_service_account +# Machine-readable error surfaced when a caller-chosen username is already taken. +USERNAME_CONFLICT = {"error": "A user with this username already exists.", "code": "USERNAME_ALREADY_EXISTS"} + class ServiceAccountAPIEndpoint(BaseAPIView): """Admin-scoped endpoint for provisioning workspace service accounts. @@ -45,7 +51,8 @@ class ServiceAccountAPIEndpoint(BaseAPIView): 201: OpenApiResponse( description="Service account created", response=ServiceAccountSerializer, - ) + ), + 409: OpenApiResponse(description="A user with the requested username already exists"), }, ) def post(self, request, slug): @@ -55,12 +62,33 @@ def post(self, request, slug): serializer.is_valid(raise_exception=True) data = serializer.validated_data - service_account = create_service_account( - workspace=workspace, - name=data["name"], - role=data["role"], - description=data.get("description", ""), - ) + username = data.get("username") + # Reject a taken username with a machine-readable code — never silently + # mutate it into a unique one. The insert below is still wrapped so a + # race between this check and the create is reported the same way. + if username and User.objects.filter(username=username).exists(): + return Response(USERNAME_CONFLICT, status=status.HTTP_409_CONFLICT) + + try: + service_account = create_service_account( + workspace=workspace, + name=data["name"], + role=data["role"], + description=data.get("description", ""), + username=username, + display_name=data.get("display_name"), + ) + except IntegrityError: + # The only caller-controlled unique field here is the username + # (email/token are server-generated). If it is now taken — including + # a race that slipped past the pre-check — report the conflict. Any + # other IntegrityError is unexpected and must not be mislabeled, so + # let it surface via BaseAPIView.handle_exception. The helper's + # @transaction.atomic has already rolled back, so this SELECT runs on + # a clean connection. + if username and User.objects.filter(username=username).exists(): + return Response(USERNAME_CONFLICT, status=status.HTTP_409_CONFLICT) + raise response = ServiceAccountSerializer( { diff --git a/apps/api/plane/db/management/commands/create_service_account.py b/apps/api/plane/db/management/commands/create_service_account.py index d934f0ea50b..2c49db50646 100644 --- a/apps/api/plane/db/management/commands/create_service_account.py +++ b/apps/api/plane/db/management/commands/create_service_account.py @@ -24,7 +24,7 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument("--workspace", type=str, required=True, help="Workspace slug") - parser.add_argument("--name", type=str, required=True, help="Display name for the service account") + parser.add_argument("--name", type=str, required=True, help="Name for the service account (token label)") parser.add_argument( "--role", type=str, @@ -32,6 +32,19 @@ def add_arguments(self, parser): default=DEFAULT_SERVICE_ACCOUNT_ROLE, help=f"Workspace role (default: {DEFAULT_SERVICE_ACCOUNT_ROLE})", ) + parser.add_argument( + "--username", + type=str, + default=None, + help="Optional globally-unique username; a synthetic svc_ is generated when omitted", + ) + parser.add_argument( + "--display-name", + type=str, + default=None, + dest="display_name", + help="Optional display name shown in the members UI; falls back to --name when omitted", + ) parser.add_argument( "--email", type=str, @@ -41,6 +54,9 @@ def add_arguments(self, parser): parser.add_argument("--description", type=str, default="", help="Optional token description") def handle(self, *args, **options): + if not options["name"].strip(): + raise CommandError("--name must not be empty") + workspace = Workspace.objects.filter(slug=options["workspace"]).first() if workspace is None: raise CommandError(f"Workspace with slug '{options['workspace']}' does not exist") @@ -49,6 +65,10 @@ def handle(self, *args, **options): if email and User.objects.filter(email=email).exists(): raise CommandError(f"A user with email '{email}' already exists") + username = options.get("username") + if username and User.objects.filter(username=username).exists(): + raise CommandError(f"A user with username '{username}' already exists") + try: service_account = create_service_account( workspace=workspace, @@ -56,21 +76,24 @@ def handle(self, *args, **options): role=options["role"], email=options["email"], description=options["description"], + username=username, + display_name=options.get("display_name"), ) except IntegrityError as exc: - # A concurrent insert (email race that slipped past the check above) - # or an extremely unlikely synthetic username/email collision surfaces - # here — report it readably instead of a raw traceback. The helper's + # A concurrent insert (email/username race that slipped past the checks + # above) or an extremely unlikely synthetic collision surfaces here — + # report it readably instead of a raw traceback. The helper's # @transaction.atomic has already rolled back, so no partial account # remains. - raise CommandError(f"Could not create the service account — the email is already in use: {exc}") + raise CommandError(f"Could not create the service account — the email or username is already in use: {exc}") user = service_account.user self.stdout.write(self.style.SUCCESS("Service account created successfully")) - self.stdout.write(f" user_id : {user.id}") - self.stdout.write(f" username : {user.username}") - self.stdout.write(f" email : {user.email}") - self.stdout.write(f" role : {options['role']}") - self.stdout.write(f" workspace: {workspace.slug}") + self.stdout.write(f" user_id : {user.id}") + self.stdout.write(f" username : {user.username}") + self.stdout.write(f" display_name: {user.display_name}") + self.stdout.write(f" email : {user.email}") + self.stdout.write(f" role : {options['role']}") + self.stdout.write(f" workspace : {workspace.slug}") self.stdout.write(self.style.WARNING("API token (shown once — store it securely):")) self.stdout.write(f" {service_account.token}") diff --git a/apps/api/plane/tests/contract/api/test_service_account.py b/apps/api/plane/tests/contract/api/test_service_account.py index 5187ba358cd..15b368e787e 100644 --- a/apps/api/plane/tests/contract/api/test_service_account.py +++ b/apps/api/plane/tests/contract/api/test_service_account.py @@ -155,6 +155,54 @@ def test_command_integrity_error_becomes_command_error(self, workspace): with pytest.raises(CommandError): call_command("create_service_account", workspace=workspace.slug, name="Race", role="admin") + @pytest.mark.django_db + def test_command_username_and_display_name_round_trip(self, workspace): + call_command( + "create_service_account", + workspace=workspace.slug, + name="CI", + role="admin", + username="ci-bot", + display_name="CI Bot", + ) + + member = _service_account_for(workspace) + assert member.member.username == "ci-bot" + assert member.member.display_name == "CI Bot" + + @pytest.mark.django_db + def test_command_omitted_identity_falls_back_to_synthetic(self, workspace): + call_command("create_service_account", workspace=workspace.slug, name="Fallback Bot", role="admin") + + user = _service_account_for(workspace).member + assert user.username.startswith("svc_") + assert user.display_name == "Fallback Bot" + + @pytest.mark.django_db + def test_command_duplicate_username_errors_cleanly(self, workspace): + User.objects.create(username="ci-bot", email="someone@plane.so") + + from django.core.management.base import CommandError + + with pytest.raises(CommandError): + call_command( + "create_service_account", + workspace=workspace.slug, + name="Dup", + role="admin", + username="ci-bot", + ) + + # No partial service account leaked into the workspace. + assert not WorkspaceMember.objects.filter(workspace=workspace, member__bot_type=BotTypeEnum.SERVICE).exists() + + @pytest.mark.django_db + def test_command_blank_name_errors(self, workspace): + from django.core.management.base import CommandError + + with pytest.raises(CommandError): + call_command("create_service_account", workspace=workspace.slug, name="", role="admin") + @pytest.mark.contract class TestServiceAccountEndpoint: @@ -244,3 +292,141 @@ def test_unauthenticated_is_rejected(self, api_client, workspace): format="json", ) assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) + + @pytest.mark.django_db + def test_custom_username_and_display_name(self, api_key_client, workspace): + response = api_key_client.post( + self._url(workspace.slug), + {"name": "CI", "role": "admin", "username": "ci-provisioner", "display_name": "CI Provisioner"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + # The response echoes the effective identity. + assert response.data["username"] == "ci-provisioner" + assert response.data["display_name"] == "CI Provisioner" + # And it landed on the created user (so the members UI shows it). + user = User.objects.get(id=response.data["id"]) + assert user.username == "ci-provisioner" + assert user.display_name == "CI Provisioner" + + @pytest.mark.django_db + def test_omitted_identity_falls_back_to_synthetic(self, api_key_client, workspace): + response = api_key_client.post( + self._url(workspace.slug), + {"name": "Fallback Bot", "role": "admin"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["username"].startswith("svc_") + assert response.data["display_name"] == "Fallback Bot" + + @pytest.mark.django_db + def test_duplicate_username_is_rejected_with_code(self, api_key_client, workspace): + User.objects.create(username="ci-provisioner", email="human@plane.so") + + response = api_key_client.post( + self._url(workspace.slug), + {"name": "CI", "role": "admin", "username": "ci-provisioner"}, + format="json", + ) + + # Collision → 409 with a machine-readable code; the name is never mutated. + assert response.status_code == status.HTTP_409_CONFLICT, response.data + assert response.data["code"] == "USERNAME_ALREADY_EXISTS" + # No service account leaked into the workspace. + assert not WorkspaceMember.objects.filter(workspace=workspace, member__bot_type=BotTypeEnum.SERVICE).exists() + + @pytest.mark.django_db + def test_username_race_returns_409(self, api_key_client, workspace): + # Simulate a race: the username is free at the pre-check, then another + # actor creates it and our insert raises IntegrityError. The scoped + # handler re-checks, sees it now exists, and returns the same 409. + from django.db import IntegrityError + + def racing_create(**kwargs): + User.objects.create(username="raced", email="racer@plane.so") + raise IntegrityError("duplicate key value violates unique constraint") + + with patch("plane.api.views.service_account.create_service_account", side_effect=racing_create): + response = api_key_client.post( + self._url(workspace.slug), + {"name": "Race", "role": "admin", "username": "raced"}, + format="json", + ) + + assert response.status_code == status.HTTP_409_CONFLICT, response.data + assert response.data["code"] == "USERNAME_ALREADY_EXISTS" + + @pytest.mark.django_db + def test_non_username_integrity_error_is_not_mislabeled(self, api_key_client, workspace): + # An IntegrityError unrelated to the username (the username is still free) + # must not be reported as a username conflict — it falls through to the + # base handler (HTTP 400) instead. + from django.db import IntegrityError + + with patch( + "plane.api.views.service_account.create_service_account", + side_effect=IntegrityError("some other constraint"), + ): + response = api_key_client.post( + self._url(workspace.slug), + {"name": "Boom", "role": "admin", "username": "unique-name"}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data.get("code") != "USERNAME_ALREADY_EXISTS" + + @pytest.mark.django_db + def test_custom_username_with_omitted_display_name_falls_back_to_name(self, api_key_client, workspace): + response = api_key_client.post( + self._url(workspace.slug), + {"name": "CI Provisioner", "role": "admin", "username": "ci-only"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["username"] == "ci-only" + assert response.data["display_name"] == "CI Provisioner" + + @pytest.mark.django_db + def test_blank_identity_normalizes_to_synthetic(self, api_key_client, workspace): + # A blank username/display_name is treated as omitted (identical to the + # management command), not a 400. + response = api_key_client.post( + self._url(workspace.slug), + {"name": "Blanky", "role": "admin", "username": "", "display_name": ""}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["username"].startswith("svc_") + assert response.data["display_name"] == "Blanky" + + +@pytest.mark.contract +class TestServiceAccountHelper: + """Direct tests of the shared create_service_account helper.""" + + @pytest.mark.django_db + def test_duplicate_username_rolls_back_atomically(self, workspace): + # A genuine (non-mocked) unique violation: the helper is @transaction.atomic, + # so the failed second creation must leave nothing behind. + from django.db import IntegrityError + + from plane.utils.service_account import create_service_account + + create_service_account(workspace=workspace, name="First", username="dup") + + users_before = User.objects.count() + members_before = WorkspaceMember.objects.count() + tokens_before = APIToken.objects.count() + + with pytest.raises(IntegrityError): + create_service_account(workspace=workspace, name="Second", username="dup") + + assert User.objects.count() == users_before + assert WorkspaceMember.objects.count() == members_before + assert APIToken.objects.count() == tokens_before diff --git a/apps/api/plane/utils/service_account.py b/apps/api/plane/utils/service_account.py index 8b521852d68..f581040c805 100644 --- a/apps/api/plane/utils/service_account.py +++ b/apps/api/plane/utils/service_account.py @@ -75,6 +75,8 @@ def create_service_account( role: str | int = DEFAULT_SERVICE_ACCOUNT_ROLE, email: str | None = None, description: str = "", + username: str | None = None, + display_name: str | None = None, ) -> ServiceAccount: """Create a service account in ``workspace`` and mint its API token. @@ -86,6 +88,13 @@ def create_service_account( * an :class:`APIToken` (``user_type=Bot``, ``is_service=True``, workspace-scoped) whose plaintext value is returned on the result. + ``username`` and ``display_name`` are optional caller-chosen identity fields. + ``username`` must be globally unique (a collision raises ``IntegrityError`` + from the DB insert — the caller is expected to check for it and surface a + readable error); when omitted a synthetic ``svc_`` value is generated. + ``display_name`` is what the workspace members UI shows; it falls back to + ``name`` when omitted. + No email is sent and no password is ever round-tripped. ``is_bot=True`` additionally blocks the interactive login/signup flow (``BOT_USER_LOGIN_FORBIDDEN``), so the identity can be used *only* via its @@ -93,17 +102,22 @@ def create_service_account( """ role_value = resolve_service_account_role(role) - # A service account never logs in, so username/email are internal, unique - # identifiers rather than human contact addresses. + # A service account never logs in, so a caller-omitted username/email are + # internal, unique identifiers rather than human contact addresses. The + # synthetic email is always derived from a fresh uuid (never the caller's + # username) so it stays valid and unique regardless of the username. unique = uuid.uuid4().hex - username = f"svc_{unique}" + if not username: + username = f"svc_{unique}" if not email: - email = f"{username}@service.plane.local" + email = f"svc_{unique}@service.plane.local" + if not display_name: + display_name = name user = User( username=username, email=email, - display_name=name, + display_name=display_name, first_name=name, last_name="", # Machine identity that acts only through API tokens (mirrors the diff --git a/docs/service-accounts.md b/docs/service-accounts.md index 1d1939551c5..a0475efb8ea 100644 --- a/docs/service-accounts.md +++ b/docs/service-accounts.md @@ -20,8 +20,10 @@ Creating a service account performs three writes in a single transaction: 1. A `User` that is **active** (`is_active=True`) and **email-verified** (`is_email_verified=True`, `is_email_valid=True`), flagged as a bot (`is_bot=True`, `bot_type=SERVICE`) with an unusable password - (`is_password_autoset=True`). Its `username`/`email` are unique, synthetic - identifiers — no mail is ever sent to them. + (`is_password_autoset=True`). Its `username` and `display_name` may be + caller-chosen (see [Identity fields](#identity-fields)) or default to + synthetic values; its `email` is always a unique synthetic address — no mail + is ever sent to it. 2. A `WorkspaceMember` binding the user to the workspace at the requested role. 3. An `APIToken` (`user_type=Bot`, `is_service=True`, scoped to the workspace). Its plaintext value is printed/returned **once** — store it securely, it @@ -51,14 +53,20 @@ member/guest scope. python manage.py create_service_account \ --workspace \ --name "CI Provisioner" \ - --role admin + --role admin \ + --username ci-provisioner \ + --display-name "CI Provisioner" ``` Arguments: - `--workspace` (required) — target workspace slug. -- `--name` (required) — display name for the account. +- `--name` (required) — name for the account; used as the API token label. - `--role` — `admin` (default), `member`, or `guest`. +- `--username` — optional globally-unique username (see [Identity fields](#identity-fields)); + a synthetic `svc_` value is generated when omitted. +- `--display-name` — optional display name shown in the members UI; falls back to + `--name` when omitted. - `--email` — optional; a unique synthetic address is generated when omitted. - `--description` — optional description stored on the token. @@ -66,11 +74,12 @@ The command prints the account details and the API token (shown once): ```text Service account created successfully - user_id : 1c2f...c7f3 - username : svc_9d89245e45164385a00e42ff6056cbce - email : svc_9d89245e45164385a00e42ff6056cbce@service.plane.local - role : admin - workspace: my-workspace + user_id : 1c2f...c7f3 + username : ci-provisioner + display_name: CI Provisioner + email : svc_9d89245e45164385a00e42ff6056cbce@service.plane.local + role : admin + workspace : my-workspace API token (shown once — store it securely): plane_api_9d89245e45164385a00e42ff6056cbce ``` @@ -91,15 +100,22 @@ POST /api/v1/workspaces/{slug}/service-accounts/ X-Api-Key: Content-Type: application/json -{ "name": "CI Provisioner", "role": "admin", "description": "optional" } +{ + "name": "CI Provisioner", + "role": "admin", + "username": "ci-provisioner", + "display_name": "CI Provisioner", + "description": "optional" +} ``` -Response `201 Created` (the `token` is returned once): +`username` and `display_name` are optional (see [Identity fields](#identity-fields)). +Response `201 Created` (the `token` is returned once) echoes the effective values: ```json { "id": "1c2f...c7f3", - "username": "svc_...", + "username": "ci-provisioner", "email": "svc_...@service.plane.local", "display_name": "CI Provisioner", "role": 20, @@ -113,6 +129,24 @@ any other caller receives `403 Forbidden`. This endpoint requires an existing admin token, so the first service account in a fresh instance is typically minted with the management command above. +## Identity fields + +`username` and `display_name` let an external provisioner assign stable, readable +identity instead of the server-generated defaults: + +- **`username`** — a globally-unique handle. Use it to provision idempotently: + re-creating with a username that already exists is rejected with **`409 Conflict`** + and a machine-readable body `{"error": ..., "code": "USERNAME_ALREADY_EXISTS"}`; + the name is **never** silently mutated into a unique variant. Like every Plane + username it is bounded only by length (max 128 characters) — Plane applies no + charset validator to usernames (regular accounts get a random `uuid` handle), so + none is imposed here either. Omit it to get a synthetic `svc_` handle. +- **`display_name`** — the label the workspace members UI shows. Omit it to fall + back to `name`. + +On the management command, a taken `--username` (or `--email`) fails with a clear +`CommandError` instead of a raw traceback. + ## Notes - Tokens are stored verbatim and matched exactly at authentication time; there is From 96c8b4f93bbe44929cbcc3d0b303d7fed32003a9 Mon Sep 17 00:00:00 2001 From: Seena Fallah Date: Thu, 23 Jul 2026 20:34:46 +0000 Subject: [PATCH 3/5] feat(api): service account token lifecycle + decommission endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the lifecycle operations an external reconcile/rotation loop needs on top of the atomic create. All are admin-scoped (WorkspaceOwnerPermission), follow public-API conventions (APIKeyAuthentication, cursor pagination, OpenAPI), and are scoped to a SERVICE bot member of the workspace. - GET /workspaces/{slug}/service-accounts/{user_id}/tokens/ — list tokens with metadata (label, timestamps, expiry, is_active); the secret value is always withheld. - POST .../tokens/ — mint an additional workspace-scoped bot token (value once, optional expired_at). - POST .../tokens/{token_id}/rotate/ — atomically mint a replacement (value once) and deactivate the old token, so the old value stops authenticating. - DELETE .../tokens/{token_id}/ — revoke (soft-delete) a token. - DELETE /workspaces/{slug}/service-accounts/{user_id}/ — decommission: deactivate all tokens, remove ProjectMember + WorkspaceMember rows, deactivate the User (row kept so created_by attribution survives). Hard-guarded to is_bot + bot_type=SERVICE; a human or other bot returns 400. - Mint/rotate responses carry the plaintext token, so their bodies are redacted from api_activity_logs (reuses redact_response_body from the create endpoint). - Resolving the account intentionally does not filter membership is_active, so a deactivated (but not decommissioned) account whose tokens still authenticate can still be listed, revoked, and decommissioned. - Shared mint/rotate/decommission helpers in plane/utils/service_account.py. Tests cover: list masks values, mint/rotate return + authenticate, rotate/revoke invalidate the old token, decommission cascade + attribution survival, the is_bot/SERVICE guard (human + other bot -> 400), cross-workspace isolation, non-admin denial, and mint/rotate log redaction. A module autouse fixture resets the API-key throttle cache per test (otherwise the throttle counter is shared and uncleared across tests). Docs updated. Signed-off-by: Seena Fallah --- apps/api/plane/api/serializers/__init__.py | 8 +- .../plane/api/serializers/service_account.py | 51 ++++ apps/api/plane/api/urls/service_account.py | 28 +- apps/api/plane/api/views/__init__.py | 8 +- apps/api/plane/api/views/service_account.py | 266 +++++++++++++++- .../contract/api/test_service_account.py | 284 +++++++++++++++++- apps/api/plane/utils/service_account.py | 74 ++++- docs/service-accounts.md | 56 ++++ 8 files changed, 760 insertions(+), 15 deletions(-) diff --git a/apps/api/plane/api/serializers/__init__.py b/apps/api/plane/api/serializers/__init__.py index 2c6365ef3cb..be5694d5bf5 100644 --- a/apps/api/plane/api/serializers/__init__.py +++ b/apps/api/plane/api/serializers/__init__.py @@ -67,5 +67,11 @@ WorkspaceMemberLiteAPISerializer, ProjectMemberLiteAPISerializer, ) -from .service_account import ServiceAccountCreateSerializer, ServiceAccountSerializer +from .service_account import ( + ServiceAccountCreateSerializer, + ServiceAccountSerializer, + ServiceAccountTokenCreateSerializer, + ServiceAccountTokenCreatedSerializer, + ServiceAccountTokenSerializer, +) from .sticky import StickySerializer diff --git a/apps/api/plane/api/serializers/service_account.py b/apps/api/plane/api/serializers/service_account.py index d32ea1018b3..2939a7f7d70 100644 --- a/apps/api/plane/api/serializers/service_account.py +++ b/apps/api/plane/api/serializers/service_account.py @@ -6,6 +6,8 @@ from rest_framework import serializers # Module imports +from plane.api.serializers.base import BaseSerializer +from plane.db.models import APIToken from plane.utils.service_account import DEFAULT_SERVICE_ACCOUNT_ROLE, SERVICE_ACCOUNT_ROLES @@ -60,3 +62,52 @@ class ServiceAccountSerializer(serializers.Serializer): role = serializers.IntegerField(read_only=True, help_text="Workspace role value (20 admin, 15 member, 5 guest)") workspace = serializers.UUIDField(read_only=True) token = serializers.CharField(read_only=True, help_text="Plaintext API token — shown once") + + +class ServiceAccountTokenSerializer(BaseSerializer): + """Read/list view of a service account's API token. + + The secret ``token`` value is intentionally NOT a field here, so it can never + be exposed by the list endpoint. + """ + + class Meta: + model = APIToken + fields = [ + "id", + "label", + "description", + "is_active", + "is_service", + "user_type", + "created_at", + "updated_at", + "expired_at", + "last_used", + ] + read_only_fields = fields + + +class ServiceAccountTokenCreateSerializer(serializers.Serializer): + """Request body for minting or rotating a service account token.""" + + label = serializers.CharField( + required=False, allow_blank=True, max_length=255, help_text="Optional human-readable token label" + ) + description = serializers.CharField( + required=False, allow_blank=True, default="", help_text="Optional token description" + ) + expired_at = serializers.DateTimeField( + required=False, allow_null=True, default=None, help_text="Optional expiry; the token never expires when omitted" + ) + + +class ServiceAccountTokenCreatedSerializer(serializers.Serializer): + """Response for a newly minted/rotated token — includes the value ONCE.""" + + id = serializers.UUIDField(read_only=True) + label = serializers.CharField(read_only=True) + is_active = serializers.BooleanField(read_only=True) + created_at = serializers.DateTimeField(read_only=True) + expired_at = serializers.DateTimeField(read_only=True, allow_null=True) + token = serializers.CharField(read_only=True, help_text="Plaintext API token — shown once") diff --git a/apps/api/plane/api/urls/service_account.py b/apps/api/plane/api/urls/service_account.py index 9bc59f33ce9..46f8c033009 100644 --- a/apps/api/plane/api/urls/service_account.py +++ b/apps/api/plane/api/urls/service_account.py @@ -4,7 +4,13 @@ from django.urls import path -from plane.api.views import ServiceAccountAPIEndpoint +from plane.api.views import ( + ServiceAccountAPIEndpoint, + ServiceAccountDetailAPIEndpoint, + ServiceAccountTokenAPIEndpoint, + ServiceAccountTokenDetailAPIEndpoint, + ServiceAccountTokenRotateAPIEndpoint, +) urlpatterns = [ path( @@ -12,4 +18,24 @@ ServiceAccountAPIEndpoint.as_view(http_method_names=["post"]), name="service-accounts", ), + path( + "workspaces//service-accounts//", + ServiceAccountDetailAPIEndpoint.as_view(http_method_names=["delete"]), + name="service-account-detail", + ), + path( + "workspaces//service-accounts//tokens/", + ServiceAccountTokenAPIEndpoint.as_view(http_method_names=["get", "post"]), + name="service-account-tokens", + ), + path( + "workspaces//service-accounts//tokens//", + ServiceAccountTokenDetailAPIEndpoint.as_view(http_method_names=["delete"]), + name="service-account-token-detail", + ), + path( + "workspaces//service-accounts//tokens//rotate/", + ServiceAccountTokenRotateAPIEndpoint.as_view(http_method_names=["post"]), + name="service-account-token-rotate", + ), ] diff --git a/apps/api/plane/api/views/__init__.py b/apps/api/plane/api/views/__init__.py index 71ced15847f..3b98a69ec53 100644 --- a/apps/api/plane/api/views/__init__.py +++ b/apps/api/plane/api/views/__init__.py @@ -71,6 +71,12 @@ from .invite import WorkspaceInvitationsViewset -from .service_account import ServiceAccountAPIEndpoint +from .service_account import ( + ServiceAccountAPIEndpoint, + ServiceAccountDetailAPIEndpoint, + ServiceAccountTokenAPIEndpoint, + ServiceAccountTokenDetailAPIEndpoint, + ServiceAccountTokenRotateAPIEndpoint, +) from .sticky import StickyViewSet diff --git a/apps/api/plane/api/views/service_account.py b/apps/api/plane/api/views/service_account.py index f57edc66de5..23e49f8acca 100644 --- a/apps/api/plane/api/views/service_account.py +++ b/apps/api/plane/api/views/service_account.py @@ -8,23 +8,81 @@ # Third party imports from rest_framework import status from rest_framework.response import Response -from drf_spectacular.utils import extend_schema, OpenApiResponse, OpenApiRequest +from drf_spectacular.types import OpenApiTypes +from drf_spectacular.utils import extend_schema, OpenApiParameter, OpenApiResponse, OpenApiRequest # Module imports from plane.api.views.base import BaseAPIView from plane.api.serializers.service_account import ( ServiceAccountCreateSerializer, ServiceAccountSerializer, + ServiceAccountTokenCreateSerializer, + ServiceAccountTokenCreatedSerializer, + ServiceAccountTokenSerializer, ) -from plane.db.models import User, Workspace +from plane.db.models import APIToken, BotTypeEnum, User, Workspace, WorkspaceMember from plane.middleware.logger import redact_response_body from plane.utils.permissions import WorkspaceOwnerPermission -from plane.utils.openapi.parameters import WORKSPACE_SLUG_PARAMETER -from plane.utils.service_account import create_service_account +from plane.utils.openapi import ( + create_paginated_response, + DELETED_RESPONSE, + FORBIDDEN_RESPONSE, + NOT_FOUND_RESPONSE, + UNAUTHORIZED_RESPONSE, +) +from plane.utils.openapi.parameters import CURSOR_PARAMETER, PER_PAGE_PARAMETER, WORKSPACE_SLUG_PARAMETER +from plane.utils.service_account import ( + create_service_account, + decommission_service_account, + mint_service_account_token, + rotate_service_account_token, +) # Machine-readable error surfaced when a caller-chosen username is already taken. USERNAME_CONFLICT = {"error": "A user with this username already exists.", "code": "USERNAME_ALREADY_EXISTS"} +# Path parameters shared by the service-account lifecycle endpoints. +SERVICE_ACCOUNT_ID_PARAMETER = OpenApiParameter( + name="user_id", + description="Service account user id", + required=True, + type=OpenApiTypes.UUID, + location=OpenApiParameter.PATH, +) +TOKEN_ID_PARAMETER = OpenApiParameter( + name="token_id", + description="API token id", + required=True, + type=OpenApiTypes.UUID, + location=OpenApiParameter.PATH, +) + + +def get_service_account_member(slug, user_id): + """Return the WorkspaceMember for a SERVICE bot in the workspace. + + Returns ``None`` when ``user_id`` is not a service account + (``is_bot`` + ``bot_type=SERVICE``) member of ``slug``, so callers can map + that to a 404. The member carries the resolved ``member`` (user) and + ``workspace``. + + Membership ``is_active`` is intentionally NOT filtered: API auth ignores + membership (it checks only ``token.is_active`` + ``user__is_active``), so a + merely-*deactivated* account must stay manageable — otherwise its still-live + tokens could never be listed or revoked. A *decommissioned* account is + soft-deleted and so excluded here by the default (soft-delete) manager. + """ + return ( + WorkspaceMember.objects.filter( + workspace__slug=slug, + member_id=user_id, + member__is_bot=True, + member__bot_type=BotTypeEnum.SERVICE, + ) + .select_related("member", "workspace") + .first() + ) + class ServiceAccountAPIEndpoint(BaseAPIView): """Admin-scoped endpoint for provisioning workspace service accounts. @@ -104,3 +162,203 @@ def post(self, request, slug): # The response carries the plaintext token, so keep it out of the # api_activity_logs table (APITokenLogMiddleware logs response bodies). return redact_response_body(Response(response.data, status=status.HTTP_201_CREATED)) + + +# Machine-readable error surfaced when the decommission guard rejects a non-service user. +NOT_A_SERVICE_ACCOUNT = {"error": "This user is not a service account.", "code": "NOT_A_SERVICE_ACCOUNT"} + + +class ServiceAccountDetailAPIEndpoint(BaseAPIView): + """Decommission a service account.""" + + permission_classes = [WorkspaceOwnerPermission] + + @extend_schema( + summary="Decommission a service account", + description=( + "Retire a service account: deactivate all its API tokens, remove its project and " + "workspace memberships, and deactivate the user. The user row is preserved so historical " + "attribution (created_by/updated_by) survives. Only valid for service accounts " + "(is_bot + bot_type=SERVICE); a human or other bot returns 400. Requires workspace admin." + ), + parameters=[WORKSPACE_SLUG_PARAMETER, SERVICE_ACCOUNT_ID_PARAMETER], + responses={ + 204: DELETED_RESPONSE, + 400: OpenApiResponse(description="The user is not a service account"), + 401: UNAUTHORIZED_RESPONSE, + 403: FORBIDDEN_RESPONSE, + 404: NOT_FOUND_RESPONSE, + }, + ) + def delete(self, request, slug, user_id): + # is_active is not filtered so a deactivated (but not yet decommissioned) + # account can still be retired; a decommissioned one is soft-deleted and + # excluded by the default manager (→ 404, idempotent). + member = ( + WorkspaceMember.objects.filter(workspace__slug=slug, member_id=user_id) + .select_related("member", "workspace") + .first() + ) + if member is None: + return Response(status=status.HTTP_404_NOT_FOUND) + + user = member.member + # Hard guard: only a genuine service account may be decommissioned this way. + if not (user.is_bot and user.bot_type == BotTypeEnum.SERVICE): + return Response(NOT_A_SERVICE_ACCOUNT, status=status.HTTP_400_BAD_REQUEST) + + decommission_service_account(user=user, workspace=member.workspace) + return Response(status=status.HTTP_204_NO_CONTENT) + + +class ServiceAccountTokenAPIEndpoint(BaseAPIView): + """List and mint a service account's API tokens.""" + + permission_classes = [WorkspaceOwnerPermission] + + @extend_schema( + summary="List service account tokens", + description=( + "List a service account's API tokens with metadata (label, timestamps, expiry, " + "is_active). The secret token value is always withheld. Requires workspace admin." + ), + parameters=[WORKSPACE_SLUG_PARAMETER, SERVICE_ACCOUNT_ID_PARAMETER, CURSOR_PARAMETER, PER_PAGE_PARAMETER], + responses={ + 200: create_paginated_response( + ServiceAccountTokenSerializer, + "PaginatedServiceAccountTokenResponse", + "Paginated list of service account tokens", + "Paginated Service Account Tokens", + ), + 401: UNAUTHORIZED_RESPONSE, + 403: FORBIDDEN_RESPONSE, + 404: NOT_FOUND_RESPONSE, + }, + ) + def get(self, request, slug, user_id): + member = get_service_account_member(slug, user_id) + if member is None: + return Response(status=status.HTTP_404_NOT_FOUND) + + queryset = APIToken.objects.filter(user_id=user_id, workspace=member.workspace).order_by("-created_at") + return self.paginate( + request=request, + queryset=queryset, + on_results=lambda tokens: ServiceAccountTokenSerializer(tokens, many=True).data, + ) + + @extend_schema( + summary="Mint a service account token", + description=( + "Mint an additional workspace-scoped bot token for the service account. The token value " + "is returned once and cannot be retrieved again. Requires workspace admin." + ), + request=OpenApiRequest(request=ServiceAccountTokenCreateSerializer), + parameters=[WORKSPACE_SLUG_PARAMETER, SERVICE_ACCOUNT_ID_PARAMETER], + responses={ + 201: OpenApiResponse(description="Token created", response=ServiceAccountTokenCreatedSerializer), + 401: UNAUTHORIZED_RESPONSE, + 403: FORBIDDEN_RESPONSE, + 404: NOT_FOUND_RESPONSE, + }, + ) + def post(self, request, slug, user_id): + member = get_service_account_member(slug, user_id) + if member is None: + return Response(status=status.HTTP_404_NOT_FOUND) + + serializer = ServiceAccountTokenCreateSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data + + token = mint_service_account_token( + user=member.member, + workspace=member.workspace, + label=data.get("label"), + description=data.get("description", ""), + expired_at=data.get("expired_at"), + ) + return redact_response_body(Response(_token_created_payload(token), status=status.HTTP_201_CREATED)) + + +class ServiceAccountTokenDetailAPIEndpoint(BaseAPIView): + """Revoke a single service account token.""" + + permission_classes = [WorkspaceOwnerPermission] + + @extend_schema( + summary="Revoke a service account token", + description=( + "Revoke (soft-delete) a single API token so authenticating with it fails. Requires workspace admin." + ), + parameters=[WORKSPACE_SLUG_PARAMETER, SERVICE_ACCOUNT_ID_PARAMETER, TOKEN_ID_PARAMETER], + responses={ + 204: DELETED_RESPONSE, + 401: UNAUTHORIZED_RESPONSE, + 403: FORBIDDEN_RESPONSE, + 404: NOT_FOUND_RESPONSE, + }, + ) + def delete(self, request, slug, user_id, token_id): + member = get_service_account_member(slug, user_id) + if member is None: + return Response(status=status.HTTP_404_NOT_FOUND) + + token = APIToken.objects.filter(id=token_id, user_id=user_id, workspace=member.workspace).first() + if token is None: + return Response(status=status.HTTP_404_NOT_FOUND) + + token.delete() + return Response(status=status.HTTP_204_NO_CONTENT) + + +class ServiceAccountTokenRotateAPIEndpoint(BaseAPIView): + """Rotate a service account token.""" + + permission_classes = [WorkspaceOwnerPermission] + + @extend_schema( + summary="Rotate a service account token", + description=( + "Atomically mint a replacement token and deactivate the old one. The replacement value " + "is returned once. Authenticating with the old token fails immediately. Requires workspace admin." + ), + request=OpenApiRequest(request=ServiceAccountTokenCreateSerializer), + parameters=[WORKSPACE_SLUG_PARAMETER, SERVICE_ACCOUNT_ID_PARAMETER, TOKEN_ID_PARAMETER], + responses={ + 201: OpenApiResponse( + description="Replacement token created", response=ServiceAccountTokenCreatedSerializer + ), + 401: UNAUTHORIZED_RESPONSE, + 403: FORBIDDEN_RESPONSE, + 404: NOT_FOUND_RESPONSE, + }, + ) + def post(self, request, slug, user_id, token_id): + member = get_service_account_member(slug, user_id) + if member is None: + return Response(status=status.HTTP_404_NOT_FOUND) + + token = APIToken.objects.filter(id=token_id, user_id=user_id, workspace=member.workspace).first() + if token is None: + return Response(status=status.HTTP_404_NOT_FOUND) + + serializer = ServiceAccountTokenCreateSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + + replacement = rotate_service_account_token(token=token, expired_at=serializer.validated_data.get("expired_at")) + return redact_response_body(Response(_token_created_payload(replacement), status=status.HTTP_201_CREATED)) + + +def _token_created_payload(token): + """Serialize a freshly minted/rotated token, including its value (shown once).""" + return ServiceAccountTokenCreatedSerializer( + { + "id": token.id, + "label": token.label, + "is_active": token.is_active, + "created_at": token.created_at, + "expired_at": token.expired_at, + "token": token.token, + } + ).data diff --git a/apps/api/plane/tests/contract/api/test_service_account.py b/apps/api/plane/tests/contract/api/test_service_account.py index 15b368e787e..ebd1ce485f8 100644 --- a/apps/api/plane/tests/contract/api/test_service_account.py +++ b/apps/api/plane/tests/contract/api/test_service_account.py @@ -21,8 +21,9 @@ from rest_framework import status from rest_framework.test import APIClient -from plane.db.models import APIToken, BotTypeEnum, Project, User, Workspace, WorkspaceMember +from plane.db.models import APIToken, BotTypeEnum, Project, ProjectMember, User, Workspace, WorkspaceMember from plane.db.models.api import generate_token +from plane.utils.service_account import create_service_account USERS_ME_URL = "/api/v1/users/me/" @@ -33,6 +34,24 @@ def _client_for_token(token: str) -> APIClient: return client +@pytest.fixture(autouse=True) +def _reset_api_throttle(): + # ApiKeyRateThrottle stores per-token request counts in the shared cache and + # is not reset between tests. This suite makes many API-key calls, so clear it + # per test to avoid cross-test 429s (and to avoid inflating the bucket for + # other tests that reuse the same api_key_client token). + from django.core.cache import cache + + cache.clear() + yield + + +@pytest.fixture +def service_account(db, workspace): + """A provisioned service account (user + admin membership + one bot token).""" + return create_service_account(workspace=workspace, name="Reconcile Bot", role="admin", username="reconcile-bot") + + def _service_account_for(workspace: Workspace) -> WorkspaceMember: return WorkspaceMember.objects.select_related("member").get( workspace=workspace, @@ -430,3 +449,266 @@ def test_duplicate_username_rolls_back_atomically(self, workspace): assert User.objects.count() == users_before assert WorkspaceMember.objects.count() == members_before assert APIToken.objects.count() == tokens_before + + +@pytest.mark.contract +class TestServiceAccountTokenLifecycle: + """List, mint, rotate, and revoke a service account's API tokens.""" + + def _tokens_url(self, slug, user_id): + return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/" + + def _token_url(self, slug, user_id, token_id): + return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/{token_id}/" + + def _rotate_url(self, slug, user_id, token_id): + return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/{token_id}/rotate/" + + @pytest.mark.django_db + def test_list_tokens_masks_value(self, api_key_client, workspace, service_account): + response = api_key_client.get(self._tokens_url(workspace.slug, service_account.user.id)) + + assert response.status_code == status.HTTP_200_OK, response.data + results = response.data["results"] + assert len(results) == 1 + entry = results[0] + # Metadata is present; the secret value is always withheld. + assert entry["label"] == "Reconcile Bot" + assert entry["is_active"] is True + assert "token" not in entry + # And the plaintext value appears nowhere in the serialized body. + assert service_account.token not in str(response.data) + + @pytest.mark.django_db + def test_mint_token_returns_value_once_and_authenticates(self, api_key_client, workspace, service_account): + response = api_key_client.post( + self._tokens_url(workspace.slug, service_account.user.id), + {"label": "second-key"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["label"] == "second-key" + minted = response.data["token"] + assert minted.startswith("plane_api_") + # The new token authenticates as the same service account. + me = _client_for_token(minted).get(USERS_ME_URL) + assert me.status_code == status.HTTP_200_OK + assert str(me.data["id"]) == str(service_account.user.id) + # The account now has two tokens. + assert APIToken.objects.filter(user=service_account.user).count() == 2 + + @pytest.mark.django_db + def test_mint_token_not_persisted_in_request_log(self, api_key_client, workspace, service_account): + with patch("plane.middleware.logger.process_logs") as process_logs: + response = api_key_client.post( + self._tokens_url(workspace.slug, service_account.user.id), + {"label": "secret-key"}, + format="json", + ) + assert response.status_code == status.HTTP_201_CREATED, response.data + minted = response.data["token"] + log_data = process_logs.delay.call_args.kwargs["log_data"] + + assert log_data["response_body"] == "[REDACTED]" + assert minted not in (log_data["response_body"] or "") + + @pytest.mark.django_db + def test_mint_token_with_expiry(self, api_key_client, workspace, service_account): + from django.utils import timezone + from datetime import timedelta + + expires = (timezone.now() + timedelta(days=7)).isoformat() + response = api_key_client.post( + self._tokens_url(workspace.slug, service_account.user.id), + {"label": "expiring", "expired_at": expires}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["expired_at"] is not None + + @pytest.mark.django_db + def test_rotate_invalidates_old_token(self, api_key_client, workspace, service_account): + old_value = service_account.token + old_id = service_account.api_token.id + + # The original token works before rotation. + assert _client_for_token(old_value).get(USERS_ME_URL).status_code == status.HTTP_200_OK + + response = api_key_client.post(self._rotate_url(workspace.slug, service_account.user.id, old_id)) + assert response.status_code == status.HTTP_201_CREATED, response.data + new_value = response.data["token"] + assert new_value != old_value + + # Old token no longer authenticates; the replacement does. + assert _client_for_token(old_value).get(USERS_ME_URL).status_code in ( + status.HTTP_401_UNAUTHORIZED, + status.HTTP_403_FORBIDDEN, + ) + assert _client_for_token(new_value).get(USERS_ME_URL).status_code == status.HTTP_200_OK + + # The old token row is retained but deactivated. + service_account.api_token.refresh_from_db() + assert service_account.api_token.is_active is False + + @pytest.mark.django_db + def test_rotate_not_persisted_in_request_log(self, api_key_client, workspace, service_account): + with patch("plane.middleware.logger.process_logs") as process_logs: + response = api_key_client.post( + self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id) + ) + assert response.status_code == status.HTTP_201_CREATED, response.data + new_value = response.data["token"] + log_data = process_logs.delay.call_args.kwargs["log_data"] + + assert log_data["response_body"] == "[REDACTED]" + assert new_value not in (log_data["response_body"] or "") + + @pytest.mark.django_db + def test_revoke_token_invalidates_it(self, api_key_client, workspace, service_account): + old_value = service_account.token + response = api_key_client.delete( + self._token_url(workspace.slug, service_account.user.id, service_account.api_token.id) + ) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Auth with the revoked token fails, and it no longer appears in the list. + assert _client_for_token(old_value).get(USERS_ME_URL).status_code in ( + status.HTTP_401_UNAUTHORIZED, + status.HTTP_403_FORBIDDEN, + ) + listing = api_key_client.get(self._tokens_url(workspace.slug, service_account.user.id)) + assert listing.data["results"] == [] + + @pytest.mark.django_db + def test_token_endpoints_404_for_non_service_user(self, api_key_client, workspace, create_user): + import uuid + + # create_user is a human admin, not a service account — no token endpoint applies. + base = self._tokens_url(workspace.slug, create_user.id) + assert api_key_client.get(base).status_code == status.HTTP_404_NOT_FOUND + assert api_key_client.post(base, {"label": "x"}, format="json").status_code == status.HTTP_404_NOT_FOUND + tok = self._token_url(workspace.slug, create_user.id, uuid.uuid4()) + assert api_key_client.delete(tok).status_code == status.HTTP_404_NOT_FOUND + rotate = self._rotate_url(workspace.slug, create_user.id, uuid.uuid4()) + assert api_key_client.post(rotate).status_code == status.HTTP_404_NOT_FOUND + + @pytest.mark.django_db + def test_cross_workspace_isolation(self, api_key_client, workspace, service_account): + # A service account in another workspace must be unreachable via THIS + # workspace's slug, even by this workspace's admin (api_key_client). + owner = User.objects.create(username="other_owner", email="oo@plane.so") + other_ws = Workspace.objects.create(name="Other", owner=owner, slug="other-ws") + other = create_service_account(workspace=other_ws, name="Other Bot", username="other-bot") + + uid = other.user.id + assert api_key_client.get(self._tokens_url(workspace.slug, uid)).status_code == status.HTTP_404_NOT_FOUND + rotate = self._rotate_url(workspace.slug, uid, other.api_token.id) + assert api_key_client.post(rotate).status_code == status.HTTP_404_NOT_FOUND + revoke = self._token_url(workspace.slug, uid, other.api_token.id) + assert api_key_client.delete(revoke).status_code == status.HTTP_404_NOT_FOUND + decommission = f"/api/v1/workspaces/{workspace.slug}/service-accounts/{uid}/" + assert api_key_client.delete(decommission).status_code == status.HTTP_404_NOT_FOUND + + # The other workspace's token is untouched and still authenticates. + assert _client_for_token(other.token).get(USERS_ME_URL).status_code == status.HTTP_200_OK + + @pytest.mark.django_db + def test_non_admin_cannot_manage_tokens(self, api_client, workspace, service_account): + guest = User.objects.create(username="guest_tok", email="guesttok@plane.so") + WorkspaceMember.objects.create(workspace=workspace, member=guest, role=5, is_active=True) + guest_token = APIToken.objects.create(user=guest, label="g", token=generate_token()) + client = _client_for_token(guest_token.token) + + url = self._tokens_url(workspace.slug, service_account.user.id) + token_url = self._token_url(workspace.slug, service_account.user.id, service_account.api_token.id) + assert client.get(url).status_code == status.HTTP_403_FORBIDDEN + assert client.post(url, {"label": "x"}, format="json").status_code == status.HTTP_403_FORBIDDEN + assert client.delete(token_url).status_code == status.HTTP_403_FORBIDDEN + + +@pytest.mark.contract +class TestServiceAccountDecommission: + """DELETE /service-accounts/{user_id}/ — decommission with cascade + guards.""" + + def _url(self, slug, user_id): + return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/" + + @pytest.mark.django_db + def test_decommission_cascade(self, api_key_client, workspace, service_account): + user = service_account.user + # Give the account a project membership to prove the cascade removes it. + # Attribute the project to the service account explicitly (created_by is + # otherwise set from the request user via crum, which is absent here). + project = Project(name="P", identifier="P1", workspace=workspace) + project.save(created_by_id=user.id) + ProjectMember.objects.create(project=project, member=user, role=20, is_active=True) + old_token = service_account.token + + response = api_key_client.delete(self._url(workspace.slug, user.id)) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # Memberships removed (soft-deleted → excluded from the default manager). + assert not WorkspaceMember.objects.filter(member=user, workspace=workspace).exists() + assert not ProjectMember.objects.filter(member=user, workspace=workspace).exists() + # Tokens deactivated and the account can no longer authenticate. + assert not APIToken.objects.filter(user=user, is_active=True).exists() + assert _client_for_token(old_token).get(USERS_ME_URL).status_code in ( + status.HTTP_401_UNAUTHORIZED, + status.HTTP_403_FORBIDDEN, + ) + # The user row survives (deactivated) so historical attribution is intact. + user.refresh_from_db() + assert user.is_active is False + assert Project.objects.get(id=project.id).created_by_id == user.id + + @pytest.mark.django_db + def test_deactivated_membership_can_still_be_decommissioned(self, api_key_client, workspace, service_account): + # A merely-deactivated membership must not block management: the tokens + # still authenticate (auth ignores membership), so the admin must still + # be able to list and decommission the account. + WorkspaceMember.objects.filter(member=service_account.user, workspace=workspace).update(is_active=False) + assert _client_for_token(service_account.token).get(USERS_ME_URL).status_code == status.HTTP_200_OK + + tokens_url = f"/api/v1/workspaces/{workspace.slug}/service-accounts/{service_account.user.id}/tokens/" + assert api_key_client.get(tokens_url).status_code == status.HTTP_200_OK + + response = api_key_client.delete(self._url(workspace.slug, service_account.user.id)) + assert response.status_code == status.HTTP_204_NO_CONTENT + assert not APIToken.objects.filter(user=service_account.user, is_active=True).exists() + + @pytest.mark.django_db + def test_decommission_rejects_human_user(self, api_key_client, workspace, create_user): + response = api_key_client.delete(self._url(workspace.slug, create_user.id)) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data["code"] == "NOT_A_SERVICE_ACCOUNT" + # The human admin remains an active member. + assert WorkspaceMember.objects.filter(member=create_user, workspace=workspace, is_active=True).exists() + + @pytest.mark.django_db + def test_decommission_rejects_other_bot_type(self, api_key_client, workspace): + bot = User.objects.create( + username="seed_bot", email="seed@plane.so", is_bot=True, bot_type=BotTypeEnum.WORKSPACE_SEED + ) + WorkspaceMember.objects.create(workspace=workspace, member=bot, role=20, is_active=True) + + response = api_key_client.delete(self._url(workspace.slug, bot.id)) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data["code"] == "NOT_A_SERVICE_ACCOUNT" + + @pytest.mark.django_db + def test_decommission_unknown_user_is_404(self, api_key_client, workspace): + import uuid + + response = api_key_client.delete(self._url(workspace.slug, uuid.uuid4())) + assert response.status_code == status.HTTP_404_NOT_FOUND + + @pytest.mark.django_db + def test_non_admin_cannot_decommission(self, api_client, workspace, service_account): + guest = User.objects.create(username="guest_dec", email="guestdec@plane.so") + WorkspaceMember.objects.create(workspace=workspace, member=guest, role=5, is_active=True) + guest_token = APIToken.objects.create(user=guest, label="g", token=generate_token()) + + response = _client_for_token(guest_token.token).delete(self._url(workspace.slug, service_account.user.id)) + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/apps/api/plane/utils/service_account.py b/apps/api/plane/utils/service_account.py index f581040c805..3b3265ae5fc 100644 --- a/apps/api/plane/utils/service_account.py +++ b/apps/api/plane/utils/service_account.py @@ -23,7 +23,7 @@ from django.db import transaction # Module imports -from plane.db.models import APIToken, BotTypeEnum, User, Workspace, WorkspaceMember +from plane.db.models import APIToken, BotTypeEnum, ProjectMember, User, Workspace, WorkspaceMember # Human-friendly role name -> WorkspaceMember.role value. # Mirrors ROLE_CHOICES in plane/db/models/workspace.py ((20, Admin), (15, Member), (5, Guest)). @@ -144,14 +144,74 @@ def create_service_account( company_role="", ) - api_token = APIToken.objects.create( - label=name, - description=description or f"Service account token for {name}", + api_token = mint_service_account_token( user=user, - # 1 == Bot (see APIToken.user_type choices). - user_type=1, workspace=workspace, - is_service=True, + label=name, + description=description or f"Service account token for {name}", ) return ServiceAccount(user=user, member=member, api_token=api_token) + + +def mint_service_account_token(*, user, workspace, label=None, description="", expired_at=None) -> APIToken: + """Mint an additional workspace-scoped bot token for a service account. + + ``is_service=True`` + ``user_type=Bot`` mark it as a machine token; when + ``label`` is omitted the model's default (a random handle) is used. + """ + fields = { + "description": description, + "user": user, + # 1 == Bot (see APIToken.user_type choices). + "user_type": 1, + "workspace": workspace, + "is_service": True, + "expired_at": expired_at, + } + if label: + fields["label"] = label + return APIToken.objects.create(**fields) + + +@transaction.atomic +def rotate_service_account_token(*, token: APIToken, expired_at=None) -> APIToken: + """Atomically mint a replacement token and deactivate the old one. + + The replacement inherits the old token's label/description/workspace so the + rotation is transparent to consumers; ``expired_at`` may be set anew. The old + token is deactivated (``is_active=False``) so authenticating with it fails + immediately. + """ + replacement = mint_service_account_token( + user=token.user, + workspace=token.workspace, + label=token.label, + description=token.description, + expired_at=expired_at, + ) + token.is_active = False + token.save(update_fields=["is_active", "updated_at"]) + return replacement + + +@transaction.atomic +def decommission_service_account(*, user: User, workspace: Workspace) -> None: + """Retire a service account: revoke access and remove its memberships. + + Deactivates every token, removes (soft-deletes) the account's ProjectMember + and WorkspaceMember rows, and deactivates the User. The User row is kept (not + deleted) so historical attribution (``created_by``/``updated_by`` on + everything it created) survives; ``is_active=False`` alone revokes API access + (``APIKeyAuthentication`` requires ``user__is_active``). + + The token/user deactivation is global while membership removal is scoped to + ``workspace``; this is consistent because a service account belongs to + exactly one workspace (it is created for one workspace and there is no API to + add it to another), so "retire the user" == "retire it in this workspace". + """ + APIToken.objects.filter(user=user).update(is_active=False) + ProjectMember.objects.filter(member=user, workspace=workspace).delete() + WorkspaceMember.objects.filter(member=user, workspace=workspace).delete() + user.is_active = False + user.save(update_fields=["is_active"]) diff --git a/docs/service-accounts.md b/docs/service-accounts.md index a0475efb8ea..b1b7efd26ea 100644 --- a/docs/service-accounts.md +++ b/docs/service-accounts.md @@ -147,6 +147,62 @@ identity instead of the server-generated defaults: On the management command, a taken `--username` (or `--email`) fails with a clear `CommandError` instead of a raw traceback. +## Managing tokens + +An external reconcile/rotation loop can list, mint, rotate, and revoke a service +account's tokens over the API. All endpoints require the caller to be a workspace +**admin** (`WorkspaceOwnerPermission`) and are scoped to a service account +(`{user_id}` must be a `SERVICE` bot in `{slug}`, else `404`). + +**List** — `GET /api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/` + +Cursor-paginated. Returns each token's metadata (`label`, `is_active`, +`created_at`, `expired_at`, `last_used`, …); the **secret value is always +withheld**. Revoked tokens are omitted; a rotated-away token remains listed with +`is_active: false`. + +**Mint** — `POST .../tokens/` + +```http +POST /api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/ +X-Api-Key: +Content-Type: application/json + +{ "label": "ci-2025", "expired_at": "2026-01-01T00:00:00Z" } +``` + +`label`, `description`, and `expired_at` are optional. Response `201` returns the +new token value **once**: + +```json +{ "id": "…", "label": "ci-2025", "is_active": true, "created_at": "…", "expired_at": "…", "token": "plane_api_…" } +``` + +**Rotate** — `POST .../tokens/{token_id}/rotate/` + +Atomically mints a replacement (optional `expired_at` in the body, returned once) +and deactivates the old token, so authenticating with the old value fails +immediately. The old token stays listed as `is_active: false` for audit. + +**Revoke** — `DELETE .../tokens/{token_id}/` + +Revokes a single token (`204`); authenticating with it then fails. + +Mint and rotate responses carry the plaintext token, so — like the create +endpoint — their bodies are redacted from the `api_activity_logs` request log. + +## Decommissioning + +`DELETE /api/v1/workspaces/{slug}/service-accounts/{user_id}/` retires an account: +it deactivates **all** its tokens, removes its `ProjectMember` and +`WorkspaceMember` rows, and deactivates the `User` (`is_active=False`). The user +row is **kept**, so historical attribution (`created_by`/`updated_by` on +everything it created) survives, and `is_active=False` alone revokes API access. + +The operation is hard-guarded: it only applies to a service account +(`is_bot=True` **and** `bot_type=SERVICE`). Attempting it on a human or any other +bot returns `400 {"error": ..., "code": "NOT_A_SERVICE_ACCOUNT"}`. + ## Notes - Tokens are stored verbatim and matched exactly at authentication time; there is From 0254d025b3e646c603fd0a5b085c197019921e1e Mon Sep 17 00:00:00 2001 From: Seena Fallah Date: Sat, 25 Jul 2026 18:48:54 +0000 Subject: [PATCH 4/5] fix(api): address PR review feedback on service account tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rotation expiry semantics (CodeRabbit): - Rotating a token now INHERITS the source token's expired_at when the caller omits it, instead of minting a never-expiring replacement. An explicit null still clears the expiry; a supplied timestamp must be in the future. - The rotate body uses an OmittableDateTimeField so an empty form/multipart value reads as 'omitted' (inherit), not as null — closing a hole where a form-encoded 'expired_at=' produced an immortal token. - Rotation is restricted to ACTIVE tokens via a race-safe conditional UPDATE (409 TOKEN_NOT_ACTIVE); inheriting an already-elapsed expiry is refused (400 SOURCE_TOKEN_EXPIRY_ELAPSED). Mint and rotate reject a past expired_at. - The deactivation UPDATE writes updated_by unconditionally (None when there is no actor), matching BaseModel.save(). - Token-rotation exceptions carry code + status_code and are mapped in one place. Copilot: revoke/rotate are already scoped by construction (user_id is a SERVICE bot member of the workspace, tokens are workspace-scoped is_service bot tokens); the one real gap (rotating an inactive token) is closed above. is_service is deliberately NOT added to the revoke lookup — it would make a legacy non-service token unrevokable (fail-open). Docstring Coverage gate: document every callable in the new files (handlers, command, helpers, and all contract/unit tests). Docs: correct the rotate section (remaining-lifetime inheritance, the three expiry cases, 409 vs 404, future-timestamp requirement). Tests: rotate expiry inheritance / explicit-null / explicit-future / past rejected / form-encoded-empty-inherits / inactive-source-409 / elapsed-source-400. - Cover minting a token with no label (generated default) — CodeRabbit. - Do not conflate an omitted vs blank description: `create_service_account` now defaults only when description is None, so an explicit "" is preserved (serializer/CLI/view thread None as the "omitted" sentinel) — Copilot. - Scope decommission token deactivation to the given workspace (tokens are workspace-scoped) so retiring an account in one workspace never touches its tokens elsewhere — Copilot. - Normalize whitespace-only --email/--username/--display-name in the command (strip; blank -> None) so " " cannot create an all-whitespace identity — Copilot. - Document 401/403/404 on the create endpoint's OpenAPI schema — Copilot. - Docs: use a future expired_at in the mint example so it stays valid — Copilot. - Scope the token list/revoke/rotate queries to is_service tokens so the service-account token API only manages service tokens — Copilot. - Validate identity-field max lengths in the command (mirroring the serializer) so an over-long value fails with CommandError, not a raw DB DataError — Copilot. - resolve_service_account_role: raise the role ValueError `from None` so the user-facing message isn't buried under a KeyError chain — Copilot. - Docs: phrase the synthetic email as a default (--email can override it) and clarify that bots are hidden from the web-app member list but listed by the public members API — Copilot. - decommission token deactivation now also sets updated_at/updated_by (like rotate) via a shared _audit_fields() helper: QuerySet.update() bypasses save(), so without it a decommissioned token kept a stale updated_at and no actor attribution. Tests assert both the rotate and decommission paths stamp updated_at and attribute the deactivation to the acting admin — Copilot. Signed-off-by: Seena Fallah --- apps/api/plane/api/serializers/__init__.py | 1 + .../plane/api/serializers/service_account.py | 65 +++- apps/api/plane/api/views/service_account.py | 68 +++- .../commands/create_service_account.py | 35 +- .../contract/api/test_service_account.py | 305 +++++++++++++++++- .../tests/unit/middleware/test_logger.py | 2 + .../tests/unit/utils/test_service_account.py | 13 + apps/api/plane/utils/service_account.py | 123 ++++++- docs/service-accounts.md | 57 +++- 9 files changed, 617 insertions(+), 52 deletions(-) diff --git a/apps/api/plane/api/serializers/__init__.py b/apps/api/plane/api/serializers/__init__.py index be5694d5bf5..0c7acfe8ece 100644 --- a/apps/api/plane/api/serializers/__init__.py +++ b/apps/api/plane/api/serializers/__init__.py @@ -72,6 +72,7 @@ ServiceAccountSerializer, ServiceAccountTokenCreateSerializer, ServiceAccountTokenCreatedSerializer, + ServiceAccountTokenRotateSerializer, ServiceAccountTokenSerializer, ) from .sticky import StickySerializer diff --git a/apps/api/plane/api/serializers/service_account.py b/apps/api/plane/api/serializers/service_account.py index 2939a7f7d70..cee80602486 100644 --- a/apps/api/plane/api/serializers/service_account.py +++ b/apps/api/plane/api/serializers/service_account.py @@ -2,8 +2,13 @@ # SPDX-License-Identifier: AGPL-3.0-only # See the LICENSE file for details. +# Django imports +from django.utils import timezone + # Third party imports from rest_framework import serializers +from rest_framework.fields import empty +from rest_framework.utils import html # Module imports from plane.api.serializers.base import BaseSerializer @@ -11,6 +16,30 @@ from plane.utils.service_account import DEFAULT_SERVICE_ACCOUNT_ROLE, SERVICE_ACCOUNT_ROLES +class OmittableDateTimeField(serializers.DateTimeField): + """A DateTimeField where an empty HTML form value means "omitted", not null. + + DRF maps an empty string from a form/multipart body to ``None`` when + ``allow_null`` is set, which is indistinguishable from an explicit JSON + ``null``. Rotation needs that distinction (omitted → inherit the source + token's expiry; null → never expire), so an empty HTML value is treated as + absent instead — the key is then omitted from ``validated_data``. + """ + + def get_value(self, dictionary): + """Return ``empty`` for a blank HTML value so it reads as omitted, not null.""" + if html.is_html_input(dictionary) and dictionary.get(self.field_name, "") == "": + return empty + return super().get_value(dictionary) + + +def _validate_future_expiry(value): + """Reject a non-null expiry that is not in the future (a DOA credential).""" + if value is not None and value <= timezone.now(): + raise serializers.ValidationError("expired_at must be in the future.") + return value + + class ServiceAccountCreateSerializer(serializers.Serializer): """Request body for provisioning a workspace service account.""" @@ -41,11 +70,13 @@ class ServiceAccountCreateSerializer(serializers.Serializer): max_length=255, help_text="Optional display name shown in the members UI; falls back to name when omitted", ) + # No `default` so an omitted description is absent from validated_data (→ the + # helper applies its generated default), while an explicit "" is preserved as + # a deliberate empty description. description = serializers.CharField( required=False, allow_blank=True, - default="", - help_text="Optional description stored on the API token", + help_text="Optional description stored on the API token; a default is generated when omitted", ) @@ -89,7 +120,7 @@ class Meta: class ServiceAccountTokenCreateSerializer(serializers.Serializer): - """Request body for minting or rotating a service account token.""" + """Request body for minting a service account token.""" label = serializers.CharField( required=False, allow_blank=True, max_length=255, help_text="Optional human-readable token label" @@ -98,7 +129,33 @@ class ServiceAccountTokenCreateSerializer(serializers.Serializer): required=False, allow_blank=True, default="", help_text="Optional token description" ) expired_at = serializers.DateTimeField( - required=False, allow_null=True, default=None, help_text="Optional expiry; the token never expires when omitted" + required=False, + allow_null=True, + validators=[_validate_future_expiry], + help_text="Optional expiry; the token never expires when omitted", + ) + + +class ServiceAccountTokenRotateSerializer(serializers.Serializer): + """Request body for rotating a service account token. + + Only the expiry is caller-settable — the replacement inherits the source + token's label and description. ``expired_at`` uses :class:`OmittableDateTimeField` + and declares no ``default``, so DRF omits the key from ``validated_data`` when + the caller omits the field (or sends an empty form value). That lets rotation + tell "not supplied" (inherit the source token's expiry) apart from an explicit + ``null`` (never expire). + """ + + expired_at = OmittableDateTimeField( + required=False, + allow_null=True, + validators=[_validate_future_expiry], + help_text=( + "Expiry for the replacement token. Omit to inherit the source token's expiry; " + "pass null to deliberately make the replacement never expire. A supplied timestamp " + "must be in the future." + ), ) diff --git a/apps/api/plane/api/views/service_account.py b/apps/api/plane/api/views/service_account.py index 23e49f8acca..33ae0ccec28 100644 --- a/apps/api/plane/api/views/service_account.py +++ b/apps/api/plane/api/views/service_account.py @@ -18,6 +18,7 @@ ServiceAccountSerializer, ServiceAccountTokenCreateSerializer, ServiceAccountTokenCreatedSerializer, + ServiceAccountTokenRotateSerializer, ServiceAccountTokenSerializer, ) from plane.db.models import APIToken, BotTypeEnum, User, Workspace, WorkspaceMember @@ -36,6 +37,7 @@ decommission_service_account, mint_service_account_token, rotate_service_account_token, + ServiceAccountTokenError, ) # Machine-readable error surfaced when a caller-chosen username is already taken. @@ -110,10 +112,18 @@ class ServiceAccountAPIEndpoint(BaseAPIView): description="Service account created", response=ServiceAccountSerializer, ), + 401: UNAUTHORIZED_RESPONSE, + 403: FORBIDDEN_RESPONSE, + 404: NOT_FOUND_RESPONSE, 409: OpenApiResponse(description="A user with the requested username already exists"), }, ) def post(self, request, slug): + """Create a service account + + Provision a machine identity in the workspace and mint its first API + token. The token value is returned once and cannot be retrieved again. + """ workspace = Workspace.objects.get(slug=slug) serializer = ServiceAccountCreateSerializer(data=request.data) @@ -132,7 +142,8 @@ def post(self, request, slug): workspace=workspace, name=data["name"], role=data["role"], - description=data.get("description", ""), + # None (omitted) → generated default; an explicit "" is preserved. + description=data.get("description"), username=username, display_name=data.get("display_name"), ) @@ -191,6 +202,12 @@ class ServiceAccountDetailAPIEndpoint(BaseAPIView): }, ) def delete(self, request, slug, user_id): + """Decommission a service account + + Deactivate every token, remove the account's project and workspace + memberships, and deactivate the user. The user row is preserved so + historical attribution survives. + """ # is_active is not filtered so a deactivated (but not yet decommissioned) # account can still be retired; a decommissioned one is soft-deleted and # excluded by the default manager (→ 404, idempotent). @@ -236,11 +253,18 @@ class ServiceAccountTokenAPIEndpoint(BaseAPIView): }, ) def get(self, request, slug, user_id): + """List service account tokens + + Retrieve the account's API tokens with their metadata. The secret token + value is always withheld. + """ member = get_service_account_member(slug, user_id) if member is None: return Response(status=status.HTTP_404_NOT_FOUND) - queryset = APIToken.objects.filter(user_id=user_id, workspace=member.workspace).order_by("-created_at") + queryset = APIToken.objects.filter(user_id=user_id, workspace=member.workspace, is_service=True).order_by( + "-created_at" + ) return self.paginate( request=request, queryset=queryset, @@ -263,6 +287,11 @@ def get(self, request, slug, user_id): }, ) def post(self, request, slug, user_id): + """Mint a service account token + + Create an additional workspace-scoped bot token for the account. The + token value is returned once and cannot be retrieved again. + """ member = get_service_account_member(slug, user_id) if member is None: return Response(status=status.HTTP_404_NOT_FOUND) @@ -300,11 +329,17 @@ class ServiceAccountTokenDetailAPIEndpoint(BaseAPIView): }, ) def delete(self, request, slug, user_id, token_id): + """Revoke a service account token + + Revoke a single API token so authenticating with it fails immediately. + """ member = get_service_account_member(slug, user_id) if member is None: return Response(status=status.HTTP_404_NOT_FOUND) - token = APIToken.objects.filter(id=token_id, user_id=user_id, workspace=member.workspace).first() + token = APIToken.objects.filter( + id=token_id, user_id=user_id, workspace=member.workspace, is_service=True + ).first() if token is None: return Response(status=status.HTTP_404_NOT_FOUND) @@ -323,30 +358,49 @@ class ServiceAccountTokenRotateAPIEndpoint(BaseAPIView): "Atomically mint a replacement token and deactivate the old one. The replacement value " "is returned once. Authenticating with the old token fails immediately. Requires workspace admin." ), - request=OpenApiRequest(request=ServiceAccountTokenCreateSerializer), + request=OpenApiRequest(request=ServiceAccountTokenRotateSerializer), parameters=[WORKSPACE_SLUG_PARAMETER, SERVICE_ACCOUNT_ID_PARAMETER, TOKEN_ID_PARAMETER], responses={ 201: OpenApiResponse( description="Replacement token created", response=ServiceAccountTokenCreatedSerializer ), + 400: OpenApiResponse(description="Invalid request body, or the source token's expiry has already elapsed"), 401: UNAUTHORIZED_RESPONSE, 403: FORBIDDEN_RESPONSE, 404: NOT_FOUND_RESPONSE, + 409: OpenApiResponse(description="The token is not active and cannot be rotated"), }, ) def post(self, request, slug, user_id, token_id): + """Rotate a service account token + + Atomically mint a replacement token and deactivate the old one. The + replacement value is returned once and inherits the source token's + expiry unless the request overrides it. + """ member = get_service_account_member(slug, user_id) if member is None: return Response(status=status.HTTP_404_NOT_FOUND) - token = APIToken.objects.filter(id=token_id, user_id=user_id, workspace=member.workspace).first() + token = APIToken.objects.filter( + id=token_id, user_id=user_id, workspace=member.workspace, is_service=True + ).first() if token is None: return Response(status=status.HTTP_404_NOT_FOUND) - serializer = ServiceAccountTokenCreateSerializer(data=request.data) + serializer = ServiceAccountTokenRotateSerializer(data=request.data) serializer.is_valid(raise_exception=True) + data = serializer.validated_data + + # Only forward expired_at when the caller actually sent the key, so the + # helper can tell "inherit the source expiry" from an explicit null. + overrides = {"expired_at": data["expired_at"]} if "expired_at" in data else {} + + try: + replacement = rotate_service_account_token(token=token, **overrides) + except ServiceAccountTokenError as exc: + return Response({"error": str(exc), "code": exc.code}, status=exc.status_code) - replacement = rotate_service_account_token(token=token, expired_at=serializer.validated_data.get("expired_at")) return redact_response_body(Response(_token_created_payload(replacement), status=status.HTTP_201_CREATED)) diff --git a/apps/api/plane/db/management/commands/create_service_account.py b/apps/api/plane/db/management/commands/create_service_account.py index 2c49db50646..b95b4ab66a5 100644 --- a/apps/api/plane/db/management/commands/create_service_account.py +++ b/apps/api/plane/db/management/commands/create_service_account.py @@ -23,6 +23,7 @@ class Command(BaseCommand): ) def add_arguments(self, parser): + """Register the command's arguments on the parser.""" parser.add_argument("--workspace", type=str, required=True, help="Workspace slug") parser.add_argument("--name", type=str, required=True, help="Name for the service account (token label)") parser.add_argument( @@ -51,9 +52,15 @@ def add_arguments(self, parser): default=None, help="Optional email; a unique synthetic one is generated when omitted", ) - parser.add_argument("--description", type=str, default="", help="Optional token description") + parser.add_argument( + "--description", + type=str, + default=None, + help="Optional token description; a default is generated when omitted", + ) def handle(self, *args, **options): + """Create the service account and print its details and API token.""" if not options["name"].strip(): raise CommandError("--name must not be empty") @@ -61,11 +68,29 @@ def handle(self, *args, **options): if workspace is None: raise CommandError(f"Workspace with slug '{options['workspace']}' does not exist") - email = options.get("email") + # Normalize identity options: strip surrounding whitespace and treat a + # blank (or whitespace-only) value as omitted, so " " cannot slip past the + # synthetic-fallback checks and create an all-whitespace username/email. + email = (options.get("email") or "").strip() or None + username = (options.get("username") or "").strip() or None + display_name = (options.get("display_name") or "").strip() or None + + # Mirror the serializer's max_length checks so an over-long value fails + # with a clear CommandError instead of a raw DB DataError. Lengths are + # read from the model fields rather than hardcoded. + length_limits = { + "--name": (options["name"], User._meta.get_field("first_name").max_length), + "--username": (username, User._meta.get_field("username").max_length), + "--display-name": (display_name, User._meta.get_field("display_name").max_length), + "--email": (email, User._meta.get_field("email").max_length), + } + for flag, (value, limit) in length_limits.items(): + if value and len(value) > limit: + raise CommandError(f"{flag} must be at most {limit} characters") + if email and User.objects.filter(email=email).exists(): raise CommandError(f"A user with email '{email}' already exists") - username = options.get("username") if username and User.objects.filter(username=username).exists(): raise CommandError(f"A user with username '{username}' already exists") @@ -74,10 +99,10 @@ def handle(self, *args, **options): workspace=workspace, name=options["name"], role=options["role"], - email=options["email"], + email=email, description=options["description"], username=username, - display_name=options.get("display_name"), + display_name=display_name, ) except IntegrityError as exc: # A concurrent insert (email/username race that slipped past the checks diff --git a/apps/api/plane/tests/contract/api/test_service_account.py b/apps/api/plane/tests/contract/api/test_service_account.py index ebd1ce485f8..76db085ef2a 100644 --- a/apps/api/plane/tests/contract/api/test_service_account.py +++ b/apps/api/plane/tests/contract/api/test_service_account.py @@ -14,10 +14,12 @@ - the admin-scoped ``POST /api/v1/workspaces/{slug}/service-accounts/`` endpoint. """ +from datetime import timedelta from unittest.mock import patch import pytest from django.core.management import call_command +from django.utils import timezone from rest_framework import status from rest_framework.test import APIClient @@ -29,6 +31,7 @@ def _client_for_token(token: str) -> APIClient: + """Return an API client authenticated with the given API token""" client = APIClient() client.credentials(HTTP_X_API_KEY=token) return client @@ -36,6 +39,7 @@ def _client_for_token(token: str) -> APIClient: @pytest.fixture(autouse=True) def _reset_api_throttle(): + """Clear the shared API-key throttle cache before each test""" # ApiKeyRateThrottle stores per-token request counts in the shared cache and # is not reset between tests. This suite makes many API-key calls, so clear it # per test to avoid cross-test 429s (and to avoid inflating the bucket for @@ -53,6 +57,7 @@ def service_account(db, workspace): def _service_account_for(workspace: Workspace) -> WorkspaceMember: + """Return the workspace's SERVICE bot membership""" return WorkspaceMember.objects.select_related("member").get( workspace=workspace, member__is_bot=True, @@ -66,6 +71,7 @@ class TestServiceAccountCommand: @pytest.mark.django_db def test_command_creates_active_verified_bot_member_with_token(self, workspace): + """Test the command creates an active, email-verified bot member with a bot token""" call_command( "create_service_account", workspace=workspace.slug, @@ -98,6 +104,7 @@ def test_command_creates_active_verified_bot_member_with_token(self, workspace): @pytest.mark.django_db def test_command_token_authenticates_as_distinct_actor(self, workspace): + """Test the minted token authenticates as the service account itself""" call_command("create_service_account", workspace=workspace.slug, name="Robby", role="admin") user = _service_account_for(workspace).member @@ -113,6 +120,7 @@ def test_command_token_authenticates_as_distinct_actor(self, workspace): @pytest.mark.django_db def test_command_service_account_writes_are_attributed_to_it(self, workspace): + """Test writes made with the token are attributed to the service account""" call_command("create_service_account", workspace=workspace.slug, name="Author Bot", role="admin") user = _service_account_for(workspace).member @@ -132,6 +140,7 @@ def test_command_service_account_writes_are_attributed_to_it(self, workspace): @pytest.mark.django_db def test_command_supports_member_role(self, workspace): + """Test the command honours the member role""" call_command("create_service_account", workspace=workspace.slug, name="Member Bot", role="member") member = _service_account_for(workspace) @@ -139,6 +148,7 @@ def test_command_supports_member_role(self, workspace): @pytest.mark.django_db def test_command_unknown_workspace_errors(self): + """Test an unknown workspace slug raises CommandError""" from django.core.management.base import CommandError with pytest.raises(CommandError): @@ -146,6 +156,7 @@ def test_command_unknown_workspace_errors(self): @pytest.mark.django_db def test_command_duplicate_email_errors_cleanly(self, workspace): + """Test a taken email raises CommandError, not a traceback""" User.objects.create(username="taken", email="taken@plane.so") from django.core.management.base import CommandError @@ -161,6 +172,7 @@ def test_command_duplicate_email_errors_cleanly(self, workspace): @pytest.mark.django_db def test_command_integrity_error_becomes_command_error(self, workspace): + """Test an IntegrityError from the helper is reported as CommandError""" # A uniqueness collision that slips past the pre-check (e.g. an email # race) surfaces as IntegrityError from the helper; the command must # translate it into a readable CommandError, not a raw traceback. @@ -176,6 +188,7 @@ def test_command_integrity_error_becomes_command_error(self, workspace): @pytest.mark.django_db def test_command_username_and_display_name_round_trip(self, workspace): + """Test --username and --display-name land on the created user""" call_command( "create_service_account", workspace=workspace.slug, @@ -191,14 +204,49 @@ def test_command_username_and_display_name_round_trip(self, workspace): @pytest.mark.django_db def test_command_omitted_identity_falls_back_to_synthetic(self, workspace): + """Test the command falls back to a synthetic username and the given name""" call_command("create_service_account", workspace=workspace.slug, name="Fallback Bot", role="admin") user = _service_account_for(workspace).member assert user.username.startswith("svc_") assert user.display_name == "Fallback Bot" + @pytest.mark.django_db + def test_command_whitespace_identity_falls_back_to_synthetic(self, workspace): + """Test whitespace-only identity options are normalized, not stored verbatim""" + call_command( + "create_service_account", + workspace=workspace.slug, + name="WS Bot", + role="admin", + username=" ", + display_name=" ", + email=" ", + ) + + user = _service_account_for(workspace).member + # Whitespace-only values are treated as omitted → synthetic / name fallback. + assert user.username.startswith("svc_") + assert user.display_name == "WS Bot" + assert user.email.endswith("@service.plane.local") + + @pytest.mark.django_db + def test_command_rejects_too_long_username(self, workspace): + """Test an over-long --username fails with CommandError, not a DB DataError""" + from django.core.management.base import CommandError + + with pytest.raises(CommandError): + call_command( + "create_service_account", + workspace=workspace.slug, + name="Long", + role="admin", + username="a" * 129, # User.username max_length is 128 + ) + @pytest.mark.django_db def test_command_duplicate_username_errors_cleanly(self, workspace): + """Test a taken username raises CommandError and leaks nothing""" User.objects.create(username="ci-bot", email="someone@plane.so") from django.core.management.base import CommandError @@ -217,6 +265,7 @@ def test_command_duplicate_username_errors_cleanly(self, workspace): @pytest.mark.django_db def test_command_blank_name_errors(self, workspace): + """Test a blank --name raises CommandError""" from django.core.management.base import CommandError with pytest.raises(CommandError): @@ -228,10 +277,12 @@ class TestServiceAccountEndpoint: """The admin-scoped HTTP endpoint mirrors the command.""" def _url(self, slug: str) -> str: + """Helper to get the service-accounts collection URL""" return f"/api/v1/workspaces/{slug}/service-accounts/" @pytest.mark.django_db def test_admin_creates_service_account_and_token_works(self, api_key_client, workspace, create_user): + """Test an admin can create an account whose token authenticates and attributes""" response = api_key_client.post( self._url(workspace.slug), {"name": "HTTP Bot", "role": "admin"}, @@ -266,6 +317,7 @@ def test_admin_creates_service_account_and_token_works(self, api_key_client, wor @pytest.mark.django_db def test_minted_token_is_not_persisted_in_request_log(self, api_key_client, workspace): + """Test the created account's token is redacted from the API activity log""" # The admin calls this endpoint WITH their X-Api-Key, so APITokenLogMiddleware # runs and would otherwise persist the response body (which carries the # freshly minted token) into api_activity_logs in plaintext. The response is @@ -286,6 +338,7 @@ def test_minted_token_is_not_persisted_in_request_log(self, api_key_client, work @pytest.mark.django_db def test_non_admin_member_is_forbidden(self, api_client, workspace): + """Test a non-admin member cannot create a service account""" # A guest member of the workspace is not a workspace admin. guest = User.objects.create(username="guest_user", email="guest@plane.so") guest.set_password("guest-pass") @@ -305,6 +358,7 @@ def test_non_admin_member_is_forbidden(self, api_client, workspace): @pytest.mark.django_db def test_unauthenticated_is_rejected(self, api_client, workspace): + """Test an unauthenticated request is rejected""" response = api_client.post( self._url(workspace.slug), {"name": "Anon", "role": "admin"}, @@ -314,6 +368,7 @@ def test_unauthenticated_is_rejected(self, api_client, workspace): @pytest.mark.django_db def test_custom_username_and_display_name(self, api_key_client, workspace): + """Test a caller-chosen username and display name are applied""" response = api_key_client.post( self._url(workspace.slug), {"name": "CI", "role": "admin", "username": "ci-provisioner", "display_name": "CI Provisioner"}, @@ -331,6 +386,7 @@ def test_custom_username_and_display_name(self, api_key_client, workspace): @pytest.mark.django_db def test_omitted_identity_falls_back_to_synthetic(self, api_key_client, workspace): + """Test omitted identity falls back to a synthetic username and the given name""" response = api_key_client.post( self._url(workspace.slug), {"name": "Fallback Bot", "role": "admin"}, @@ -343,6 +399,7 @@ def test_omitted_identity_falls_back_to_synthetic(self, api_key_client, workspac @pytest.mark.django_db def test_duplicate_username_is_rejected_with_code(self, api_key_client, workspace): + """Test a duplicate username returns 409 with a machine-readable code""" User.objects.create(username="ci-provisioner", email="human@plane.so") response = api_key_client.post( @@ -359,12 +416,14 @@ def test_duplicate_username_is_rejected_with_code(self, api_key_client, workspac @pytest.mark.django_db def test_username_race_returns_409(self, api_key_client, workspace): + """Test a username taken after the pre-check still returns 409""" # Simulate a race: the username is free at the pre-check, then another # actor creates it and our insert raises IntegrityError. The scoped # handler re-checks, sees it now exists, and returns the same 409. from django.db import IntegrityError def racing_create(**kwargs): + """Take the username, then fail as the database would.""" User.objects.create(username="raced", email="racer@plane.so") raise IntegrityError("duplicate key value violates unique constraint") @@ -380,6 +439,7 @@ def racing_create(**kwargs): @pytest.mark.django_db def test_non_username_integrity_error_is_not_mislabeled(self, api_key_client, workspace): + """Test an unrelated IntegrityError is not reported as a username conflict""" # An IntegrityError unrelated to the username (the username is still free) # must not be reported as a username conflict — it falls through to the # base handler (HTTP 400) instead. @@ -400,6 +460,7 @@ def test_non_username_integrity_error_is_not_mislabeled(self, api_key_client, wo @pytest.mark.django_db def test_custom_username_with_omitted_display_name_falls_back_to_name(self, api_key_client, workspace): + """Test a custom username with no display name falls back to the given name""" response = api_key_client.post( self._url(workspace.slug), {"name": "CI Provisioner", "role": "admin", "username": "ci-only"}, @@ -412,6 +473,7 @@ def test_custom_username_with_omitted_display_name_falls_back_to_name(self, api_ @pytest.mark.django_db def test_blank_identity_normalizes_to_synthetic(self, api_key_client, workspace): + """Test blank identity fields normalize to a synthetic username and the given name""" # A blank username/display_name is treated as omitted (identical to the # management command), not a 400. response = api_key_client.post( @@ -424,6 +486,18 @@ def test_blank_identity_normalizes_to_synthetic(self, api_key_client, workspace) assert response.data["username"].startswith("svc_") assert response.data["display_name"] == "Blanky" + @pytest.mark.django_db + def test_explicit_empty_description_is_preserved(self, api_key_client, workspace): + """Test an explicit empty description is stored as-is, not defaulted""" + response = api_key_client.post( + self._url(workspace.slug), + {"name": "Desc HTTP", "role": "admin", "description": ""}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert APIToken.objects.get(user_id=response.data["id"]).description == "" + @pytest.mark.contract class TestServiceAccountHelper: @@ -431,6 +505,7 @@ class TestServiceAccountHelper: @pytest.mark.django_db def test_duplicate_username_rolls_back_atomically(self, workspace): + """Test a duplicate username rolls the whole creation back""" # A genuine (non-mocked) unique violation: the helper is @transaction.atomic, # so the failed second creation must leave nothing behind. from django.db import IntegrityError @@ -450,22 +525,38 @@ def test_duplicate_username_rolls_back_atomically(self, workspace): assert WorkspaceMember.objects.count() == members_before assert APIToken.objects.count() == tokens_before + @pytest.mark.django_db + def test_omitted_description_uses_generated_default(self, workspace): + """Test an omitted description falls back to the generated default""" + sa = create_service_account(workspace=workspace, name="Desc Bot", username="desc-default") + assert sa.api_token.description == "Service account token for Desc Bot" + + @pytest.mark.django_db + def test_explicit_empty_description_is_preserved(self, workspace): + """Test an explicit empty description is preserved, not replaced by the default""" + sa = create_service_account(workspace=workspace, name="Desc Bot", username="desc-blank", description="") + assert sa.api_token.description == "" + @pytest.mark.contract class TestServiceAccountTokenLifecycle: """List, mint, rotate, and revoke a service account's API tokens.""" def _tokens_url(self, slug, user_id): + """Helper to get the tokens endpoint URL""" return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/" def _token_url(self, slug, user_id, token_id): + """Helper to get a single token endpoint URL""" return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/{token_id}/" def _rotate_url(self, slug, user_id, token_id): + """Helper to get the token rotate endpoint URL""" return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/{token_id}/rotate/" @pytest.mark.django_db def test_list_tokens_masks_value(self, api_key_client, workspace, service_account): + """Test the token list returns metadata but never the secret value""" response = api_key_client.get(self._tokens_url(workspace.slug, service_account.user.id)) assert response.status_code == status.HTTP_200_OK, response.data @@ -481,6 +572,7 @@ def test_list_tokens_masks_value(self, api_key_client, workspace, service_accoun @pytest.mark.django_db def test_mint_token_returns_value_once_and_authenticates(self, api_key_client, workspace, service_account): + """Test a minted token is returned once and authenticates as the account""" response = api_key_client.post( self._tokens_url(workspace.slug, service_account.user.id), {"label": "second-key"}, @@ -498,8 +590,25 @@ def test_mint_token_returns_value_once_and_authenticates(self, api_key_client, w # The account now has two tokens. assert APIToken.objects.filter(user=service_account.user).count() == 2 + @pytest.mark.django_db + def test_mint_token_without_label_generates_one(self, api_key_client, workspace, service_account): + """Test minting with no label generates one and still returns a usable token""" + response = api_key_client.post( + self._tokens_url(workspace.slug, service_account.user.id), + {}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + # A label was generated (the model default), not left blank. + assert response.data["label"] + minted = response.data["token"] + assert minted.startswith("plane_api_") + assert _client_for_token(minted).get(USERS_ME_URL).status_code == status.HTTP_200_OK + @pytest.mark.django_db def test_mint_token_not_persisted_in_request_log(self, api_key_client, workspace, service_account): + """Test a minted token is redacted from the API activity log""" with patch("plane.middleware.logger.process_logs") as process_logs: response = api_key_client.post( self._tokens_url(workspace.slug, service_account.user.id), @@ -515,9 +624,7 @@ def test_mint_token_not_persisted_in_request_log(self, api_key_client, workspace @pytest.mark.django_db def test_mint_token_with_expiry(self, api_key_client, workspace, service_account): - from django.utils import timezone - from datetime import timedelta - + """Test a token can be minted with an expiry""" expires = (timezone.now() + timedelta(days=7)).isoformat() response = api_key_client.post( self._tokens_url(workspace.slug, service_account.user.id), @@ -529,7 +636,26 @@ def test_mint_token_with_expiry(self, api_key_client, workspace, service_account assert response.data["expired_at"] is not None @pytest.mark.django_db - def test_rotate_invalidates_old_token(self, api_key_client, workspace, service_account): + def test_non_service_tokens_are_not_managed(self, api_key_client, workspace, service_account): + """Test the token APIs only manage service tokens, not other tokens on the user""" + # A non-service token is outside this API's scope: not listed, and not + # revokable/rotatable through it. + other = APIToken.objects.create( + user=service_account.user, workspace=workspace, token=generate_token(), is_service=False + ) + + listing = api_key_client.get(self._tokens_url(workspace.slug, service_account.user.id)) + listed_ids = {str(row["id"]) for row in listing.data["results"]} + assert str(other.id) not in listed_ids + + token_url = self._token_url(workspace.slug, service_account.user.id, other.id) + rotate_url = self._rotate_url(workspace.slug, service_account.user.id, other.id) + assert api_key_client.delete(token_url).status_code == status.HTTP_404_NOT_FOUND + assert api_key_client.post(rotate_url).status_code == status.HTTP_404_NOT_FOUND + + @pytest.mark.django_db + def test_rotate_invalidates_old_token(self, api_key_client, workspace, service_account, create_user): + """Test rotation returns a new token and invalidates the old one""" old_value = service_account.token old_id = service_account.api_token.id @@ -548,12 +674,17 @@ def test_rotate_invalidates_old_token(self, api_key_client, workspace, service_a ) assert _client_for_token(new_value).get(USERS_ME_URL).status_code == status.HTTP_200_OK - # The old token row is retained but deactivated. + # The old token row is retained but deactivated. Its deactivation goes + # through QuerySet.update() (bypassing save()), so the audit fields are + # set explicitly and attributed to the acting admin. service_account.api_token.refresh_from_db() assert service_account.api_token.is_active is False + assert service_account.api_token.updated_at is not None + assert service_account.api_token.updated_by_id == create_user.id @pytest.mark.django_db def test_rotate_not_persisted_in_request_log(self, api_key_client, workspace, service_account): + """Test a rotated token value is redacted from the API activity log""" with patch("plane.middleware.logger.process_logs") as process_logs: response = api_key_client.post( self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id) @@ -565,8 +696,138 @@ def test_rotate_not_persisted_in_request_log(self, api_key_client, workspace, se assert log_data["response_body"] == "[REDACTED]" assert new_value not in (log_data["response_body"] or "") + @pytest.mark.django_db + def test_rotate_inherits_source_expiry_when_omitted(self, api_key_client, workspace, service_account): + """Test rotation inherits the source token's expiry when none is supplied""" + # Rotating a bounded-lifetime credential must not silently produce an + # immortal one, so an omitted expired_at carries the source expiry over. + expires = timezone.now() + timedelta(days=7) + APIToken.objects.filter(pk=service_account.api_token.pk).update(expired_at=expires) + + response = api_key_client.post( + self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id) + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["expired_at"] is not None + replacement = APIToken.objects.get(id=response.data["id"]) + assert replacement.expired_at == expires + + @pytest.mark.django_db + def test_rotate_explicit_null_clears_expiry(self, api_key_client, workspace, service_account): + """Test an explicit null expiry makes the replacement never expire""" + expires = timezone.now() + timedelta(days=7) + APIToken.objects.filter(pk=service_account.api_token.pk).update(expired_at=expires) + + response = api_key_client.post( + self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id), + {"expired_at": None}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["expired_at"] is None + assert APIToken.objects.get(id=response.data["id"]).expired_at is None + + @pytest.mark.django_db + def test_rotate_explicit_expiry_is_applied(self, api_key_client, workspace, service_account): + """Test an explicitly supplied expiry is applied to the replacement""" + new_expiry = timezone.now() + timedelta(days=30) + + response = api_key_client.post( + self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id), + {"expired_at": new_expiry.isoformat()}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + replacement = APIToken.objects.get(id=response.data["id"]) + assert replacement.expired_at is not None + assert abs((replacement.expired_at - new_expiry).total_seconds()) < 1 + + @pytest.mark.django_db + def test_rotate_inactive_token_is_rejected(self, api_key_client, workspace, service_account): + """Test rotating an inactive token returns 409 and mints nothing""" + url = self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id) + first = api_key_client.post(url) + assert first.status_code == status.HTTP_201_CREATED, first.data + + active_before = APIToken.objects.filter(user=service_account.user, is_active=True).count() + + # The source token is now deactivated — it cannot mint further replacements. + second = api_key_client.post(url) + assert second.status_code == status.HTTP_409_CONFLICT, second.data + assert second.data["code"] == "TOKEN_NOT_ACTIVE" + assert APIToken.objects.filter(user=service_account.user, is_active=True).count() == active_before + + @pytest.mark.django_db + def test_rotate_elapsed_source_expiry_requires_explicit_intent(self, api_key_client, workspace, service_account): + """Test rotating a token whose expiry elapsed requires an explicit expiry""" + # An elapsed expiry is still is_active=True; inheriting it would mint a + # replacement that can never authenticate, so the caller must say what it + # wants instead. + APIToken.objects.filter(pk=service_account.api_token.pk).update(expired_at=timezone.now() - timedelta(days=1)) + url = self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id) + + rejected = api_key_client.post(url) + assert rejected.status_code == status.HTTP_400_BAD_REQUEST, rejected.data + assert rejected.data["code"] == "SOURCE_TOKEN_EXPIRY_ELAPSED" + # Nothing was minted and the source token is untouched. + assert APIToken.objects.filter(user=service_account.user).count() == 1 + assert APIToken.objects.get(pk=service_account.api_token.pk).is_active is True + + # Supplying the expiry explicitly is the documented way forward. + accepted = api_key_client.post(url, {"expired_at": None}, format="json") + assert accepted.status_code == status.HTTP_201_CREATED, accepted.data + assert _client_for_token(accepted.data["token"]).get(USERS_ME_URL).status_code == status.HTTP_200_OK + + @pytest.mark.django_db + def test_rotate_form_encoded_empty_expiry_inherits(self, api_key_client, workspace, service_account): + """Test an empty form-encoded expiry is treated as omitted, not as null""" + # A form/multipart client sending an empty expired_at must NOT clear the + # expiry (that would silently mint an immortal token); it inherits like an + # omitted field. + expires = timezone.now() + timedelta(days=7) + APIToken.objects.filter(pk=service_account.api_token.pk).update(expired_at=expires) + + # Default APIClient format is multipart (HTML input), not JSON. + response = api_key_client.post( + self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id), + {"expired_at": ""}, + ) + + assert response.status_code == status.HTTP_201_CREATED, response.data + assert response.data["expired_at"] is not None + assert APIToken.objects.get(id=response.data["id"]).expired_at == expires + + @pytest.mark.django_db + def test_rotate_past_expiry_is_rejected(self, api_key_client, workspace, service_account): + """Test an explicitly supplied past expiry is rejected with 400""" + response = api_key_client.post( + self._rotate_url(workspace.slug, service_account.user.id, service_account.api_token.id), + {"expired_at": (timezone.now() - timedelta(days=1)).isoformat()}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST, response.data + # The source token is untouched — a rejected rotation mints nothing. + assert APIToken.objects.get(pk=service_account.api_token.pk).is_active is True + assert APIToken.objects.filter(user=service_account.user).count() == 1 + + @pytest.mark.django_db + def test_mint_past_expiry_is_rejected(self, api_key_client, workspace, service_account): + """Test minting a token with a past expiry is rejected with 400""" + response = api_key_client.post( + self._tokens_url(workspace.slug, service_account.user.id), + {"label": "dead", "expired_at": (timezone.now() - timedelta(days=1)).isoformat()}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST, response.data + @pytest.mark.django_db def test_revoke_token_invalidates_it(self, api_key_client, workspace, service_account): + """Test a revoked token stops authenticating and disappears from the list""" old_value = service_account.token response = api_key_client.delete( self._token_url(workspace.slug, service_account.user.id, service_account.api_token.id) @@ -583,6 +844,7 @@ def test_revoke_token_invalidates_it(self, api_key_client, workspace, service_ac @pytest.mark.django_db def test_token_endpoints_404_for_non_service_user(self, api_key_client, workspace, create_user): + """Test every token endpoint 404s for a non-service user""" import uuid # create_user is a human admin, not a service account — no token endpoint applies. @@ -596,6 +858,7 @@ def test_token_endpoints_404_for_non_service_user(self, api_key_client, workspac @pytest.mark.django_db def test_cross_workspace_isolation(self, api_key_client, workspace, service_account): + """Test another workspace's account and tokens are unreachable""" # A service account in another workspace must be unreachable via THIS # workspace's slug, even by this workspace's admin (api_key_client). owner = User.objects.create(username="other_owner", email="oo@plane.so") @@ -616,6 +879,7 @@ def test_cross_workspace_isolation(self, api_key_client, workspace, service_acco @pytest.mark.django_db def test_non_admin_cannot_manage_tokens(self, api_client, workspace, service_account): + """Test a non-admin cannot list, mint, or revoke tokens""" guest = User.objects.create(username="guest_tok", email="guesttok@plane.so") WorkspaceMember.objects.create(workspace=workspace, member=guest, role=5, is_active=True) guest_token = APIToken.objects.create(user=guest, label="g", token=generate_token()) @@ -633,10 +897,12 @@ class TestServiceAccountDecommission: """DELETE /service-accounts/{user_id}/ — decommission with cascade + guards.""" def _url(self, slug, user_id): + """Helper to get the decommission (detail) URL""" return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/" @pytest.mark.django_db - def test_decommission_cascade(self, api_key_client, workspace, service_account): + def test_decommission_cascade(self, api_key_client, workspace, service_account, create_user): + """Test decommissioning revokes access and removes memberships""" user = service_account.user # Give the account a project membership to prove the cascade removes it. # Attribute the project to the service account explicitly (created_by is @@ -658,6 +924,13 @@ def test_decommission_cascade(self, api_key_client, workspace, service_account): status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN, ) + # Token deactivation goes through QuerySet.update() (bypassing save()), so + # the audit fields are set explicitly: the deactivated token is stamped + # updated_at and attributed to the acting admin, mirroring rotation. + deactivated = APIToken.objects.get(pk=service_account.api_token.pk) + assert deactivated.is_active is False + assert deactivated.updated_at is not None + assert deactivated.updated_by_id == create_user.id # The user row survives (deactivated) so historical attribution is intact. user.refresh_from_db() assert user.is_active is False @@ -665,6 +938,7 @@ def test_decommission_cascade(self, api_key_client, workspace, service_account): @pytest.mark.django_db def test_deactivated_membership_can_still_be_decommissioned(self, api_key_client, workspace, service_account): + """Test a deactivated membership is still manageable and can be decommissioned""" # A merely-deactivated membership must not block management: the tokens # still authenticate (auth ignores membership), so the admin must still # be able to list and decommission the account. @@ -680,6 +954,7 @@ def test_deactivated_membership_can_still_be_decommissioned(self, api_key_client @pytest.mark.django_db def test_decommission_rejects_human_user(self, api_key_client, workspace, create_user): + """Test decommissioning a human user is rejected""" response = api_key_client.delete(self._url(workspace.slug, create_user.id)) assert response.status_code == status.HTTP_400_BAD_REQUEST assert response.data["code"] == "NOT_A_SERVICE_ACCOUNT" @@ -688,6 +963,7 @@ def test_decommission_rejects_human_user(self, api_key_client, workspace, create @pytest.mark.django_db def test_decommission_rejects_other_bot_type(self, api_key_client, workspace): + """Test decommissioning a non-service bot is rejected""" bot = User.objects.create( username="seed_bot", email="seed@plane.so", is_bot=True, bot_type=BotTypeEnum.WORKSPACE_SEED ) @@ -699,6 +975,7 @@ def test_decommission_rejects_other_bot_type(self, api_key_client, workspace): @pytest.mark.django_db def test_decommission_unknown_user_is_404(self, api_key_client, workspace): + """Test decommissioning an unknown user returns 404""" import uuid response = api_key_client.delete(self._url(workspace.slug, uuid.uuid4())) @@ -706,9 +983,25 @@ def test_decommission_unknown_user_is_404(self, api_key_client, workspace): @pytest.mark.django_db def test_non_admin_cannot_decommission(self, api_client, workspace, service_account): + """Test a non-admin cannot decommission a service account""" guest = User.objects.create(username="guest_dec", email="guestdec@plane.so") WorkspaceMember.objects.create(workspace=workspace, member=guest, role=5, is_active=True) guest_token = APIToken.objects.create(user=guest, label="g", token=generate_token()) response = _client_for_token(guest_token.token).delete(self._url(workspace.slug, service_account.user.id)) assert response.status_code == status.HTTP_403_FORBIDDEN + + @pytest.mark.django_db + def test_decommission_scopes_token_deactivation_to_workspace(self, workspace, service_account): + """Test decommissioning does not deactivate the account's tokens in another workspace""" + from plane.utils.service_account import decommission_service_account, mint_service_account_token + + owner = User.objects.create(username="dec_owner", email="decowner@plane.so") + other_ws = Workspace.objects.create(name="Other Dec", owner=owner, slug="other-ws-dec") + other_token = mint_service_account_token(user=service_account.user, workspace=other_ws) + + decommission_service_account(user=service_account.user, workspace=workspace) + + # This workspace's token is deactivated; the other workspace's is untouched. + assert APIToken.objects.get(pk=service_account.api_token.pk).is_active is False + assert APIToken.objects.get(pk=other_token.pk).is_active is True diff --git a/apps/api/plane/tests/unit/middleware/test_logger.py b/apps/api/plane/tests/unit/middleware/test_logger.py index d9b678f4041..f8aee8b9c55 100644 --- a/apps/api/plane/tests/unit/middleware/test_logger.py +++ b/apps/api/plane/tests/unit/middleware/test_logger.py @@ -77,6 +77,7 @@ def test_no_log_without_api_key(self, middleware, request_factory): assert not process_logs.delay.called def test_response_body_is_logged_by_default(self, middleware, request_factory): + """Test an unflagged response body is logged verbatim""" request = request_factory.get("/api/v1/workspaces/", HTTP_X_API_KEY=self.API_KEY) request.user = AnonymousUser() response = HttpResponse(b'{"ok": true}') @@ -86,6 +87,7 @@ def test_response_body_is_logged_by_default(self, middleware, request_factory): assert log_data["response_body"] == '{"ok": true}' def test_flagged_response_body_is_redacted(self, middleware, request_factory): + """Test a flagged (secret-bearing) response body is redacted""" # A view that returns a secret (e.g. a minted API token) flags its response # via redact_response_body so the body is never persisted in plaintext. request = request_factory.post("/api/v1/workspaces/x/service-accounts/", HTTP_X_API_KEY=self.API_KEY) diff --git a/apps/api/plane/tests/unit/utils/test_service_account.py b/apps/api/plane/tests/unit/utils/test_service_account.py index cb991db5af3..8b6d186befd 100644 --- a/apps/api/plane/tests/unit/utils/test_service_account.py +++ b/apps/api/plane/tests/unit/utils/test_service_account.py @@ -11,20 +11,33 @@ @pytest.mark.unit class TestResolveServiceAccountRole: + """Test service-account role resolution""" + def test_valid_role_names_map_to_values(self): + """Test role names map to their WorkspaceMember role values""" assert resolve_service_account_role("admin") == 20 assert resolve_service_account_role("member") == 15 assert resolve_service_account_role("guest") == 5 def test_valid_integer_roles_pass_through(self): + """Test allowed integer roles pass through unchanged""" for value in SERVICE_ACCOUNT_ROLES.values(): assert resolve_service_account_role(value) == value def test_unknown_role_name_raises(self): + """Test an unknown role name raises ValueError""" with pytest.raises(ValueError): resolve_service_account_role("owner") + def test_unknown_role_name_suppresses_keyerror_context(self): + """Test the ValueError does not chain the internal KeyError""" + with pytest.raises(ValueError) as exc_info: + resolve_service_account_role("owner") + assert exc_info.value.__suppress_context__ is True + assert exc_info.value.__cause__ is None + def test_unsupported_integer_role_raises(self): + """Test an integer outside the allowed set raises ValueError""" # An integer outside the allowed set (20/15/5) must not slip through and # create a member with an invalid role. with pytest.raises(ValueError): diff --git a/apps/api/plane/utils/service_account.py b/apps/api/plane/utils/service_account.py index 3b3265ae5fc..73be7da7d8a 100644 --- a/apps/api/plane/utils/service_account.py +++ b/apps/api/plane/utils/service_account.py @@ -21,6 +21,10 @@ # Django imports from django.db import transaction +from django.utils import timezone + +# Third party imports +from crum import get_current_user # Module imports from plane.db.models import APIToken, BotTypeEnum, ProjectMember, User, Workspace, WorkspaceMember @@ -34,6 +38,50 @@ # account only needs member/guest scope. DEFAULT_SERVICE_ACCOUNT_ROLE = "admin" +# Sentinel for "the caller supplied no expiry" on rotation — distinct from an +# explicit None, which means "the replacement never expires". +INHERIT_EXPIRY = object() + + +def _audit_fields() -> dict: + """Audit fields for a bulk ``QuerySet.update()``. + + ``QuerySet.update()`` bypasses ``Model.save()``, so ``updated_at`` (auto_now) + and ``updated_by`` are not set automatically. Set them explicitly, mirroring + what ``BaseModel.save()`` would do — ``updated_by`` is the current actor, or + ``None`` when there is no authenticated user. + """ + actor = get_current_user() + return { + "updated_at": timezone.now(), + "updated_by_id": actor.id if (actor is not None and not actor.is_anonymous) else None, + } + + +class ServiceAccountTokenError(Exception): + """Base for token-rotation preconditions the caller has to resolve. + + Subclasses carry a machine-readable ``code`` and the ``status_code`` the HTTP + layer should surface, so a view can map the whole family in one ``except``. + """ + + code = "SERVICE_ACCOUNT_TOKEN_ERROR" + status_code = 400 + + +class TokenNotActiveError(ServiceAccountTokenError): + """Raised when rotating a token that is not active.""" + + code = "TOKEN_NOT_ACTIVE" + status_code = 409 + + +class SourceExpiryElapsedError(ServiceAccountTokenError): + """Raised when inheriting the source token's expiry would mint an already-expired token.""" + + code = "SOURCE_TOKEN_EXPIRY_ELAPSED" + status_code = 400 + @dataclass class ServiceAccount: @@ -50,6 +98,7 @@ class ServiceAccount: @property def token(self) -> str: + """Return the plaintext API key minted for this account.""" return self.api_token.token @@ -64,7 +113,9 @@ def resolve_service_account_role(role: str | int) -> int: return SERVICE_ACCOUNT_ROLES[role] except KeyError: valid = ", ".join(SERVICE_ACCOUNT_ROLES) - raise ValueError(f"Invalid role '{role}'. Choose one of: {valid}.") + # `from None` so the user-facing validation error isn't buried under a + # noisy KeyError chain. + raise ValueError(f"Invalid role '{role}'. Choose one of: {valid}.") from None @transaction.atomic @@ -74,7 +125,7 @@ def create_service_account( name: str, role: str | int = DEFAULT_SERVICE_ACCOUNT_ROLE, email: str | None = None, - description: str = "", + description: str | None = None, username: str | None = None, display_name: str | None = None, ) -> ServiceAccount: @@ -93,7 +144,8 @@ def create_service_account( from the DB insert — the caller is expected to check for it and surface a readable error); when omitted a synthetic ``svc_`` value is generated. ``display_name`` is what the workspace members UI shows; it falls back to - ``name`` when omitted. + ``name`` when omitted. ``description`` is the token's description; ``None`` + (omitted) applies a generated default, while an explicit ``""`` is preserved. No email is sent and no password is ever round-tripped. ``is_bot=True`` additionally blocks the interactive login/signup flow @@ -144,11 +196,17 @@ def create_service_account( company_role="", ) + # None means "omitted" → apply the generated default; an explicit "" is a + # deliberate empty description and is preserved (do not use `or`, which would + # conflate blank with omitted). + if description is None: + description = f"Service account token for {name}" + api_token = mint_service_account_token( user=user, workspace=workspace, label=name, - description=description or f"Service account token for {name}", + description=description, ) return ServiceAccount(user=user, member=member, api_token=api_token) @@ -175,23 +233,53 @@ def mint_service_account_token(*, user, workspace, label=None, description="", e @transaction.atomic -def rotate_service_account_token(*, token: APIToken, expired_at=None) -> APIToken: +def rotate_service_account_token(*, token: APIToken, expired_at=INHERIT_EXPIRY) -> APIToken: """Atomically mint a replacement token and deactivate the old one. - The replacement inherits the old token's label/description/workspace so the - rotation is transparent to consumers; ``expired_at`` may be set anew. The old - token is deactivated (``is_active=False``) so authenticating with it fails - immediately. + The replacement inherits the source token's label, description, workspace + and — unless the caller says otherwise — its ``expired_at``, so rotating a + bounded-lifetime credential never silently produces a never-expiring one. + Pass a datetime to set a new expiry, or an explicit ``None`` to clear it. + + Raises :class:`TokenNotActiveError` if the source token is not active (an + already-rotated or deactivated token cannot be used to mint further + replacements), and :class:`SourceExpiryElapsedError` if inheriting would + produce a token that is already expired — the caller must state its intent + instead of receiving a credential that can never authenticate. + + The old token is deactivated (``is_active=False``) so authenticating with it + fails immediately. """ + inherited = expired_at is INHERIT_EXPIRY + new_expired_at = token.expired_at if inherited else expired_at + + # When inheriting, the replacement copies the source's ABSOLUTE expiry instant + # (its remaining lifetime), never a fresh window. If that instant has already + # elapsed, inheriting would hand back a token that can never authenticate + # (auth requires expired_at > now), so make the caller state its intent. An + # explicitly-supplied expiry is validated for future-ness at the serializer, + # so it is trusted here. + if inherited and new_expired_at is not None and new_expired_at <= timezone.now(): + raise SourceExpiryElapsedError( + "The source token's expiry has already elapsed; pass expired_at explicitly (a future timestamp or null)." + ) + + # Deactivate FIRST, conditionally on the row still being active: a plain + # read-then-write is racy, so two concurrent rotations of the same token + # would each mint an active replacement. Only the caller whose UPDATE + # actually matched a row proceeds. .update() bypasses save(), so updated_at + # (auto_now) and the acting user are set explicitly via _audit_fields. + if not APIToken.objects.filter(pk=token.pk, is_active=True).update(is_active=False, **_audit_fields()): + raise TokenNotActiveError("The token is not active and cannot be rotated.") + replacement = mint_service_account_token( user=token.user, workspace=token.workspace, label=token.label, description=token.description, - expired_at=expired_at, + expired_at=new_expired_at, ) token.is_active = False - token.save(update_fields=["is_active", "updated_at"]) return replacement @@ -205,12 +293,15 @@ def decommission_service_account(*, user: User, workspace: Workspace) -> None: everything it created) survives; ``is_active=False`` alone revokes API access (``APIKeyAuthentication`` requires ``user__is_active``). - The token/user deactivation is global while membership removal is scoped to - ``workspace``; this is consistent because a service account belongs to - exactly one workspace (it is created for one workspace and there is no API to - add it to another), so "retire the user" == "retire it in this workspace". + Token deactivation and membership removal are scoped to ``workspace`` (tokens + are workspace-scoped via ``APIToken.workspace``), so decommissioning in one + workspace never touches tokens or memberships in another. The User is then + deactivated to retire the identity — a service account belongs to exactly one + workspace, so this completes the revocation. """ - APIToken.objects.filter(user=user).update(is_active=False) + # .update() bypasses save(), so set the audit fields (updated_at/updated_by) + # explicitly, consistent with rotation. + APIToken.objects.filter(user=user, workspace=workspace).update(is_active=False, **_audit_fields()) ProjectMember.objects.filter(member=user, workspace=workspace).delete() WorkspaceMember.objects.filter(member=user, workspace=workspace).delete() user.is_active = False diff --git a/docs/service-accounts.md b/docs/service-accounts.md index b1b7efd26ea..ef0dedf0c98 100644 --- a/docs/service-accounts.md +++ b/docs/service-accounts.md @@ -22,18 +22,19 @@ Creating a service account performs three writes in a single transaction: (`is_bot=True`, `bot_type=SERVICE`) with an unusable password (`is_password_autoset=True`). Its `username` and `display_name` may be caller-chosen (see [Identity fields](#identity-fields)) or default to - synthetic values; its `email` is always a unique synthetic address — no mail - is ever sent to it. + synthetic values; its `email` defaults to a unique synthetic address (the + management command's `--email` can override it) — no mail is ever sent to it. 2. A `WorkspaceMember` binding the user to the workspace at the requested role. 3. An `APIToken` (`user_type=Bot`, `is_service=True`, scoped to the workspace). Its plaintext value is printed/returned **once** — store it securely, it cannot be retrieved again. -Because `is_bot=True`, the account is intentionally omitted from the human-facing -workspace **member list** (the same behaviour as the built-in workspace-seed bot). -It is still a full member for authorization and attribution: it passes permission -checks and appears as the `created_by`/`updated_by` actor on everything it writes, -and it is visible via the API (e.g. `GET /api/v1/workspaces/{slug}/members/`). +Because `is_bot=True`, the account is omitted from the **web app's** member list +(the internal app API filters bots out of its member endpoints, the same +behaviour as the built-in workspace-seed bot). It is still a full member for +authorization and attribution: it passes permission checks and appears as the +`created_by`/`updated_by` actor on everything it writes, and the **public API** +lists it (e.g. `GET /api/v1/workspaces/{slug}/members/` returns bot members). ## Roles @@ -168,21 +169,49 @@ POST /api/v1/workspaces/{slug}/service-accounts/{user_id}/tokens/ X-Api-Key: Content-Type: application/json -{ "label": "ci-2025", "expired_at": "2026-01-01T00:00:00Z" } +{ "label": "ci-runner", "expired_at": "2099-12-31T23:59:59Z" } ``` -`label`, `description`, and `expired_at` are optional. Response `201` returns the -new token value **once**: +`label`, `description`, and `expired_at` are optional (a supplied `expired_at` +must be in the future). Response `201` returns the new token value **once**: ```json -{ "id": "…", "label": "ci-2025", "is_active": true, "created_at": "…", "expired_at": "…", "token": "plane_api_…" } +{ "id": "…", "label": "ci-runner", "is_active": true, "created_at": "…", "expired_at": "…", "token": "plane_api_…" } ``` **Rotate** — `POST .../tokens/{token_id}/rotate/` -Atomically mints a replacement (optional `expired_at` in the body, returned once) -and deactivates the old token, so authenticating with the old value fails -immediately. The old token stays listed as `is_active: false` for audit. +Atomically mints a replacement (returned once) and deactivates the old token, so +authenticating with the old value fails immediately. The old token stays listed as +`is_active: false` for audit. The replacement inherits the source token's `label` +and `description`; only the expiry is caller-settable: + +| request body | replacement `expired_at` | +| --- | --- | +| `expired_at` omitted (or empty form value) | **inherits** the source token's expiry | +| `{"expired_at": null}` | never expires (explicit opt-out) | +| `{"expired_at": ""}` | that timestamp | + +Rotation never *widens* a credential's validity window unless you ask: an omitted +expiry copies the source token's **absolute** expiry instant, so the replacement +carries the source's *remaining* lifetime — not a renewed window. Rotating a token +that expires next Tuesday yields a replacement that also expires next Tuesday; pass +`expired_at` explicitly to extend it. A supplied timestamp must be in the future +(a past value is rejected with `400`). + +Two preconditions are enforced: + +- Only an **active** token can be rotated. A token that has already been rotated + away returns `409 {"code": "TOKEN_NOT_ACTIVE"}` — one source token cannot mint an + endless chain of replacements; mint a new token instead. (A revoked or + decommissioned token is no longer addressable and returns `404`.) +- If the source token's expiry has **already elapsed**, inheriting it would hand + back a token that can never authenticate, so the request returns + `400 {"code": "SOURCE_TOKEN_EXPIRY_ELAPSED"}`; re-send it with `expired_at` set + to a **future** timestamp, or `null` for no expiry. + +If a `201` is lost in transit (the secret is unrecoverable), recover with +list → revoke the orphaned token → mint a new one. **Revoke** — `DELETE .../tokens/{token_id}/` From c5f77efa8b8abb4587eb5b3a7e22132d278ab0b6 Mon Sep 17 00:00:00 2001 From: Seena Fallah Date: Mon, 27 Jul 2026 20:40:47 +0000 Subject: [PATCH 5/5] feat(api): members-list discoverability + decommissioned-account reactivation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two stacked follow-ups on the service-accounts work. A. Members-list discoverability - GET /api/v1/workspaces/{slug}/members/ rows now also carry username, is_bot, and bot_type (added per-row in the view loop, NOT by changing the shared UserLiteSerializer), so external automation can locate a service account by the stable username it chose and tell a bot member apart from a human. A human row reports is_bot=false; a service-account row reports is_bot=true / bot_type="SERVICE". The endpoint's OpenAPI 200 response schema and WORKSPACE_MEMBER_EXAMPLE are updated to match. members-lite is a separate paginated endpoint that does not share this loop and already exposes is_bot; it is intentionally left unchanged. B. Create reactivates a decommissioned service account - POST /service-accounts/, the management command, and the shared helper now REVIVE a decommissioned service account in place when the requested username belongs to one: the User is re-activated (is_active=True), its soft-deleted WorkspaceMember is restored at the requested role (the partial unique index on (workspace, member) WHERE deleted_at IS NULL means the row is restored, never duplicated), and a fresh token is minted — all under the pre-existing user id, so a retired seat is re-provisionable by its stable username. Identity fields (username/email/display_name) are preserved; only role + token are re-provisioned. - A username owned by an active account, or by any non-service user (human or non-SERVICE bot), still raises ServiceAccountUsernameConflictError (409 USERNAME_ALREADY_EXISTS). The helper is the single source of truth; the view's and command's username pre-checks are removed so they cannot block reactivation. Hardening found by adversarial self-review of the above: - Reactivation is SCOPED to the originating workspace. A username is globally unique, so an inactive SERVICE user may belong to a different workspace; reviving it across the boundary would resurrect a foreign-tenant identity under a shared user id (leaking its email/display_name, seizing its username, and conflating attribution), breaking the "a service account belongs to exactly one workspace" invariant. is_reactivatable_service_account() now also requires a (soft-deleted) membership in THIS workspace; a cross-workspace collision returns the same generic 409 (the message does not confirm the username exists elsewhere). - The brand-new-username insert race is classified at its source: the helper wraps the User insert in a savepoint and, on a unique violation where the requested username now exists, raises the typed conflict — so the view no longer needs a post-hoc IntegrityError heuristic (which mislabeled ANY IntegrityError on the reactivation path, where the user always pre-exists, as a username conflict). The view's IntegrityError fallback and the dead USERNAME_CONFLICT constant are removed; a genuinely unexpected IntegrityError now surfaces via handle_exception (400) instead of a misleading 409. - The management command's --email uniqueness guard is now reactivation-aware: it no longer false-blocks a username-keyed reactivation that passes the revived account's own (preserved) --email. Tests: members row carries username/is_bot/bot_type (bot vs human); reactivation via endpoint, command, and helper (same id, restored membership at new role, no duplicate row, fresh working token, old token dead); active / human / non-service / cross-workspace collisions all 409 with the identity untouched; the insert race returns 409; an unrelated IntegrityError is not mislabeled; --email does not block reactivation. Docs (docs/service-accounts.md) updated for both features. Signed-off-by: Seena Fallah --- apps/api/plane/api/views/member.py | 25 +- apps/api/plane/api/views/service_account.py | 45 +-- .../commands/create_service_account.py | 24 +- .../contract/api/test_service_account.py | 346 +++++++++++++++++- apps/api/plane/utils/openapi/examples.py | 18 + apps/api/plane/utils/service_account.py | 192 +++++++++- docs/service-accounts.md | 60 ++- 7 files changed, 631 insertions(+), 79 deletions(-) diff --git a/apps/api/plane/api/views/member.py b/apps/api/plane/api/views/member.py index 4c8a04d599c..049ee5dad3a 100644 --- a/apps/api/plane/api/views/member.py +++ b/apps/api/plane/api/views/member.py @@ -60,7 +60,20 @@ class WorkspaceMemberAPIEndpoint(BaseAPIView): "role": { "type": "integer", "description": "Member role in the workspace", - } + }, + "username": { + "type": "string", + "description": "Stable, globally-unique username of the member", + }, + "is_bot": { + "type": "boolean", + "description": "Whether the member is a bot/service account", + }, + "bot_type": { + "type": "string", + "nullable": True, + "description": "Bot type (e.g. SERVICE); null for human members", + }, }, }, ] @@ -92,8 +105,16 @@ def get(self, request, slug): # Get all the users with their roles users_with_roles = [] for workspace_member in workspace_members: - user_data = UserLiteSerializer(workspace_member.member).data + member = workspace_member.member + user_data = UserLiteSerializer(member).data user_data["role"] = workspace_member.role + # Surface the stable username and bot identity so external automation + # can locate a service account by the username it chose and tell a bot + # member apart from a human. UserLiteSerializer intentionally omits + # these (it is shared across many endpoints), so add them per-row here. + user_data["username"] = member.username + user_data["is_bot"] = member.is_bot + user_data["bot_type"] = member.bot_type users_with_roles.append(user_data) return Response(users_with_roles, status=status.HTTP_200_OK) diff --git a/apps/api/plane/api/views/service_account.py b/apps/api/plane/api/views/service_account.py index 33ae0ccec28..e11eb07666e 100644 --- a/apps/api/plane/api/views/service_account.py +++ b/apps/api/plane/api/views/service_account.py @@ -2,9 +2,6 @@ # SPDX-License-Identifier: AGPL-3.0-only # See the LICENSE file for details. -# Django imports -from django.db import IntegrityError - # Third party imports from rest_framework import status from rest_framework.response import Response @@ -21,7 +18,7 @@ ServiceAccountTokenRotateSerializer, ServiceAccountTokenSerializer, ) -from plane.db.models import APIToken, BotTypeEnum, User, Workspace, WorkspaceMember +from plane.db.models import APIToken, BotTypeEnum, Workspace, WorkspaceMember from plane.middleware.logger import redact_response_body from plane.utils.permissions import WorkspaceOwnerPermission from plane.utils.openapi import ( @@ -38,11 +35,9 @@ mint_service_account_token, rotate_service_account_token, ServiceAccountTokenError, + ServiceAccountUsernameConflictError, ) -# Machine-readable error surfaced when a caller-chosen username is already taken. -USERNAME_CONFLICT = {"error": "A user with this username already exists.", "code": "USERNAME_ALREADY_EXISTS"} - # Path parameters shared by the service-account lifecycle endpoints. SERVICE_ACCOUNT_ID_PARAMETER = OpenApiParameter( name="user_id", @@ -115,7 +110,12 @@ class ServiceAccountAPIEndpoint(BaseAPIView): 401: UNAUTHORIZED_RESPONSE, 403: FORBIDDEN_RESPONSE, 404: NOT_FOUND_RESPONSE, - 409: OpenApiResponse(description="A user with the requested username already exists"), + 409: OpenApiResponse( + description=( + "The requested username belongs to an active account or a non-service user. " + "(A decommissioned service account with the same username is reactivated, not rejected.)" + ) + ), }, ) def post(self, request, slug): @@ -130,13 +130,6 @@ def post(self, request, slug): serializer.is_valid(raise_exception=True) data = serializer.validated_data - username = data.get("username") - # Reject a taken username with a machine-readable code — never silently - # mutate it into a unique one. The insert below is still wrapped so a - # race between this check and the create is reported the same way. - if username and User.objects.filter(username=username).exists(): - return Response(USERNAME_CONFLICT, status=status.HTTP_409_CONFLICT) - try: service_account = create_service_account( workspace=workspace, @@ -144,20 +137,18 @@ def post(self, request, slug): role=data["role"], # None (omitted) → generated default; an explicit "" is preserved. description=data.get("description"), - username=username, + username=data.get("username"), display_name=data.get("display_name"), ) - except IntegrityError: - # The only caller-controlled unique field here is the username - # (email/token are server-generated). If it is now taken — including - # a race that slipped past the pre-check — report the conflict. Any - # other IntegrityError is unexpected and must not be mislabeled, so - # let it surface via BaseAPIView.handle_exception. The helper's - # @transaction.atomic has already rolled back, so this SELECT runs on - # a clean connection. - if username and User.objects.filter(username=username).exists(): - return Response(USERNAME_CONFLICT, status=status.HTTP_409_CONFLICT) - raise + except ServiceAccountUsernameConflictError as exc: + # The username belongs to an active account, a non-service user, or a + # service account from another workspace — never silently mutated into a + # unique one. (A decommissioned service account OF THIS workspace with + # this username is reactivated by the helper instead.) The helper also + # classifies the brand-new-username insert race as this same conflict, + # so any IntegrityError that still reaches BaseAPIView.handle_exception + # is genuinely unexpected and is reported there rather than mislabeled. + return Response({"error": str(exc), "code": exc.code}, status=exc.status_code) response = ServiceAccountSerializer( { diff --git a/apps/api/plane/db/management/commands/create_service_account.py b/apps/api/plane/db/management/commands/create_service_account.py index b95b4ab66a5..ad34d82aa37 100644 --- a/apps/api/plane/db/management/commands/create_service_account.py +++ b/apps/api/plane/db/management/commands/create_service_account.py @@ -11,7 +11,9 @@ from plane.utils.service_account import ( DEFAULT_SERVICE_ACCOUNT_ROLE, SERVICE_ACCOUNT_ROLES, + ServiceAccountUsernameConflictError, create_service_account, + is_reactivatable_service_account, ) @@ -88,12 +90,21 @@ def handle(self, *args, **options): if value and len(value) > limit: raise CommandError(f"{flag} must be at most {limit} characters") - if email and User.objects.filter(email=email).exists(): + # Reactivation is keyed on --username and PRESERVES the existing account's + # email (--email is ignored on that path), so the email-uniqueness guard + # must not fire against the very account being revived. Only enforce it on + # the fresh-create path. + reactivating = username is not None and is_reactivatable_service_account( + User.objects.filter(username=username).first(), workspace + ) + if email and not reactivating and User.objects.filter(email=email).exists(): raise CommandError(f"A user with email '{email}' already exists") - if username and User.objects.filter(username=username).exists(): - raise CommandError(f"A user with username '{username}' already exists") - + # No username pre-check: a --username that already belongs to a + # decommissioned service account is REACTIVATED in place by the helper + # (not rejected), so a retired seat can be re-provisioned by its stable + # username. The helper raises ServiceAccountUsernameConflictError only for + # a genuine conflict (an active account, or a non-service user). try: service_account = create_service_account( workspace=workspace, @@ -104,6 +115,8 @@ def handle(self, *args, **options): username=username, display_name=display_name, ) + except ServiceAccountUsernameConflictError as exc: + raise CommandError(str(exc)) except IntegrityError as exc: # A concurrent insert (email/username race that slipped past the checks # above) or an extremely unlikely synthetic collision surfaces here — @@ -113,7 +126,8 @@ def handle(self, *args, **options): raise CommandError(f"Could not create the service account — the email or username is already in use: {exc}") user = service_account.user - self.stdout.write(self.style.SUCCESS("Service account created successfully")) + action = "reactivated" if service_account.reactivated else "created" + self.stdout.write(self.style.SUCCESS(f"Service account {action} successfully")) self.stdout.write(f" user_id : {user.id}") self.stdout.write(f" username : {user.username}") self.stdout.write(f" display_name: {user.display_name}") diff --git a/apps/api/plane/tests/contract/api/test_service_account.py b/apps/api/plane/tests/contract/api/test_service_account.py index 76db085ef2a..105315fccaf 100644 --- a/apps/api/plane/tests/contract/api/test_service_account.py +++ b/apps/api/plane/tests/contract/api/test_service_account.py @@ -271,6 +271,82 @@ def test_command_blank_name_errors(self, workspace): with pytest.raises(CommandError): call_command("create_service_account", workspace=workspace.slug, name="", role="admin") + @pytest.mark.django_db + def test_command_reactivates_decommissioned_account(self, workspace): + """Test re-running with a decommissioned username revives the same identity""" + from io import StringIO + + from plane.utils.service_account import decommission_service_account + + call_command("create_service_account", workspace=workspace.slug, name="Runner", role="admin", username="runner") + user = _service_account_for(workspace).member + original_id = user.id + + # Retire it, then re-provision by the same stable username. + decommission_service_account(user=user, workspace=workspace) + user.refresh_from_db() + assert user.is_active is False + + out = StringIO() + call_command( + "create_service_account", + workspace=workspace.slug, + name="Runner", + role="member", + username="runner", + stdout=out, + ) + + member = _service_account_for(workspace) + # Same user id — the seat is re-provisioned, not duplicated. + assert member.member.id == original_id + assert member.member.is_active is True + # Reactivated at the newly requested role, with exactly one live membership. + assert member.role == 15 + assert WorkspaceMember.all_objects.filter(member=member.member, workspace=workspace).count() == 1 + # A single fresh, active token; the command reports the reactivation. + assert APIToken.objects.filter(user=member.member, is_active=True).count() == 1 + assert "reactivated successfully" in out.getvalue() + + @pytest.mark.django_db + def test_command_reactivation_with_email_is_not_blocked(self, workspace): + """Test passing --email while reactivating does not false-trip the email guard""" + from io import StringIO + + from plane.utils.service_account import decommission_service_account + + call_command( + "create_service_account", + workspace=workspace.slug, + name="Runner", + role="admin", + username="runner2", + email="runner2@bots.example.com", + ) + user = _service_account_for(workspace).member + assert user.email == "runner2@bots.example.com" + decommission_service_account(user=user, workspace=workspace) + + # Re-provision by username, passing the SAME --email (the account's own, + # preserved value). Reactivation is keyed on --username and preserves email, + # so the email uniqueness guard must not fire against the very account being + # revived — previously this raised "A user with email ... already exists". + out = StringIO() + call_command( + "create_service_account", + workspace=workspace.slug, + name="Runner", + role="member", + username="runner2", + email="runner2@bots.example.com", + stdout=out, + ) + + member = _service_account_for(workspace) + assert member.member.id == user.id + assert member.member.is_active is True + assert "reactivated successfully" in out.getvalue() + @pytest.mark.contract class TestServiceAccountEndpoint: @@ -416,18 +492,21 @@ def test_duplicate_username_is_rejected_with_code(self, api_key_client, workspac @pytest.mark.django_db def test_username_race_returns_409(self, api_key_client, workspace): - """Test a username taken after the pre-check still returns 409""" - # Simulate a race: the username is free at the pre-check, then another - # actor creates it and our insert raises IntegrityError. The scoped - # handler re-checks, sees it now exists, and returns the same 409. - from django.db import IntegrityError - - def racing_create(**kwargs): - """Take the username, then fail as the database would.""" - User.objects.create(username="raced", email="racer@plane.so") - raise IntegrityError("duplicate key value violates unique constraint") - - with patch("plane.api.views.service_account.create_service_account", side_effect=racing_create): + """Test a brand-new username taken between the helper's check and insert returns 409""" + # Simulate a race: the username is free when create_service_account checks + # it, then a concurrent actor inserts it just before our User insert, so the + # DB raises IntegrityError. The helper's savepoint classifies that at its + # source as a username conflict (409), rather than letting it surface as an + # opaque error. Inject the racing insert via set_unusable_password, which + # runs immediately before the User row is saved. + original = User.set_unusable_password + + def racing(user_self): + if not User.objects.filter(username="raced").exists(): + User.objects.create(username="raced", email="racer@plane.so") + original(user_self) + + with patch.object(User, "set_unusable_password", racing): response = api_key_client.post( self._url(workspace.slug), {"name": "Race", "role": "admin", "username": "raced"}, @@ -498,33 +577,266 @@ def test_explicit_empty_description_is_preserved(self, api_key_client, workspace assert response.status_code == status.HTTP_201_CREATED, response.data assert APIToken.objects.get(user_id=response.data["id"]).description == "" + def _detail_url(self, slug: str, user_id) -> str: + """Helper to get the service-account detail (decommission) URL""" + return f"/api/v1/workspaces/{slug}/service-accounts/{user_id}/" + + @pytest.mark.django_db + def test_create_reactivates_decommissioned_account(self, api_key_client, workspace): + """Test creating with a decommissioned account's username revives it in place""" + # Provision, capture identity + token. + first = api_key_client.post( + self._url(workspace.slug), + {"name": "Reconciler", "role": "admin", "username": "reconciler"}, + format="json", + ) + assert first.status_code == status.HTTP_201_CREATED, first.data + user_id = first.data["id"] + old_token = first.data["token"] + + # Decommission it — memberships removed, user + tokens deactivated. + decommission = api_key_client.delete(self._detail_url(workspace.slug, user_id)) + assert decommission.status_code == status.HTTP_204_NO_CONTENT + assert _client_for_token(old_token).get(USERS_ME_URL).status_code in ( + status.HTTP_401_UNAUTHORIZED, + status.HTTP_403_FORBIDDEN, + ) + + # Re-create with the SAME username → reactivates the same identity, at the + # newly requested role, with a fresh token. + second = api_key_client.post( + self._url(workspace.slug), + {"name": "Reconciler", "role": "member", "username": "reconciler"}, + format="json", + ) + assert second.status_code == status.HTTP_201_CREATED, second.data + # Same user id — the retired seat is re-provisioned, not a new user. + assert str(second.data["id"]) == str(user_id) + assert second.data["role"] == 15 + + # New token authenticates; the old one stays dead. + new_token = second.data["token"] + assert new_token != old_token + assert _client_for_token(new_token).get(USERS_ME_URL).status_code == status.HTTP_200_OK + assert _client_for_token(old_token).get(USERS_ME_URL).status_code in ( + status.HTTP_401_UNAUTHORIZED, + status.HTTP_403_FORBIDDEN, + ) + + # The user is active again with a single live membership at the new role + # (the soft-deleted row was restored in place, not duplicated). + user = User.objects.get(id=user_id) + assert user.is_active is True + assert WorkspaceMember.all_objects.filter(member=user, workspace=workspace).count() == 1 + member = WorkspaceMember.objects.get(member=user, workspace=workspace) + assert member.is_active is True + assert member.role == 15 + # Exactly one active token now (the reactivation mint); old ones are inactive. + assert APIToken.objects.filter(user=user, is_active=True).count() == 1 + + @pytest.mark.django_db + def test_create_active_service_username_still_conflicts(self, api_key_client, workspace, service_account): + """Test an ACTIVE service account's username still 409s (no reactivation)""" + # service_account is a live SERVICE account with username "reconcile-bot". + response = api_key_client.post( + self._url(workspace.slug), + {"name": "Dup", "role": "admin", "username": "reconcile-bot"}, + format="json", + ) + assert response.status_code == status.HTTP_409_CONFLICT, response.data + assert response.data["code"] == "USERNAME_ALREADY_EXISTS" + # The live account is untouched: still active, still one active token. + service_account.user.refresh_from_db() + assert service_account.user.is_active is True + assert APIToken.objects.filter(user=service_account.user, is_active=True).count() == 1 + + @pytest.mark.django_db + def test_create_human_username_conflicts(self, api_key_client, workspace): + """Test a human's username 409s and is never reactivated as a service account""" + User.objects.create(username="a-human", email="ahuman@plane.so", is_active=False) + response = api_key_client.post( + self._url(workspace.slug), + {"name": "X", "role": "admin", "username": "a-human"}, + format="json", + ) + # Even an INACTIVE human is not a reactivatable service account. + assert response.status_code == status.HTTP_409_CONFLICT, response.data + assert response.data["code"] == "USERNAME_ALREADY_EXISTS" + assert not WorkspaceMember.objects.filter(workspace=workspace, member__bot_type=BotTypeEnum.SERVICE).exists() + + @pytest.mark.django_db + def test_create_non_service_bot_username_conflicts(self, api_key_client, workspace): + """Test an inactive NON-service bot's username 409s (only SERVICE bots reactivate)""" + User.objects.create( + username="seed-bot", + email="seed2@plane.so", + is_bot=True, + bot_type=BotTypeEnum.WORKSPACE_SEED, + is_active=False, + ) + response = api_key_client.post( + self._url(workspace.slug), + {"name": "X", "role": "admin", "username": "seed-bot"}, + format="json", + ) + assert response.status_code == status.HTTP_409_CONFLICT, response.data + assert response.data["code"] == "USERNAME_ALREADY_EXISTS" + + @pytest.mark.django_db + def test_reactivation_is_scoped_to_originating_workspace(self, api_key_client, workspace): + """Test a decommissioned account's username cannot be revived from another workspace""" + # Provision + decommission "shared-bot" in `workspace`. + first = api_key_client.post( + self._url(workspace.slug), + {"name": "Shared", "role": "admin", "username": "shared-bot"}, + format="json", + ) + assert first.status_code == status.HTTP_201_CREATED, first.data + user_id = first.data["id"] + assert ( + api_key_client.delete(self._detail_url(workspace.slug, user_id)).status_code == status.HTTP_204_NO_CONTENT + ) + + # A DIFFERENT workspace, with its own admin, tries to claim the same globally + # unique username. A username belongs to exactly one workspace's service + # account, so this must NOT resurrect W1's identity into W2. + other_owner = User.objects.create(username="w2-owner", email="w2owner@plane.so") + other_ws = Workspace.objects.create(name="W2", owner=other_owner, slug="w2-ws") + WorkspaceMember.objects.create(workspace=other_ws, member=other_owner, role=20, is_active=True) + w2_token = APIToken.objects.create(user=other_owner, label="w2", token=generate_token()) + + response = _client_for_token(w2_token.token).post( + self._url(other_ws.slug), + {"name": "Shared", "role": "admin", "username": "shared-bot"}, + format="json", + ) + # Cross-workspace resurrection is refused as a genuine conflict. + assert response.status_code == status.HTTP_409_CONFLICT, response.data + assert response.data["code"] == "USERNAME_ALREADY_EXISTS" + # W1's decommissioned identity is untouched: still inactive, no W2 membership + # or token was created for it. + w1_user = User.objects.get(id=user_id) + assert w1_user.is_active is False + assert not WorkspaceMember.all_objects.filter(member=w1_user, workspace=other_ws).exists() + assert not APIToken.objects.filter(user=w1_user, workspace=other_ws).exists() + + +@pytest.mark.contract +class TestServiceAccountMembersListing: + """The public members list exposes username + bot identity for discoverability.""" + + def _url(self, slug: str) -> str: + """Helper to get the public workspace members-list URL""" + return f"/api/v1/workspaces/{slug}/members/" + + @pytest.mark.django_db + def test_service_account_row_carries_username_and_bot_identity( + self, api_key_client, workspace, service_account, create_user + ): + """Test a service-account row carries its username + is_bot/bot_type; a human is is_bot=false""" + response = api_key_client.get(self._url(workspace.slug)) + assert response.status_code == status.HTTP_200_OK, response.data + + rows_by_username = {row["username"]: row for row in response.data} + rows_by_id = {str(row["id"]): row for row in response.data} + + # The service account is discoverable by the stable username it chose, and + # is flagged as a SERVICE bot. + assert "reconcile-bot" in rows_by_username + bot_row = rows_by_username["reconcile-bot"] + assert str(bot_row["id"]) == str(service_account.user.id) + assert bot_row["is_bot"] is True + assert bot_row["bot_type"] == BotTypeEnum.SERVICE + + # The human admin row is present, carries a username, and is not a bot. + human_row = rows_by_id[str(create_user.id)] + assert human_row["is_bot"] is False + assert "username" in human_row + @pytest.mark.contract class TestServiceAccountHelper: """Direct tests of the shared create_service_account helper.""" @pytest.mark.django_db - def test_duplicate_username_rolls_back_atomically(self, workspace): - """Test a duplicate username rolls the whole creation back""" - # A genuine (non-mocked) unique violation: the helper is @transaction.atomic, - # so the failed second creation must leave nothing behind. + def test_email_collision_rolls_back_atomically(self, workspace): + """Test a genuine DB unique violation rolls the whole creation back""" + # A caller-provided email that already exists triggers a real IntegrityError + # from the User insert; the helper is @transaction.atomic, so the failed + # creation must leave nothing behind. (A duplicate *username* is caught + # earlier with ServiceAccountUsernameConflictError — see the reactivation + # and conflict tests — so an email collision exercises the DB-level rollback.) from django.db import IntegrityError from plane.utils.service_account import create_service_account - create_service_account(workspace=workspace, name="First", username="dup") + User.objects.create(username="human-x", email="taken@plane.so") users_before = User.objects.count() members_before = WorkspaceMember.objects.count() tokens_before = APIToken.objects.count() with pytest.raises(IntegrityError): + create_service_account(workspace=workspace, name="Second", username="brand-new", email="taken@plane.so") + + assert User.objects.count() == users_before + assert WorkspaceMember.objects.count() == members_before + assert APIToken.objects.count() == tokens_before + + @pytest.mark.django_db + def test_active_service_username_raises_typed_conflict(self, workspace): + """Test re-using an ACTIVE service account's username raises a typed conflict, no leak""" + from plane.utils.service_account import ServiceAccountUsernameConflictError, create_service_account + + create_service_account(workspace=workspace, name="First", username="dup") + + users_before = User.objects.count() + members_before = WorkspaceMember.objects.count() + tokens_before = APIToken.objects.count() + + with pytest.raises(ServiceAccountUsernameConflictError) as exc_info: create_service_account(workspace=workspace, name="Second", username="dup") + assert exc_info.value.code == "USERNAME_ALREADY_EXISTS" + assert exc_info.value.status_code == 409 + # The conflict is detected before any write — nothing is created. assert User.objects.count() == users_before assert WorkspaceMember.objects.count() == members_before assert APIToken.objects.count() == tokens_before + @pytest.mark.django_db + def test_reactivation_preserves_identity_and_flags_result(self, workspace): + """Test reactivating a decommissioned account preserves identity and sets reactivated""" + from plane.utils.service_account import ( + create_service_account, + decommission_service_account, + ) + + original = create_service_account( + workspace=workspace, name="Ident", role="admin", username="ident-bot", display_name="Ident Bot" + ) + original_email = original.user.email + original_id = original.user.id + + decommission_service_account(user=original.user, workspace=workspace) + + revived = create_service_account(workspace=workspace, name="Ident Renamed", role="member", username="ident-bot") + + # Same identity, revived in place — username/email/display_name preserved + # (reactivation is keyed on the stable username, not the new name). + assert revived.reactivated is True + assert revived.user.id == original_id + assert revived.user.is_active is True + assert revived.user.username == "ident-bot" + assert revived.user.email == original_email + assert revived.user.display_name == "Ident Bot" + # Membership restored in place at the newly requested role (no duplicate row). + assert revived.member.role == 15 + assert WorkspaceMember.all_objects.filter(member=revived.user, workspace=workspace).count() == 1 + # A fresh token that differs from the (now-inactive) original. + assert revived.token != original.token + assert revived.api_token.is_active is True + @pytest.mark.django_db def test_omitted_description_uses_generated_default(self, workspace): """Test an omitted description falls back to the generated default""" diff --git a/apps/api/plane/utils/openapi/examples.py b/apps/api/plane/utils/openapi/examples.py index 20aff18958a..9817d432821 100644 --- a/apps/api/plane/utils/openapi/examples.py +++ b/apps/api/plane/utils/openapi/examples.py @@ -627,6 +627,9 @@ "email": "john.doe@example.com", "avatar": "https://example.com/avatar.jpg", "role": 20, + "username": "john.doe", + "is_bot": False, + "bot_type": None, }, { "id": "550e8400-e29b-41d4-a716-446655440001", @@ -636,6 +639,21 @@ "email": "jane.smith@example.com", "avatar": "https://example.com/avatar2.jpg", "role": 15, + "username": "jane.smith", + "is_bot": False, + "bot_type": None, + }, + { + "id": "550e8400-e29b-41d4-a716-446655440002", + "first_name": "CI Provisioner", + "last_name": "", + "display_name": "CI Provisioner", + "email": "svc_9d89245e45164385a00e42ff6056cbce@service.plane.local", + "avatar": "", + "role": 20, + "username": "ci-provisioner", + "is_bot": True, + "bot_type": "SERVICE", }, ], ) diff --git a/apps/api/plane/utils/service_account.py b/apps/api/plane/utils/service_account.py index 73be7da7d8a..f2eb79f1b37 100644 --- a/apps/api/plane/utils/service_account.py +++ b/apps/api/plane/utils/service_account.py @@ -20,7 +20,7 @@ from dataclasses import dataclass # Django imports -from django.db import transaction +from django.db import IntegrityError, transaction from django.utils import timezone # Third party imports @@ -83,6 +83,22 @@ class SourceExpiryElapsedError(ServiceAccountTokenError): status_code = 400 +class ServiceAccountUsernameConflictError(Exception): + """Raised when a requested username cannot be provisioned as a service account. + + The username belongs to an *active* account, or to a user that is not a + reactivatable service account (a human, or a non-``SERVICE`` bot). A + *decommissioned* (inactive ``SERVICE``) account with the same username is + reactivated in place instead of rejected — see :func:`create_service_account`. + + Carries a machine-readable ``code`` and the ``status_code`` the HTTP layer + surfaces, mirroring :class:`ServiceAccountTokenError`. + """ + + code = "USERNAME_ALREADY_EXISTS" + status_code = 409 + + @dataclass class ServiceAccount: """Result of :func:`create_service_account`. @@ -90,11 +106,16 @@ class ServiceAccount: ``token`` is the *plaintext* API key. Plane stores API tokens verbatim (the authentication layer looks them up by exact match), so this is the value the caller must persist — it is surfaced here exactly once at creation time. + + ``reactivated`` is ``True`` when the account was revived from a decommissioned + identity of the same username rather than freshly created; ``user.id`` is then + the pre-existing id (see :func:`create_service_account`). """ user: User member: WorkspaceMember api_token: APIToken + reactivated: bool = False @property def token(self) -> str: @@ -102,6 +123,29 @@ def token(self) -> str: return self.api_token.token +def is_reactivatable_service_account(user: User | None, workspace: Workspace) -> bool: + """Whether ``user`` is a decommissioned service account revivable in ``workspace``. + + True only when ``user`` is an INACTIVE ``bot_type=SERVICE`` account that was a + member of ``workspace`` (a decommissioned seat, whose membership row survives + soft-deleted, so it is visible via ``all_objects``). A username is GLOBALLY + unique, so it may name a SERVICE account that belonged to a DIFFERENT workspace; + that account is *not* revivable here — reviving it would resurrect a + foreign-tenant identity under a shared user id (leaking its email/display_name, + seizing its username, and conflating attribution), breaking the "a service + account belongs to exactly one workspace" invariant. Such a cross-workspace + collision, an *active* account, and any non-service user all return ``False`` so + the caller treats them as a genuine conflict. + """ + return bool( + user is not None + and user.is_bot + and user.bot_type == BotTypeEnum.SERVICE + and not user.is_active + and WorkspaceMember.all_objects.filter(workspace=workspace, member=user).exists() + ) + + def resolve_service_account_role(role: str | int) -> int: """Coerce a role name (or raw value) to a WorkspaceMember.role integer.""" if isinstance(role, int): @@ -129,7 +173,7 @@ def create_service_account( username: str | None = None, display_name: str | None = None, ) -> ServiceAccount: - """Create a service account in ``workspace`` and mint its API token. + """Create (or reactivate) a service account in ``workspace`` and mint its API token. Creates, in a single transaction: @@ -140,12 +184,27 @@ def create_service_account( workspace-scoped) whose plaintext value is returned on the result. ``username`` and ``display_name`` are optional caller-chosen identity fields. - ``username`` must be globally unique (a collision raises ``IntegrityError`` - from the DB insert — the caller is expected to check for it and surface a - readable error); when omitted a synthetic ``svc_`` value is generated. - ``display_name`` is what the workspace members UI shows; it falls back to - ``name`` when omitted. ``description`` is the token's description; ``None`` - (omitted) applies a generated default, while an explicit ``""`` is preserved. + ``username`` must be globally unique; when omitted a synthetic ``svc_`` + value is generated. ``display_name`` is what the workspace members UI shows; + it falls back to ``name`` when omitted. ``description`` is the token's + description; ``None`` (omitted) applies a generated default, while an explicit + ``""`` is preserved. + + **Reactivation.** If ``username`` is supplied and already belongs to a + *decommissioned* service account of **this** workspace (an inactive + ``bot_type=SERVICE`` user that was a member of ``workspace``), that identity is + revived in place instead of colliding: the user is reactivated + (``is_active=True``), its workspace membership is restored at ``role``, and a + fresh token is minted and returned — all under the pre-existing user id, so a + retired seat can be re-provisioned by its stable username. Its identity fields + (username/email/display_name) are preserved; only the membership role and a new + token are (re)provisioned, and the result carries ``reactivated=True``. A + username owned by an *active* account, by any non-service user (a human or a + non-``SERVICE`` bot), or by a service account that belonged to a *different* + workspace is a genuine conflict and raises + :class:`ServiceAccountUsernameConflictError` (see + :func:`is_reactivatable_service_account` — reactivation never crosses a + workspace boundary). No email is sent and no password is ever round-tripped. ``is_bot=True`` additionally blocks the interactive login/signup flow @@ -154,10 +213,40 @@ def create_service_account( """ role_value = resolve_service_account_role(role) - # A service account never logs in, so a caller-omitted username/email are - # internal, unique identifiers rather than human contact addresses. The - # synthetic email is always derived from a fresh uuid (never the caller's - # username) so it stays valid and unique regardless of the username. + # None means "omitted" → apply the generated default; an explicit "" is a + # deliberate empty description and is preserved (do not use `or`, which would + # conflate blank with omitted). Resolved once so the fresh-create and the + # reactivation paths share it. + token_description = f"Service account token for {name}" if description is None else description + + # Reactivation: a caller-chosen username that already belongs to a + # DECOMMISSIONED service account OF THIS WORKSPACE revives that identity in + # place rather than colliding, so a retired seat can be re-provisioned by its + # stable username. select_for_update serializes concurrent re-provisions of + # the same username. + if username: + existing = User.objects.select_for_update().filter(username=username).first() + if existing is not None: + if not is_reactivatable_service_account(existing, workspace): + # Owned by an active account, a non-service user (human/other bot), + # or a SERVICE account from a DIFFERENT workspace — none revivable + # here. The message is intentionally generic so a cross-workspace + # probe cannot confirm a username exists elsewhere. + raise ServiceAccountUsernameConflictError("A user with this username already exists.") + return _reactivate_service_account( + user=existing, + workspace=workspace, + role_value=role_value, + label=name, + description=token_description, + ) + + # Fresh creation. A service account never logs in, so a caller-omitted + # username/email are internal, unique identifiers rather than human contact + # addresses. The synthetic email is always derived from a fresh uuid (never + # the caller's username) so it stays valid and unique regardless of the + # username. + requested_username = username # caller-provided (or None) — used to classify a race below unique = uuid.uuid4().hex if not username: username = f"svc_{unique}" @@ -187,7 +276,19 @@ def create_service_account( # No password round-trip: the account authenticates only via its API token, # so give it an unusable password that can never be used to log in. user.set_unusable_password() - user.save() + # A concurrent create of the same brand-new caller-chosen username can win the + # race between the conflict check above and this insert. Wrap the insert in a + # savepoint so the unique-violation IntegrityError is classified at its source + # (a username conflict) without poisoning the outer transaction; any OTHER + # IntegrityError (e.g. a caller-supplied email collision) propagates unchanged + # for the caller to surface. + try: + with transaction.atomic(): + user.save() + except IntegrityError: + if requested_username and User.objects.filter(username=requested_username).exists(): + raise ServiceAccountUsernameConflictError("A user with this username already exists.") from None + raise member = WorkspaceMember.objects.create( workspace=workspace, @@ -196,22 +297,73 @@ def create_service_account( company_role="", ) - # None means "omitted" → apply the generated default; an explicit "" is a - # deliberate empty description and is preserved (do not use `or`, which would - # conflate blank with omitted). - if description is None: - description = f"Service account token for {name}" - api_token = mint_service_account_token( user=user, workspace=workspace, label=name, - description=description, + description=token_description, ) return ServiceAccount(user=user, member=member, api_token=api_token) +def _reactivate_service_account( + *, user: User, workspace: Workspace, role_value: int, label: str, description: str +) -> ServiceAccount: + """Revive a decommissioned service account in place. + + Reactivates the ``User`` (``is_active=True``), restores its workspace + membership at ``role_value``, and mints a fresh token. Identity fields + (username/email/display_name) are preserved — reactivation is keyed on the + stable username. Previously-deactivated tokens stay inactive; the new one is + returned. Called only from :func:`create_service_account` inside its + transaction. + """ + user.is_active = True + user.save(update_fields=["is_active"]) + + member = _provision_workspace_membership(workspace=workspace, user=user, role_value=role_value) + + api_token = mint_service_account_token( + user=user, + workspace=workspace, + label=label, + description=description, + ) + return ServiceAccount(user=user, member=member, api_token=api_token, reactivated=True) + + +def _provision_workspace_membership(*, workspace: Workspace, user: User, role_value: int) -> WorkspaceMember: + """Bind ``user`` to ``workspace`` at ``role_value``, restoring a prior membership. + + Decommissioning SOFT-deletes the ``WorkspaceMember`` (sets ``deleted_at``), so + a reactivated account has a soft-deleted membership row. The partial unique + index on ``(workspace, member) WHERE deleted_at IS NULL`` permits only one + live row, so restore the existing row in place rather than inserting a + duplicate: + + * a live row (never soft-deleted) is reactivated at the requested role; + * otherwise a soft-deleted row is restored (``deleted_at`` cleared); + * otherwise a new membership is created. + """ + live = WorkspaceMember.objects.filter(workspace=workspace, member=user).first() + if live is not None: + live.role = role_value + live.is_active = True + live.save() + return live + + removed = WorkspaceMember.all_objects.filter(workspace=workspace, member=user).order_by("-created_at").first() + if removed is not None: + removed.deleted_at = None + removed.is_active = True + removed.role = role_value + removed.save() + return removed + + return WorkspaceMember.objects.create(workspace=workspace, member=user, role=role_value, company_role="") + + def mint_service_account_token(*, user, workspace, label=None, description="", expired_at=None) -> APIToken: """Mint an additional workspace-scoped bot token for a service account. diff --git a/docs/service-accounts.md b/docs/service-accounts.md index ef0dedf0c98..3d375773afa 100644 --- a/docs/service-accounts.md +++ b/docs/service-accounts.md @@ -36,6 +36,13 @@ authorization and attribution: it passes permission checks and appears as the `created_by`/`updated_by` actor on everything it writes, and the **public API** lists it (e.g. `GET /api/v1/workspaces/{slug}/members/` returns bot members). +Each row from `GET /api/v1/workspaces/{slug}/members/` carries `username`, +`is_bot`, and `bot_type` alongside the usual profile fields and `role`, so +external automation can **find a service account by the stable username it +chose** and tell a bot member apart from a human (`is_bot: true`, +`bot_type: "SERVICE"`). (The paginated `GET .../members-lite/` picker is a +separate, minimal endpoint; it is unchanged and already exposes `is_bot`.) + ## Roles | Name | Value | Capability | @@ -136,17 +143,24 @@ with the management command above. identity instead of the server-generated defaults: - **`username`** — a globally-unique handle. Use it to provision idempotently: - re-creating with a username that already exists is rejected with **`409 Conflict`** - and a machine-readable body `{"error": ..., "code": "USERNAME_ALREADY_EXISTS"}`; - the name is **never** silently mutated into a unique variant. Like every Plane - username it is bounded only by length (max 128 characters) — Plane applies no - charset validator to usernames (regular accounts get a random `uuid` handle), so - none is imposed here either. Omit it to get a synthetic `svc_` handle. + re-creating with a username that belongs to a **decommissioned** service account + **of this workspace** **reactivates** that account in place (see [Re-provisioning a + decommissioned account](#re-provisioning-a-decommissioned-account)); re-creating + with a username owned by an **active** account, by any **non-service** user (a human + or a non-`SERVICE` bot), or by a service account from **another workspace**, is + rejected with **`409 Conflict`** and a machine-readable body + `{"error": ..., "code": "USERNAME_ALREADY_EXISTS"}`. The name is **never** silently + mutated into a unique variant. Like every Plane username it is bounded only by + length (max 128 characters) — Plane applies no charset validator to usernames + (regular accounts get a random `uuid` handle), so none is imposed here either. + Omit it to get a synthetic `svc_` handle. - **`display_name`** — the label the workspace members UI shows. Omit it to fall back to `name`. -On the management command, a taken `--username` (or `--email`) fails with a clear -`CommandError` instead of a raw traceback. +On the management command, a `--username` owned by an active or non-service user +(and a taken `--email`) fails with a clear `CommandError` instead of a raw +traceback; a `--username` belonging to a decommissioned service account reactivates +it (the command prints `Service account reactivated successfully`). ## Managing tokens @@ -232,6 +246,36 @@ The operation is hard-guarded: it only applies to a service account (`is_bot=True` **and** `bot_type=SERVICE`). Attempting it on a human or any other bot returns `400 {"error": ..., "code": "NOT_A_SERVICE_ACCOUNT"}`. +### Re-provisioning a decommissioned account + +A decommissioned service account is **not** a dead end: because its `User` row is +kept, its username stays claimed. Re-creating a service account with that **same +username** — via `POST .../service-accounts/` or the management command — +**reactivates the retired identity in place** instead of returning `409`: + +- the `User` is re-activated (`is_active=True`); +- its workspace membership is restored at the **requested** role (the soft-deleted + membership row is revived, never duplicated); +- a **fresh** token is minted and returned once (the old, deactivated tokens stay + inactive). + +The response is a normal `201` and carries the **same `id`** as before, so a +retired seat can be re-provisioned by its stable username without minting a new +identity — historical `created_by`/`updated_by` attribution is preserved. Identity +fields other than the membership role (username/email/display_name) are kept as-is; +reactivation is keyed on the username, so a different `name`/`display_name` in the +re-create request does not rename the existing account, and the management command's +`--email` is ignored on this path (it never changes the revived account's email). + +Reactivation is **scoped to the workspace the account belonged to.** A username is +globally unique, so it may name a decommissioned service account from a *different* +workspace; that account is **not** revived here (which would resurrect a +foreign-tenant identity under a shared user id). Only a **decommissioned service +account of this workspace** reactivates — a username owned by an active account, by +a human or non-`SERVICE` bot, or by a service account from another workspace all +return `409 {"code": "USERNAME_ALREADY_EXISTS"}` (with a deliberately generic +message that does not confirm the username exists elsewhere). + ## Notes - Tokens are stored verbatim and matched exactly at authentication time; there is