Skip to content

Commit

Permalink
fix permission check with multiple permissions
Browse files Browse the repository at this point in the history
    don't try to be too clever and actually add tests

    fixes graylog-labs/graylog2-web-interface#1456
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 64d90a4 commit 2b6cd45
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 54 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,33 @@
*/
package org.graylog2.rest.resources;

import com.google.inject.Module;
import org.apache.shiro.subject.Subject;
import org.assertj.core.util.Lists;
import org.graylog2.shared.bindings.GuiceInjectorHolder;
import org.graylog2.shared.rest.resources.RestResource;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentMatcher;
import org.mockito.internal.matchers.VarargMatcher;

import java.util.Collections;
import java.util.List;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Matchers.argThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class RestResourceBaseTest {
@Before
public void setUpInjector() throws Exception {
// The list of modules is empty for now so only JIT injection will be used.
GuiceInjectorHolder.createInjector(Collections.emptyList());
final List<Module> modules = Lists.emptyList();
GuiceInjectorHolder.createInjector(modules);
}


@Test
public void testisAnyPermitted() {
final PermissionDeniedResource failingResource = new PermissionDeniedResource();
Expand All @@ -52,7 +58,7 @@ private static class PermissionDeniedResource extends RestResource {
@Override
protected Subject getSubject() {
final Subject mock = mock(Subject.class);
when(mock.isPermitted((String[]) any())).thenReturn(new boolean[]{false, false});
when(mock.isPermitted(argThat(new MyVarargMatcher()))).thenReturn(new boolean[]{false, false});
return mock;
}

Expand All @@ -64,7 +70,7 @@ private static class AllPermissionsGrantedResource extends RestResource {
@Override
protected Subject getSubject() {
final Subject mock = mock(Subject.class);
when(mock.isPermitted((String[]) any())).thenReturn(new boolean[]{true, true});
when(mock.isPermitted(argThat(new MyVarargMatcher()))).thenReturn(new boolean[]{true, true});
return mock;
}

Expand All @@ -76,12 +82,18 @@ private static class SomePermissionsGrantedResource extends RestResource {
@Override
protected Subject getSubject() {
final Subject mock = mock(Subject.class);
when(mock.isPermitted((String[]) any())).thenReturn(new boolean[]{false, true});
when(mock.isPermitted(argThat(new MyVarargMatcher()))).thenReturn(new boolean[]{false, true});
return mock;
}

public boolean runCheck() {
return isAnyPermitted(new String[]{"a:b", "a:c"}, "instance");
}
}

static class MyVarargMatcher extends ArgumentMatcher<String[]> implements VarargMatcher {
@Override public boolean matches(Object varargArgument) {
return /* does it match? */ true;
}
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
/**
* This file is part of Graylog2.
* This file is part of Graylog.
*
* Graylog2 is free software: you can redistribute it and/or modify
* Graylog is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* Graylog2 is distributed in the hope that it will be useful,
* Graylog is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with Graylog2. If not, see <http://www.gnu.org/licenses/>.
* along with Graylog. If not, see <http://www.gnu.org/licenses/>.
*/
package org.graylog2.shared.rest.resources;

Expand All @@ -22,25 +22,28 @@
import com.fasterxml.jackson.jaxrs.cfg.EndpointConfigBase;
import com.fasterxml.jackson.jaxrs.cfg.ObjectWriterInjector;
import com.fasterxml.jackson.jaxrs.cfg.ObjectWriterModifier;
import com.github.joschi.jadconfig.util.Size;
import com.google.common.collect.ImmutableMap;
import com.google.common.base.Function;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.Iterables;
import org.apache.shiro.subject.Subject;
import org.graylog2.plugin.ServerStatus;
import org.graylog2.shared.security.ShiroSecurityContext;
import org.graylog2.plugin.BaseConfiguration;
import org.graylog2.plugin.database.users.User;
import org.graylog2.shared.security.ShiroSecurityContext;
import org.graylog2.shared.users.UserService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.Nullable;
import javax.inject.Inject;
import javax.ws.rs.ForbiddenException;
import javax.ws.rs.QueryParam;
import javax.ws.rs.core.Context;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.MultivaluedMap;
import javax.ws.rs.core.SecurityContext;
import javax.ws.rs.core.UriBuilder;
import javax.ws.rs.core.UriInfo;
import java.security.Principal;
import java.util.Map;
import java.util.Arrays;

public abstract class RestResource {
private static final Logger LOG = LoggerFactory.getLogger(RestResource.class);
Expand All @@ -52,11 +55,14 @@ public abstract class RestResource {
protected UserService userService;

@Inject
protected ServerStatus serverStatus;
private BaseConfiguration configuration;

@Context
SecurityContext securityContext;

@Context
UriInfo uriInfo;

@QueryParam("pretty")
public void setPrettyPrint(boolean prettyPrint) {
if (prettyPrint) {
Expand All @@ -70,10 +76,6 @@ public ObjectWriter modify(EndpointConfigBase<?> endpoint, MultivaluedMap<String
}
}

protected int page(int page) {
return Math.max(0, page - 1);
}

protected Subject getSubject() {
if (securityContext == null) {
LOG.error("Cannot retrieve current subject, SecurityContext isn't set.");
Expand Down Expand Up @@ -110,44 +112,31 @@ protected void checkPermission(String permission, String instanceId) {
}
}

protected Map<String, Long> bytesToValueMap(long bytes) {
final Size size = Size.bytes(bytes);
return ImmutableMap.of(
"bytes", size.toBytes(),
"kilobytes", size.toKilobytes(),
"megabytes", size.toMegabytes());
protected boolean isAnyPermitted(String[] permissions, final String instanceId) {
final Iterable<String> instancePermissions = Iterables.transform(Arrays.asList(permissions),
new Function<String, String>() {
@Nullable
@Override
public String apply(String permission) {
return permission + ":" + instanceId;
}
});
return isAnyPermitted(FluentIterable.from(instancePermissions).toArray(String.class));
}

protected String guessContentType(final String filename) {
// A really dumb but for us good enough approach. We only need this for a very few static files we control.

if (filename.endsWith(".png")) {
return "image/png";
protected boolean isAnyPermitted(String... permissions) {
final boolean[] permitted = getSubject().isPermitted(permissions);
for (boolean p : permitted) {
if (p) {
return true;
}
}

if (filename.endsWith(".gif")) {
return "image/gif";
}

if (filename.endsWith(".css")) {
return "text/css";
}

if (filename.endsWith(".js")) {
return "application/javascript";
}

if (filename.endsWith(".html")) {
return MediaType.TEXT_HTML;
}

return MediaType.TEXT_PLAIN;
return false;
}

protected void restrictToMaster() {
if (!serverStatus.hasCapability(ServerStatus.Capability.MASTER)) {
LOG.warn("Rejected request that is only allowed against master nodes. Returning HTTP 403.");
throw new ForbiddenException("Request is only allowed against master nodes.");
protected void checkAnyPermission(String permissions[], String instanceId) {
if (!isAnyPermitted(permissions, instanceId)) {
throw new ForbiddenException("Not authorized to access resource id " + instanceId);
}
}

Expand All @@ -161,4 +150,11 @@ protected User getCurrentUser() {

return user;
}

protected UriBuilder getUriBuilderToSelf() {
if (configuration.getRestTransportUri() != null) {
return UriBuilder.fromUri(configuration.getRestTransportUri());
} else
return uriInfo.getBaseUriBuilder();
}
}

0 comments on commit 2b6cd45

Please sign in to comment.