Skip to content

Graceful handling of unauthorized requests to the Management API#16836

Merged
Zeegaan merged 1 commit intov14/devfrom
v14/fix/unauthorized-request-yields-exception
Jul 31, 2024
Merged

Graceful handling of unauthorized requests to the Management API#16836
Zeegaan merged 1 commit intov14/devfrom
v14/fix/unauthorized-request-yields-exception

Conversation

@kjac
Copy link
Copy Markdown
Contributor

@kjac kjac commented Jul 29, 2024

Prerequisites

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

Description

#16552 has had the unfortunate side-effect that exceptions are being thrown by the AllowedApplicationHandler, whenever a request is made to an endpoint that is guarded by this handler ("section" based access), resulting in a 500 error instead of the expected 401:

image

This PR changes AllowedApplicationHandler so it doesn't assume a logged-in user by default.

Testing this PR

  1. Make an unauthorized request to any endpoint that requires "section" access (e.g. to /umbraco/management/api/v1/tree/data-type/root?skip=0&take=100) and verify that the response is a 401.
  2. Log into the backoffice and make sure things still work 😄

@Zeegaan
Copy link
Copy Markdown
Member

Zeegaan commented Jul 31, 2024

LGTM 🚀

@Zeegaan Zeegaan merged commit 5d866d7 into v14/dev Jul 31, 2024
@Zeegaan Zeegaan deleted the v14/fix/unauthorized-request-yields-exception branch July 31, 2024 07:45
/// <param name="currentUser">The current user's principal.</param>
/// <param name="user">The resulting <see cref="IUser" />, if the conversion is successful.</param>
/// <returns>True if the conversion is successful, false otherwise</returns>
bool TryGetUmbracoUser(IPrincipal currentUser, [NotNullWhen(true)] out IUser? user);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is this not breaking. ?

Do we mis something to check for breaking changes?

I think we should make an interface implementation just catching the exception from GetUmbracoUser

@kjac @Zeegaan

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are right, seems some package validation is missing if it's not catching these things automatically 😢

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fix here: #16899

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants