Skip to content

TUL/Authorization redirect fix#1137

Merged
milanmajchrak merged 1 commit intocustomer/TULfrom
tul/authorization-redirect-fix
Nov 3, 2025
Merged

TUL/Authorization redirect fix#1137
milanmajchrak merged 1 commit intocustomer/TULfrom
tul/authorization-redirect-fix

Conversation

@milanmajchrak
Copy link
Copy Markdown
Collaborator

Phases MP MM MB MR JM Total
ETA 0 0 0 0 0 0
Developing 0 0 0 0 0 0
Review 0 0 0 0 0 0
Total - - - - - 0
ETA est. 0
ETA cust. - - - - - 0

Problem description

@milanmajchrak milanmajchrak requested a review from Copilot November 3, 2025 13:25
@milanmajchrak milanmajchrak merged commit 31a0ccc into customer/TUL Nov 3, 2025
5 checks passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a null check for the result of converter.toRest() in the identifier lookup endpoint to handle cases where users lack permissions to access the resolved DSpace object. When the converter returns null due to authorization failures, the endpoint now returns a 401 Unauthorized error instead of attempting to use the null object.

Key Changes

  • Added null check after calling converter.toRest() to prevent NullPointerException when authorization fails
  • Returns HTTP 401 Unauthorized with "Authentication is required" message when conversion returns null

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// Convert and respond with a redirect to the object itself
DSpaceObjectRest dsor = converter.toRest(dso, utils.obtainProjection());
if (dsor == null) {
response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authentication is required");
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The error response incorrectly assumes the user is unauthenticated when toRest() returns null. According to ConverterService.toRest() (lines 115-126), null is returned when authorization fails, which can occur for both authenticated and unauthenticated users. The standard DSpace pattern (see DSpaceApiExceptionControllerAdvice lines 75-84) is to check context.getCurrentUser() and return SC_FORBIDDEN (403) with 'Access is denied' if the user is authenticated, or SC_UNAUTHORIZED (401) with 'Authentication is required' if the user is not authenticated. This should be: if (context.getCurrentUser() != null) { response.sendError(HttpServletResponse.SC_FORBIDDEN, \"Access is denied\"); } else { response.sendError(HttpServletResponse.SC_UNAUTHORIZED, \"Authentication is required\"); }

Suggested change
response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authentication is required");
if (context.getCurrentUser() != null) {
response.sendError(HttpServletResponse.SC_FORBIDDEN, "Access is denied");
} else {
response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authentication is required");
}

Copilot uses AI. Check for mistakes.
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