New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lookup table/cache/adapter permissions #3914

Merged
merged 3 commits into from Jun 19, 2017

Conversation

Projects
2 participants
@bernd
Member

bernd commented Jun 14, 2017

This adds custom permissions to lookup tables, caches and adapters. We decided to only use lookuptables for now, instead of separate permissions for tables, caches and adapters.

@bernd bernd added this to the 2.3.0 milestone Jun 14, 2017

@bernd bernd requested a review from kroepke Jun 14, 2017

@bernd bernd added this to In Progress in Lookup Tables Jun 14, 2017

@@ -243,6 +247,7 @@ public LookupTablePage tables(@ApiParam(name = "page") @QueryParam("page") @Defa
public LookupTablePage get(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName,
@ApiParam(name = "resolve") @QueryParam("resolve") @DefaultValue("false") boolean resolveObjects) {

checkPermission(RestPermissions.LOOKUP_TABLES_READ, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

This (and the other references to idOrName) can become problematic, if we ever grant individual permissions, because then it is not clear which one to use when editing the permission: the id or the name.

I think it is better to load the DTO before and then check the permission on the id and never the names.

This comment has been minimized.

@bernd

bernd Jun 19, 2017

Member

Good point. I will change this to load the DTO and check against that. 👍

@@ -284,10 +290,12 @@ public LookupTableApi createTable(@ApiParam LookupTableApi lookupTable) {
}

@PUT
@Path("tables")
@Path("tables/{idOrName}")

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

Is this strictly required? I tried to avoid putting the ID in there twice, because then we should also check that the ID in the payload is the same as in the URL.

Also idOrName in the permission.

This comment has been minimized.

@bernd

bernd Jun 19, 2017

Member

@kroepke I added this for consistency with other resources in our system. We are using this everywhere, basically. When addressing a single resource, we put the ID in the URL. I think it's a good practice and common in many other HTTP APIs.

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

Ok fair enough!

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

Then I'd say we should at least check the ids for equality to prevent mistakes when passing different ids.

This comment has been minimized.

@bernd

bernd Jun 19, 2017

Member

Then I'd say we should at least check the ids for equality to prevent mistakes when passing different ids.

Yes! I will do that. 👍

public LookupTableApi updateTable(@Valid @ApiParam LookupTableApi toUpdate) {
public LookupTableApi updateTable(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName,
@Valid @ApiParam LookupTableApi toUpdate) {
checkPermission(RestPermissions.LOOKUP_TABLES_EDIT, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

idOrName permission

@@ -299,6 +307,8 @@ public LookupTableApi updateTable(@Valid @ApiParam LookupTableApi toUpdate) {
@AuditEvent(type = AuditEventTypes.LOOKUP_TABLE_DELETE)
@ApiOperation(value = "Delete the lookup table")
public LookupTableApi removeTable(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName) {
checkPermission(RestPermissions.LOOKUP_TABLES_DELETE, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

idOrName permission

@@ -423,6 +436,7 @@ public ErrorStates errorStates(@ApiParam(name = "request") @Valid ErrorStatesReq
@Path("adapters/{idOrName}")
@ApiOperation(value = "List the given data adapter")
public DataAdapterApi getAdapter(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName) {
checkPermission(RestPermissions.LOOKUP_TABLES_READ, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

idOrName permission

public DataAdapterApi updateAdapter(@Valid @ApiParam DataAdapterApi toUpdate) {
public DataAdapterApi updateAdapter(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName,
@Valid @ApiParam DataAdapterApi toUpdate) {
checkPermission(RestPermissions.LOOKUP_TABLES_EDIT, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

idOrName permission

@@ -588,6 +610,7 @@ public CachesPage caches(@ApiParam(name = "page") @QueryParam("page") @DefaultVa
@Path("caches/{idOrName}")
@ApiOperation(value = "List the given cache")
public CacheApi getCache(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName) {
checkPermission(RestPermissions.LOOKUP_TABLES_READ, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

idOrName permission

@@ -614,6 +638,7 @@ public CacheApi createCache(@ApiParam CacheApi newCache) {
@AuditEvent(type = AuditEventTypes.LOOKUP_CACHE_DELETE)
@ApiOperation(value = "Delete the given cache", notes = "The cache cannot be in use by any lookup table, otherwise the request will fail.")
public CacheApi deleteCache(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName) {
checkPermission(RestPermissions.LOOKUP_TABLES_DELETE, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

idOrName permission

@@ -630,10 +655,12 @@ public CacheApi deleteCache(@ApiParam(name = "idOrName") @PathParam("idOrName")
}

@PUT
@Path("caches")
@Path("caches/{idOrName}")

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

path vs body

public CacheApi updateCache(@ApiParam CacheApi toUpdate) {
public CacheApi updateCache(@ApiParam(name = "idOrName") @PathParam("idOrName") @NotEmpty String idOrName,
@ApiParam CacheApi toUpdate) {
checkPermission(RestPermissions.LOOKUP_TABLES_EDIT, idOrName);

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

idOrName permission

Make sure to always use the entity id when doing a permission check
Also check that the id in the url is the same as the id in the request
payload.
@kroepke

lgtm, caveat is listed, but for the sake of moving forward we can live with the edge case for now, I think

@@ -173,9 +176,29 @@ public LookupTableResource(DBLookupTableService dbTableService,
this.cacheSearchQueryParser = new SearchQueryParser(CacheDto.FIELD_TITLE, CACHE_SEARCH_FIELD_MAPPING);
}

private void checkLookupTableId(String idOrName, LookupTableApi toUpdate) {
requireNonNull(idOrName, "idOrName parameter cannot be null");
if (!idOrName.equals(toUpdate.id()) || !idOrName.equals(toUpdate.name())) {

This comment has been minimized.

@kroepke

kroepke Jun 19, 2017

Member

There's still the edge case that the name changed and is used as the identifier for updating.
Our code doesn't do that, but I think we can live with it for now.

If that's not the case, we can load the DTO first, and then check the old version of them.

This comment has been minimized.

@bernd

bernd Jun 19, 2017

Member

Agreed. Changing the name will cause havoc anyway because we do not check if it's used...

@kroepke kroepke merged commit 8372fa8 into master Jun 19, 2017

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1751 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 240 has succeeded
Details

@kroepke kroepke deleted the lut-permissions branch Jun 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment