From f740ba0ffed6a3d6e2343895c79ece1ab08c1ac8 Mon Sep 17 00:00:00 2001 From: Jens L Date: Thu, 22 Feb 2024 22:57:47 +0100 Subject: [PATCH] core: rework recovery API to return better error messages (#8655) Signed-off-by: Jens Langhammer --- authentik/core/api/users.py | 26 ++++++++--------------- authentik/core/tests/test_users_api.py | 26 ++++++++++++++--------- schema.yml | 26 ++++++++--------------- web/src/admin/groups/RelatedUserList.ts | 2 +- web/src/admin/users/UserListPage.ts | 2 +- web/src/admin/users/UserResetEmailForm.ts | 8 +++---- 6 files changed, 40 insertions(+), 50 deletions(-) diff --git a/authentik/core/api/users.py b/authentik/core/api/users.py index 3d56d378ca..6449d6760a 100644 --- a/authentik/core/api/users.py +++ b/authentik/core/api/users.py @@ -397,15 +397,14 @@ class UserViewSet(UsedByMixin, ModelViewSet): def get_queryset(self): # pragma: no cover return User.objects.all().exclude_anonymous().prefetch_related("ak_groups") - def _create_recovery_link(self) -> tuple[Optional[str], Optional[Token]]: + def _create_recovery_link(self) -> tuple[str, Token]: """Create a recovery link (when the current brand has a recovery flow set), that can either be shown to an admin or sent to the user directly""" brand: Brand = self.request._request.brand # Check that there is a recovery flow, if not return an error flow = brand.flow_recovery if not flow: - LOGGER.debug("No recovery flow set") - return None, None + raise ValidationError({"non_field_errors": "No recovery flow set."}) user: User = self.get_object() planner = FlowPlanner(flow) planner.allow_empty_flows = True @@ -417,8 +416,7 @@ class UserViewSet(UsedByMixin, ModelViewSet): }, ) except FlowNonApplicableException: - LOGGER.warning("Recovery flow not applicable to user") - return None, None + raise ValidationError({"non_field_errors": "Recovery flow not applicable to user"}) token, __ = FlowToken.objects.update_or_create( identifier=f"{user.uid}-password-reset", defaults={ @@ -563,16 +561,13 @@ class UserViewSet(UsedByMixin, ModelViewSet): @extend_schema( responses={ "200": LinkSerializer(many=False), - "404": LinkSerializer(many=False), }, + request=None, ) - @action(detail=True, pagination_class=None, filter_backends=[]) + @action(detail=True, pagination_class=None, filter_backends=[], methods=["POST"]) def recovery(self, request: Request, pk: int) -> Response: """Create a temporary link that a user can use to recover their accounts""" link, _ = self._create_recovery_link() - if not link: - LOGGER.debug("Couldn't create token") - return Response({"link": ""}, status=404) return Response({"link": link}) @permission_required("authentik_core.reset_user_password") @@ -587,27 +582,24 @@ class UserViewSet(UsedByMixin, ModelViewSet): ], responses={ "204": OpenApiResponse(description="Successfully sent recover email"), - "404": OpenApiResponse(description="Bad request"), }, + request=None, ) - @action(detail=True, pagination_class=None, filter_backends=[]) + @action(detail=True, pagination_class=None, filter_backends=[], methods=["POST"]) def recovery_email(self, request: Request, pk: int) -> Response: """Create a temporary link that a user can use to recover their accounts""" for_user: User = self.get_object() if for_user.email == "": LOGGER.debug("User doesn't have an email address") - return Response(status=404) + raise ValidationError({"non_field_errors": "User does not have an email address set."}) link, token = self._create_recovery_link() - if not link: - LOGGER.debug("Couldn't create token") - return Response(status=404) # Lookup the email stage to assure the current user can access it stages = get_objects_for_user( request.user, "authentik_stages_email.view_emailstage" ).filter(pk=request.query_params.get("email_stage")) if not stages.exists(): LOGGER.debug("Email stage does not exist/user has no permissions") - return Response(status=404) + raise ValidationError({"non_field_errors": "Email stage does not exist."}) email_stage: EmailStage = stages.first() message = TemplateEmailMessage( subject=_(email_stage.subject), diff --git a/authentik/core/tests/test_users_api.py b/authentik/core/tests/test_users_api.py index 5b168caca4..a2e7923693 100644 --- a/authentik/core/tests/test_users_api.py +++ b/authentik/core/tests/test_users_api.py @@ -60,10 +60,11 @@ class TestUsersAPI(APITestCase): def test_recovery_no_flow(self): """Test user recovery link (no recovery flow set)""" self.client.force_login(self.admin) - response = self.client.get( + response = self.client.post( reverse("authentik_api:user-recovery", kwargs={"pk": self.user.pk}) ) - self.assertEqual(response.status_code, 404) + self.assertEqual(response.status_code, 400) + self.assertJSONEqual(response.content, {"non_field_errors": "No recovery flow set."}) def test_set_password(self): """Test Direct password set""" @@ -84,7 +85,7 @@ class TestUsersAPI(APITestCase): brand.flow_recovery = flow brand.save() self.client.force_login(self.admin) - response = self.client.get( + response = self.client.post( reverse("authentik_api:user-recovery", kwargs={"pk": self.user.pk}) ) self.assertEqual(response.status_code, 200) @@ -92,16 +93,20 @@ class TestUsersAPI(APITestCase): def test_recovery_email_no_flow(self): """Test user recovery link (no recovery flow set)""" self.client.force_login(self.admin) - response = self.client.get( + response = self.client.post( reverse("authentik_api:user-recovery-email", kwargs={"pk": self.user.pk}) ) - self.assertEqual(response.status_code, 404) + self.assertEqual(response.status_code, 400) + self.assertJSONEqual( + response.content, {"non_field_errors": "User does not have an email address set."} + ) self.user.email = "foo@bar.baz" self.user.save() - response = self.client.get( + response = self.client.post( reverse("authentik_api:user-recovery-email", kwargs={"pk": self.user.pk}) ) - self.assertEqual(response.status_code, 404) + self.assertEqual(response.status_code, 400) + self.assertJSONEqual(response.content, {"non_field_errors": "No recovery flow set."}) def test_recovery_email_no_stage(self): """Test user recovery link (no email stage)""" @@ -112,10 +117,11 @@ class TestUsersAPI(APITestCase): brand.flow_recovery = flow brand.save() self.client.force_login(self.admin) - response = self.client.get( + response = self.client.post( reverse("authentik_api:user-recovery-email", kwargs={"pk": self.user.pk}) ) - self.assertEqual(response.status_code, 404) + self.assertEqual(response.status_code, 400) + self.assertJSONEqual(response.content, {"non_field_errors": "Email stage does not exist."}) def test_recovery_email(self): """Test user recovery link""" @@ -129,7 +135,7 @@ class TestUsersAPI(APITestCase): stage = EmailStage.objects.create(name="email") self.client.force_login(self.admin) - response = self.client.get( + response = self.client.post( reverse( "authentik_api:user-recovery-email", kwargs={"pk": self.user.pk}, diff --git a/schema.yml b/schema.yml index 5bd9751e58..38c0d04283 100644 --- a/schema.yml +++ b/schema.yml @@ -4896,8 +4896,8 @@ paths: $ref: '#/components/schemas/GenericError' description: '' /core/users/{id}/recovery/: - get: - operationId: core_users_recovery_retrieve + post: + operationId: core_users_recovery_create description: Create a temporary link that a user can use to recover their accounts parameters: - in: path @@ -4917,12 +4917,6 @@ paths: schema: $ref: '#/components/schemas/Link' description: '' - '404': - content: - application/json: - schema: - $ref: '#/components/schemas/Link' - description: '' '400': content: application/json: @@ -4936,8 +4930,8 @@ paths: $ref: '#/components/schemas/GenericError' description: '' /core/users/{id}/recovery_email/: - get: - operationId: core_users_recovery_email_retrieve + post: + operationId: core_users_recovery_email_create description: Create a temporary link that a user can use to recover their accounts parameters: - in: query @@ -4958,8 +4952,6 @@ paths: responses: '204': description: Successfully sent recover email - '404': - description: Bad request '400': content: application/json: @@ -17893,7 +17885,7 @@ paths: schema: type: string format: uuid - description: A UUID string identifying this connection token. + description: A UUID string identifying this RAC Connection token. required: true tags: - rac @@ -17927,7 +17919,7 @@ paths: schema: type: string format: uuid - description: A UUID string identifying this connection token. + description: A UUID string identifying this RAC Connection token. required: true tags: - rac @@ -17967,7 +17959,7 @@ paths: schema: type: string format: uuid - description: A UUID string identifying this connection token. + description: A UUID string identifying this RAC Connection token. required: true tags: - rac @@ -18006,7 +17998,7 @@ paths: schema: type: string format: uuid - description: A UUID string identifying this connection token. + description: A UUID string identifying this RAC Connection token. required: true tags: - rac @@ -18037,7 +18029,7 @@ paths: schema: type: string format: uuid - description: A UUID string identifying this connection token. + description: A UUID string identifying this RAC Connection token. required: true tags: - rac diff --git a/web/src/admin/groups/RelatedUserList.ts b/web/src/admin/groups/RelatedUserList.ts index abde678fbe..933e80c595 100644 --- a/web/src/admin/groups/RelatedUserList.ts +++ b/web/src/admin/groups/RelatedUserList.ts @@ -305,7 +305,7 @@ export class RelatedUserList extends WithBrandConfig(WithCapabilitiesConfig(Tabl class="pf-m-secondary" .apiRequest=${() => { return new CoreApi(DEFAULT_CONFIG) - .coreUsersRecoveryRetrieve({ + .coreUsersRecoveryCreate({ id: item.pk, }) .then((rec) => { diff --git a/web/src/admin/users/UserListPage.ts b/web/src/admin/users/UserListPage.ts index e05e7700e1..28d269228e 100644 --- a/web/src/admin/users/UserListPage.ts +++ b/web/src/admin/users/UserListPage.ts @@ -42,7 +42,7 @@ import { CoreApi, ResponseError, SessionUser, User, UserPath } from "@goauthenti export const requestRecoveryLink = (user: User) => new CoreApi(DEFAULT_CONFIG) - .coreUsersRecoveryRetrieve({ + .coreUsersRecoveryCreate({ id: user.pk, }) .then((rec) => diff --git a/web/src/admin/users/UserResetEmailForm.ts b/web/src/admin/users/UserResetEmailForm.ts index bd450a57bd..a11f17d440 100644 --- a/web/src/admin/users/UserResetEmailForm.ts +++ b/web/src/admin/users/UserResetEmailForm.ts @@ -10,7 +10,7 @@ import { customElement, property } from "lit/decorators.js"; import { CoreApi, - CoreUsersRecoveryEmailRetrieveRequest, + CoreUsersRecoveryEmailCreateRequest, Stage, StagesAllListRequest, StagesApi, @@ -18,7 +18,7 @@ import { } from "@goauthentik/api"; @customElement("ak-user-reset-email-form") -export class UserResetEmailForm extends Form { +export class UserResetEmailForm extends Form { @property({ attribute: false }) user!: User; @@ -26,9 +26,9 @@ export class UserResetEmailForm extends Form { + async send(data: CoreUsersRecoveryEmailCreateRequest): Promise { data.id = this.user.pk; - return new CoreApi(DEFAULT_CONFIG).coreUsersRecoveryEmailRetrieve(data); + return new CoreApi(DEFAULT_CONFIG).coreUsersRecoveryEmailCreate(data); } renderForm(): TemplateResult {