Most efficient way to check if user is allowed to update an object

Question

So I have an user which can create his own Locations.

I implemented a webservice in order to add and update locations. If the user is not the owner of the location i want to throw an appropriate exception so that the user knows whats wrong.

The current method looks like this:

private void updateLocation(String loginToken, MapLocation ml) throws Exception{
    String username = this.getUsernameBySingInToken(loginToken); //throws exception is wrong token, otherwise returns username

    PreparedStatement stmt = conn.prepareStatement("SELECT username FROM Location WHERE idLoc=?");
    stmt.setInt(1, ml.getLocationId());
    ResultSet rs = stmt.executeQuery();
    String actuallUsername = null;
    while (rs.next()) {
       actuallUsername=rs.getString("username");
    }
    if(actuallUsername==null)
        throw new Exception("no location found!!"); //appropriate exception


    if(!username.equals(actuallUsername) || !username.equals(ml.getUsername()))
        throw new Exception("you are not the creator of this location!"); //appropriate exception 

    PreparedStatement stmtVerification = conn.prepareStatement("UPDATE Location SET positionX=?,positionY=?"
            + " WHERE idLoc=?");
    stmtVerification.setDouble(1, ml.getPosX());
    stmtVerification.setDouble(2, ml.getPosY());
    stmtVerification.setInt(3, ml.getLocationId());
    stmtVerification.executeUpdate();

    disconnectFromMySQL();
}

The second method I implemented is shorter but the errors are not clear:

private void updateLocationV2(String loginToken, MapLocation ml) throws Exception{
    String username = this.getUsernameBySingInToken(loginToken);
    if(!username.equals(ml.getUsername()))
        throw new Exception("username is not the same as username of maplocation object"); //not sure if neccesary 

    PreparedStatement stmtVerification = conn.prepareStatement("UPDATE Location SET positionX=?,positionY=?"
            + " WHERE idLoc=? and username=?");
    stmtVerification.setDouble(1, ml.getPosX());
    stmtVerification.setDouble(2, ml.getPosY());
    stmtVerification.setInt(3, ml.getLocationId());
    stmtVerification.setString(4, username);

    int executeUpdate = stmtVerification.executeUpdate();
    if(executeUpdate==0)
        throw new Exception("update failed. either wrong username or idLocation is wrong"); //not clear what is the error BUT shorter
    disconnectFromMySQL();
}    

So yeah, now I have two methods which update my location but I am not sure which of them is the best. The first method has the advantage that the errors are clear but its slower since I have two queries (select and update). the second method is faster but the errors are not clear. WHich one of them is better, or is there even a better solution?

EDIT: This is my webservice method which calls the update function:

@PUT
@Path("/update")
@Consumes({MediaType.APPLICATION_JSON})
public Response update(String token, MapLocation ml) {
    Response r = Response.status(Response.Status.OK).entity("location updated").build();
    try {
        isUpdated = db.updateLocation(token,ml);
    } catch (Exception ex) {
        r = Response.status(Response.Status.BAD_REQUEST).entity(ex.getMessage()).build();
    }
    return r;
}

Answer 1

Soo ... I'm not quite sure how to formulate this without it sounding brash, but... Both of these methods are less than ideal...

Let's follow the behavioural flow to see what issues this code has:

• The webservice portion you provide only understands PUT requests.
  Conventionally, PUT requests are used to create objects, not to update them. Instead one would expect a POST endpoint for that.

• You first create an OK response and then overwrite it in case something fails. That's somewhat... annoying in comparison to the simpler direct return.

  try {
      db.updateLocation(token, ml);
      return Response.status(Status.OK).entity("location updated").build();
  } catch (Exception e) {
      return Response.status(Status.BAD_REQUEST).entity(ex.getMessage()).build();
  }
  

• All this code assumes that the user gets to see the Exception message.
  This is a bad assumption. Your webservice method should be responsible for creating useful messages, not your business logic.

• Considering that the webservice code does not compile (because the declaration for isUpdated is missing) I hope that you don't actually pretend that every Exception ever is the user's fault by throwing a HTTP 400 to them...

The root cause of your conundrum is that you're not using the power of widely used java sql abstractions. Instead of manually writing SQL statements and performing updates, you should really make use of JPA and Entities.

If you had these, all of this would boil down to the following simplified code:

private void updateLocation(String username, MapLocation update) {
    MapLocation stored = em.find(MapLocation.class, update.getLocationId());
    if (stored == null) {
        throw new LocationDoesNotExistException();
    }
    if (!stored.getUsername().equals(username)) {
        throw new IllegalLocationUpdateException();
    }
    em.persist(update);
}

Note that the codesample above makes some simplifying assumptions about transaction management and the class hierarchy of the Exceptions it throws.

Note that I'm suggesting to rewrite your webservice method to something like:

public Response update(String loginToken, MapLocation request) {
    final String username = getUsernameFromToken(loginToken);
    try {
        db.updateLocation(username, request);
        return Response.ok().build();
    } catch (LocationDoesNotExistException e) {
        return Response.status(Response.Status.NOT_FOUND).build();
    } catch (IllegalLocationUpdateException e) {
        return Response.status(Response.Status.FORBIDDEN).build();
    } catch (Exception e) {
        LOGGER.warn("Unexpected exception in updateLocation", e);
        return Response.serverError().build();
    }
}