Github user kevdoran commented on a diff in the pull request:
    --- Diff: 
    @@ -62,4 +85,214 @@ public Response getAvailableFlowFields() {
             return Response.status(Response.Status.OK).entity(fields).build();
    +    @GET
    +    @Path("{flowId}")
    +    @Consumes(MediaType.WILDCARD)
    +    @Produces(MediaType.APPLICATION_JSON)
    +    @ApiOperation(
    +            value = "Gets a flow",
    +            response = VersionedFlow.class,
    +            extensions = {
    +                    @Extension(name = "access-policy", properties = {
    +                            @ExtensionProperty(name = "action", value = 
    +                            @ExtensionProperty(name = "resource", value = 
"/buckets/{bucketId}") })
    +            }
    +    )
    +    @ApiResponses({
    +            @ApiResponse(code = 400, message = 
    +            @ApiResponse(code = 401, message = 
    +            @ApiResponse(code = 403, message = 
    +            @ApiResponse(code = 404, message = 
    +            @ApiResponse(code = 409, message = 
HttpStatusMessages.MESSAGE_409) })
    +    public Response getFlow(
    +            @PathParam("flowId")
    +            @ApiParam("The flow identifier")
    +            final String flowId) {
    +        final VersionedFlow flow = registryService.getFlow(flowId);
    +        // this should never happen, but if somehow the back-end didn't 
populate the bucket id let's make sure the flow isn't returned
    +        if (StringUtils.isBlank(flow.getBucketIdentifier())) {
    +            throw new IllegalStateException("Unable to authorize access 
because bucket identifier is null or blank");
    +        }
    +        authorizeBucketAccess(RequestAction.READ, 
    --- End diff --
    When I try this endpoint with an unauthorized user, I get the following 
response back:
    HTTP/1.1 403 Forbidden
    Connection: close
    Date: Mon, 09 Apr 2018 14:08:27 GMT
    Content-Type: text/plain
    Content-Length: 101
    Server: Jetty(9.4.3.v20170317)
    Unable to view Bucket with ID 6eaeae9c-dbdb-4af3-a98e-4f3b880a0fb2. Contact 
the system administrator.
    The 403 status code is good, but I'm not sure about the error message in 
the response. If someone is attempting to access the flow through /flows/{id}, 
I don't think the server should return the bucket id containing the flow, as 
that's leaking information the user would not otherwise have access to. It's a 
fairly harmless piece of information on it's own, but in a multi-tenant 
scenario it could reveal more than the owner of the bucket would like, 
especially if correlated with other information an attacker is able to obtain.
    It's probably not a huge issue, but if you agree, we could strip this by 
wrapping the call to authorizeBucketAccess() in a try catch that obscures the 
error message returned in the response body. We would have to do this for all 
the GET methods in the FlowResource. Something like:
    try {
        authorizeBucketAccess(RequestAction.READ, flow.getBucketIdentifier());
    } catch (AccessDeniedException e) {
        throw new AccessDeniedException("User not authorized to view the 
specified flow");
    By adding the root throwable to the cause of a custom exception, we could 
even keep the root cause with the bucket id in the logs for easier admin 
    Now that I think about it, that approach for customizing HTTP response 
error messages to differ from internal/logged error message probably be a 
better approach than the solution that we came up with in PR #99, which 
inadvertently introduced a "log & throw" pattern in order to maintain resource 
ids in the logs.


Reply via email to