Skip to content

Make public access endpoints work correctly#16136

Merged
Migaroez merged 1 commit intov14/devfrom
v14/fix/public-access-endpoints
Apr 25, 2024
Merged

Make public access endpoints work correctly#16136
Migaroez merged 1 commit intov14/devfrom
v14/fix/public-access-endpoints

Conversation

@kjac
Copy link
Contributor

@kjac kjac commented Apr 24, 2024

Prerequisites

  • I have added steps to test this contribution in the description below

Description

There are a few issues with the "get" and "create" endpoints for public access.

Create

  1. Upon successful creation of a public access rule, the endpoint yields the wrong "created at" location. The correct location to retrieve the created rule is /document/[document key]/public-access.

GET

  1. The HTTP 200 OK response is not documented towards OpenAPI.
  2. When queried for a non-existing public access rule, the endpoint should return HTTP 404 NotFound.
  3. When queried for a public access rule with one or more explicit members, the operation fails because the umbraco mapper does not know how to map IMember to MemberItemResponseModel.
  4. Fixing the previous problem uncovers that public access rules with explicit members yields a MemberItemResponseModel that contains all existing member groups instead of none.

Testing this PR

Use Swagger to ensure that the public access endpoints work as expected.

Copy link
Contributor

@Migaroez Migaroez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, works as described

await _publicAccessService.GetEntryByContentKeyAsync(id);

if (accessAttempt.Success is false)
if (accessAttempt.Success is false || accessAttempt.Result is null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I read this wrong this means that

  • it is possible for the attempt to succeed without a result
  • if it does, we return a 500

That's the goal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter ShouldNotHappen™ but the Result is a nullable value, so -- need a null check for the compiler to stop complaining.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShouldNotHappen™ => 500 sounds good to me. Merging!

@Migaroez Migaroez merged commit c85b1d4 into v14/dev Apr 25, 2024
@Migaroez Migaroez deleted the v14/fix/public-access-endpoints branch April 25, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants