Skip to content

Fixing REST API response format for authorization error scenario

Aman Verma requested to merge users/amaverma/fixExceptionAndTests2 into master

Problem

Currently file service sends a response in following format in case mandatory headers are missing/ user is unauthorized:

{
    "code": 401,
    "reason": "Access denied",
    "message": "The user is not authorized to perform this action"
}

This format is not consistent with the API response format prescribed here https://cloud.google.com/storage/docs/json_api/v1/status-codes#401-unauthorized . Most of other services follow the prescribed format and hence there's a need to bring file service at parity.

Expected Behaviour

The response should look like this:

{
  "error": {
    "code": 401,
    "message": "User is unauthorized to perform this action",
    "errors": [
      {
        "domain": "global",
        "reason": "unauthorized",
        "message": "User is unauthorized to perform this action"
      }
    ]
  }
}

Are the error response in other scenarios correct?

Yes

Why there is a mismatch in format for this scenario

For authorization, file service uses AuthorizationService implemented in os-core-common library. Like other client wrappers, this guy also throws AppException if it encounters any error. In general, AppExceptions are not handled by APIs as it is an instance of RuntimeException. Thus the API is returning whatever response it receives from AuthorizationService as is, resulting in undesirable formatting.

Fix:

While invoking AuthorizationService from file service, I've introduced exception handling for AppException. In case the the Error code is 401 or 403, we throw OsduUnauthorizedException instead of AppException. OsduUnauthorizedException is already handled by *ExceptionHandler class which takes care of the formatting.

Changes in this MR

  1. Added try catch block in AuthorizationFiler to handle above mentioned scenarios
  2. Introduced a new error message in constants
  3. Updated the output payload for integration tests

What testing has been done:

  1. Tested the API response for concerned scenarios locally.
  2. Integration tests are GREEN

cc: @polavishnu @krveduru @pbehede @ethiraj

Merge request reports

Loading