[SENTRY-1546] Generic Policy provides bad error messages for Sentry exceptions

• Type: Bug
• Status: Resolved
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: 1.8.0, 2.0.0
• Fix Version/s: 1.8.0, 2.0.0
• Labels: bite-sized
• Assignee: Krishna Kalyan
• Reporter: Alex Kolbasov
• Created: 28/Nov/16 17:35
• Updated: 24/Jul/17 15:37
• Resolved: 06/Mar/17 21:39

Description

I discovered that when you attempt to create a role that already exists the error message you get back from Thrift i just 'Role: foo' which is very confusing.

The reason is that the SentryStore throws

SentryAlreadyExistsException("Role: " + trimmedRoleName);

and the generic policy processor passes the message as is:

  public TCreateSentryRoleResponse create_sentry_role(
      final TCreateSentryRoleRequest request) throws TException {
    Response<Void> respose = requestHandle(new RequestHandler<Void>() {
      @Override
      public Response<Void> handle() throws Exception {
        validateClientVersion(request.getProtocol_version());
        authorize(request.getRequestorUserName(),
            getRequestorGroups(conf, request.getRequestorUserName()));
        store.createRole(request.getComponent(), request.getRoleName(),
                request.getRequestorUserName());
        return new Response<Void>(Status.OK());
      }
    });
...

The similar thing is happening for other requests and other Sentry-specific exceptions.

The legacy policy processor does decorate the error a bit:

  public TCreateSentryRoleResponse create_sentry_role(
    TCreateSentryRoleRequest request) throws TException {
    final Timer.Context timerContext = sentryMetrics.createRoleTimer.time();
    TCreateSentryRoleResponse response = new TCreateSentryRoleResponse();
    try {
      validateClientVersion(request.getProtocol_version());
      authorize(request.getRequestorUserName(),
          getRequestorGroups(request.getRequestorUserName()));
      sentryStore.createSentryRole(request.getRoleName());
      response.setStatus(Status.OK());
      notificationHandlerInvoker.create_sentry_role(request, response);
    } catch (SentryAlreadyExistsException e) {
      String msg = "Role: " + request + " already exists.";
      LOGGER.error(msg, e);
      response.setStatus(Status.AlreadyExists(msg, e));
    } catch (SentryAccessDeniedException e) {
      LOGGER.error(e.getMessage(), e);
      response.setStatus(Status.AccessDenied(e.getMessage(), e));
    } catch (SentryThriftAPIMismatchException e) {
      LOGGER.error(e.getMessage(), e);
      response.setStatus(Status.THRIFT_VERSION_MISMATCH(e.getMessage(), e));
    } catch (Exception e) {
      String msg = "Unknown error for request: " + request + ", message: " + e.getMessage();
      LOGGER.error(msg, e);
      response.setStatus(Status.RuntimeError(msg, e));
    } finally {
...

I think that it is better to just put the right message in the exception itself and do not decorate it later.

Issue Links

• links to: Review board link