You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by bbende <gi...@git.apache.org> on 2018/04/05 20:52:11 UTC
[GitHub] nifi-registry pull request #108: NIFIREG-158 Added ability to retrieve flow ...
GitHub user bbende opened a pull request:
https://github.com/apache/nifi-registry/pull/108
NIFIREG-158 Added ability to retrieve flow directly by id without kno…
…wing the bucket
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bbende/nifi-registry NIFIREG-158
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-registry/pull/108.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #108
----
commit 216f325dc6d15134207e32db3a3c5af6c17d504e
Author: Bryan Bende <bb...@...>
Date: 2018-04-05T20:22:08Z
NIFIREG-158 Added ability to retrieve flow directly by id without knowing the bucket
----
---
[GitHub] nifi-registry pull request #108: NIFIREG-158 Added ability to retrieve flow ...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:
https://github.com/apache/nifi-registry/pull/108#discussion_r180112539
--- Diff: nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/FlowResource.java ---
@@ -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 = "read"),
+ @ExtensionProperty(name = "resource", value = "/buckets/{bucketId}") })
+ }
+ )
+ @ApiResponses({
+ @ApiResponse(code = 400, message = HttpStatusMessages.MESSAGE_400),
+ @ApiResponse(code = 401, message = HttpStatusMessages.MESSAGE_401),
+ @ApiResponse(code = 403, message = HttpStatusMessages.MESSAGE_403),
+ @ApiResponse(code = 404, message = HttpStatusMessages.MESSAGE_404),
+ @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, flow.getBucketIdentifier());
--- 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 troubleshooting.
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.
---
[GitHub] nifi-registry issue #108: NIFIREG-158 Added ability to retrieve flow directl...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi-registry/pull/108
@kevdoran good idea, I pushed up another commit that wraps the exception
---
[GitHub] nifi-registry issue #108: NIFIREG-158 Added ability to retrieve flow directl...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi-registry/pull/108
@kevdoran ok good to go now, ended up adding all of the GET operations from BucketFlowResource that could be done without a bucket id...
```
/flows/{flowId}
/flows/{flowId}/versions/{version}
/flows/{flowId}/versions/latest
/flows/{flowId}/versions/latest/metadata
/flows/{flowId}/versions
```
---
[GitHub] nifi-registry pull request #108: NIFIREG-158 Added ability to retrieve flow ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-registry/pull/108
---
[GitHub] nifi-registry issue #108: NIFIREG-158 Added ability to retrieve flow directl...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/108
@bbende alright will wait until it's ready. thanks for letting me know!
---
[GitHub] nifi-registry issue #108: NIFIREG-158 Added ability to retrieve flow directl...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi-registry/pull/108
@kevdoran thanks, I have some more stuff to push up that I kept working on after submitting, incoming shortly...
---
[GitHub] nifi-registry issue #108: NIFIREG-158 Added ability to retrieve flow directl...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/108
Will review...
---