3
\$\begingroup\$

I need to check that a user is allowed to save/retrieve contacts to/from the database by calling a web service, and return an HTTP403 with an explanation if it is not the case.

So for the sake of factorizing the code, I decided to use a boolean method that "never" returns false and is there only to check this, and throws a corresponding exception.

public boolean canPerformAction(User authenticatedUser) throws ForbiddenActionException{
  if(authenticatedUser == null){
    throw new ForbiddenActionException("There must be a user to perform an action !");
  }

  if(!authenticatedUser.BelongToCompany(company)){
    throw new ForbiddenActionException("The user doesn't belong to the company, therefore he can't perform the action");
  }

  return true;
}

Then in saveContact, I call it as follows:

public Person saveContact(User user, Person contact) throws ForbiddenActionException{

  canPerformAction(user);

  contact= persistContact(contact);

  return contact;
}

I do the same thing in retrieveContact:

public Person retrieveContact(User user, String contactId) throws ForbiddenActionException{

    canPerformAction(user);

    return fetchContact(contactId);
 }

And finally these methods are called in the web service:

@GET
@Path("{id}")
@Produces(MediaType.APPLICATION_JSON)
public Response retrieveContact(@PathParam("contactId") String contactId) {
  try{
    Person contact= retrieveContact(getUser(), contactId);
    return Response.status(Response.Status.OK).entity(contact).build();
  }catch(ForbiddenActionException e){
    return Response.status(Response.Status.FORBIDDEN).entity(e.getMessage()).build();
  }
}

@POST
@Path("{id}")
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
public Response saveContact(@PathParam("id") String id, Person contact) {
  try{
    Person contact= saveContact(getUser(), contact);
    return Response.status(Response.Status.OK).entity(contact).build();
  }catch(ForbiddenActionException e){
    return Response.status(Response.Status.FORBIDDEN).entity(e.getMessage()).build();
  }
}

Do you think that this is an acceptable approach of dealing with the problem ?

\$\endgroup\$
  • 2
    \$\begingroup\$ Why not just make it void since you're not doing anything with the true value anyway? \$\endgroup\$ – SuperBiasedMan Mar 23 '16 at 11:02
  • \$\begingroup\$ @SuperBiasedMan Absolutely, void is more suitable in this case. But is it a good practice / common to put checks in void methods ? Won't it be consided as bad code ? \$\endgroup\$ – Anas Mar 23 '16 at 11:16
  • 1
    \$\begingroup\$ I'll defer to someone who's more familiar with Java to answer properly. I personally don't like it because it implies that the return value matters or should be used, which can confuse users. To me void signals that the function is run to execute commands. I'm specifically thinking of Command Query Separation when I say that. \$\endgroup\$ – SuperBiasedMan Mar 23 '16 at 11:30
  • \$\begingroup\$ @SuperBiasedMan We'll wait then for the other users' opinions. Thanks for your comments :) \$\endgroup\$ – Anas Mar 23 '16 at 11:43
  • \$\begingroup\$ Your braces don't match. Please ensure that you are posting real code, not code that has been sanitized for this question. \$\endgroup\$ – 200_success Mar 23 '16 at 16:00
3
\$\begingroup\$

The Question

The approach is acceptable and it's perfectly valid to use a method like canPerformAction. But it shouldn't return an unused boolean and can be static:

private static void ensureUserAuthorized(User user) throws ForbiddenActionException {
  if (user == null) {
    throw new ForbiddenActionException("message");
  }
  if (!user.belongsToCompany(company)) {
    throw new ForbiddenActionException("message");
  }
}

Other Stuff

The boilerplate response building instructions return Response.status... in service methods can be avoided in two ways:

1) Create an ad-hoc shortcut method like

private static Response buildResponse(Status status, Object entity) {
  return Response.status(status).entity(entity).build();
}

Or, better

2) If you are using Jersey behind your JAX-RS annotations, add an exception mapper class:

@Provider
public class ForbiddenActionMapper implements ExceptionMapper<ForbiddenActionException> {

  @Override
  public Response toResponse(ForbiddenActionException ex) {
    return Response.status(Response.Status.FORBIDDEN)
                   .entity(ex.getMessage())
                   .type(MediaType.APPLICATION_JSON)
                   .build();
  }
}

In this second case the service method can be shortened as follows:

@POST
@annotations...
public Contact saveContact(@PathParam("id") String id, Person contact) throws ForbiddenActionException {
  return saveContact(getUser(), contact);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed answer !! But I'd like to confirm something; I assume you meant that the service method should return buildResponse(Response.Status.OK, saveContact(getUser(), contact) instead of saveContact(getUser(), contact)once the response builder method buildResponse has been defined ? \$\endgroup\$ – Anas Mar 24 '16 at 8:09
  • 1
    \$\begingroup\$ If you keep the Response return type of the service method, the try-catch blocks should be kept and return buildResponse(args); can be used. Otherwise you can change the return type to Contact and use directly return saveContact(args); + add throws clause. In case of exception, it will be intercepted by the framework and ForbiddenActionMapper will be used to build the 403 response. \$\endgroup\$ – Antot Mar 24 '16 at 9:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.