Skip to content

Commit

Permalink
Merge pull request #2516 from objectcomputing/bugfix-handle-null-request
Browse files Browse the repository at this point in the history 
fix: Request is marked nullable, so should be checked
  • Loading branch information
@mkimberlin
mkimberlin authored Jun 24, 2024
2 parents 4964a26 + 33cdf30 commit 471f110
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 59 deletions.
44 changes: 23 additions & 21 deletions ...c/main/java/com/objectcomputing/checkins/security/permissions/PermissionSecurityRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.objectcomputing.checkins.services.permissions.RequiredPermission;
import com.objectcomputing.checkins.services.role.role_permissions.RolePermission;
import com.objectcomputing.checkins.services.role.role_permissions.RolePermissionServices;
import io.micronaut.core.annotation.AnnotationValue;
import io.micronaut.core.annotation.Nullable;
import io.micronaut.http.HttpAttributes;
import io.micronaut.http.HttpRequest;
Expand All @@ -19,7 +20,6 @@
import reactor.core.publisher.Mono;

import java.util.Map;
import java.util.Optional;

@Singleton
public class PermissionSecurityRule implements SecurityRule<HttpRequest<?>> {
Expand All @@ -39,26 +39,28 @@ public int getOrder() {

@Override
public Publisher<SecurityRuleResult> check(@Nullable HttpRequest request, @Nullable Authentication authentication) {
RouteMatch routeMatch = request.getAttribute(HttpAttributes.ROUTE_MATCH, RouteMatch.class).orElse(null);
if (routeMatch instanceof MethodBasedRouteMatch methodBasedRouteMatch
&& methodBasedRouteMatch.hasAnnotation(RequiredPermission.class)) {
Optional<String> optionalPermission = methodBasedRouteMatch.findAnnotation(RequiredPermission.class).flatMap(r -> r.stringValue("value"));

Map<String, Object> claims = authentication != null ? authentication.getAttributes() : null;
if (optionalPermission.isPresent() && claims != null && claims.containsKey("roles")) {
final Permission requiredPermission = Permission.valueOf(optionalPermission.get());

JSONArray jsonArray = new JSONArray(claims.get("roles").toString());

boolean requiredPermissionFoundInRoles = jsonArray.toList().stream()
.map(Object::toString)
.flatMap(role -> rolePermissionServices.findByRole(role).stream())
.map(RolePermission::getPermission)
.anyMatch(p -> p == requiredPermission);

return Mono.just(requiredPermissionFoundInRoles ? SecurityRuleResult.ALLOWED : SecurityRuleResult.REJECTED);
}
if (request == null) {
return Mono.just(SecurityRuleResult.UNKNOWN);
}
return Mono.just(SecurityRuleResult.UNKNOWN);
return request.getAttribute(HttpAttributes.ROUTE_MATCH, RouteMatch.class)
.map(routeMatch -> routeMatch instanceof MethodBasedRouteMatch<?,?> ? (MethodBasedRouteMatch<?,?>) routeMatch : null)
.flatMap(routeMatch -> routeMatch.findAnnotation(RequiredPermission.class).flatMap(AnnotationValue::stringValue))
.map(Permission::valueOf)
.map(requiredPermission -> {
Map<String, Object> claims = authentication != null ? authentication.getAttributes() : null;
if (claims != null && claims.containsKey("roles")) {
JSONArray jsonArray = new JSONArray(claims.get("roles").toString());
boolean requiredPermissionFoundInRoles = jsonArray.toList().stream()
.map(Object::toString)
.flatMap(role -> rolePermissionServices.findByRole(role).stream())
.map(RolePermission::getPermission)
.anyMatch(p -> p == requiredPermission);
return requiredPermissionFoundInRoles ? SecurityRuleResult.ALLOWED : SecurityRuleResult.REJECTED;
} else {
return SecurityRuleResult.UNKNOWN;
}
})
.map(Mono::just)
.orElseGet(() -> Mono.just(SecurityRuleResult.UNKNOWN));
}
}
114 changes: 76 additions & 38 deletions server/src/test/java/com/objectcomputing/checkins/security/SecurityRuleResultTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.micronaut.security.authentication.Authentication;
import io.micronaut.security.rules.SecurityRuleResult;
import io.micronaut.web.router.MethodBasedRouteMatch;
import io.micronaut.web.router.RouteMatch;
import jakarta.inject.Inject;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
Expand All @@ -22,7 +23,6 @@
import org.reactivestreams.Publisher;
import reactor.test.StepVerifier;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand All @@ -34,13 +34,12 @@

class SecurityRuleResultTest extends TestContainersSuite {

List<String> userPermissions = List.of(
private static final List<String> USER_PERMISSIONS = List.of(
"CAN_VIEW_FEEDBACK_REQUEST",
"CAN_CREATE_FEEDBACK_REQUEST",
"CAN_DELETE_FEEDBACK_REQUEST"
);

List<String> userRoles = List.of("ADMIN");
private static final List<String> USER_ROLES = List.of("ADMIN");

@Inject
PermissionSecurityRule permissionSecurityRule;
Expand All @@ -52,7 +51,10 @@ class SecurityRuleResultTest extends TestContainersSuite {
RoleServices roleServices;

@Mock
private MethodBasedRouteMatch mockMethodBasedRouteMatch;
private MethodBasedRouteMatch<?, ?> mockMethodBasedRouteMatch;

@Mock
private RouteMatch<?> mockNonMethodBasedRouteMatch;

@Mock
private AnnotationValue<RequiredPermission> mockRequiredPermissionAnnotation;
Expand All @@ -68,54 +70,73 @@ void resetMocks() {

@AfterEach
void afterEach() throws Exception {
reset(mockMethodBasedRouteMatch);
reset(mockRequiredPermissionAnnotation);
reset(mockMethodBasedRouteMatch, mockRequiredPermissionAnnotation);
mockFinalizer.close();
}

@Test
void allowSecurityRuleResultTest() {

final HttpRequest<?> request = HttpRequest.POST("/", null)
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE)
.setAttribute(HttpAttributes.ROUTE_MATCH, mockMethodBasedRouteMatch);

Map<String, Object> attributes = new HashMap<>();
attributes.put("permissions", userPermissions);
attributes.put("roles", userRoles);
attributes.put("email", "test.email.address");
Map<String, Object> attributes = Map.of(
"permissions", USER_PERMISSIONS,
"roles", USER_ROLES,
"email", "test.email.address"
);

when(mockMethodBasedRouteMatch.hasAnnotation(RequiredPermission.class)).thenReturn(true);
when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.of(mockRequiredPermissionAnnotation));
when(mockRequiredPermissionAnnotation.stringValue("value")).thenReturn(Optional.of("CAN_VIEW_FEEDBACK_REQUEST"));
when(mockRequiredPermissionAnnotation.stringValue()).thenReturn(Optional.of("CAN_VIEW_FEEDBACK_REQUEST"));

Authentication auth = Authentication.build("test.email.address", attributes);

Publisher<SecurityRuleResult> result = permissionSecurityRule.check(request, auth);

assertNotNull(result);
StepVerifier.create(result)
.expectNext(SecurityRuleResult.ALLOWED)
.expectComplete()
.verify();
.expectNext(SecurityRuleResult.ALLOWED)
.expectComplete()
.verify();
}

@Test
void unknownResultForNonMethodBasedRouteMatch() {
final HttpRequest<?> request = HttpRequest.POST("/", null)
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE)
.setAttribute(HttpAttributes.ROUTE_MATCH, mockNonMethodBasedRouteMatch);

Map<String, Object> attributes = Map.of(
"permissions", USER_PERMISSIONS,
"roles", USER_ROLES,
"email", "test.email.address"
);

Authentication auth = Authentication.build("test.email.address", attributes);

Publisher<SecurityRuleResult> result = permissionSecurityRule.check(request, auth);

assertNotNull(result);
StepVerifier.create(result)
.expectNext(SecurityRuleResult.UNKNOWN)
.expectComplete()
.verify();
}

@Test
void rejectSecurityRuleResultTest() {

final HttpRequest<?> request = HttpRequest.POST("/", null)
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE)
.setAttribute(HttpAttributes.ROUTE_MATCH, mockMethodBasedRouteMatch);

Map<String, Object> attributes = new HashMap<>();
attributes.put("permissions", userPermissions);
attributes.put("roles", userRoles);
attributes.put("email", "test.email.address");
Map<String, Object> attributes = Map.of(
"permissions", USER_PERMISSIONS,
"roles", USER_ROLES,
"email", "test.email.address"
);

when(mockMethodBasedRouteMatch.hasAnnotation(RequiredPermission.class)).thenReturn(true);
when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.of(mockRequiredPermissionAnnotation));
when(mockRequiredPermissionAnnotation.stringValue("value")).thenReturn(Optional.of("CAN_CREATE_ORGANIZATION_MEMBERS"));
when(mockRequiredPermissionAnnotation.stringValue()).thenReturn(Optional.of("CAN_CREATE_ORGANIZATION_MEMBERS"));

Authentication auth = Authentication.build("test.email.address", attributes);
Publisher<SecurityRuleResult> result = permissionSecurityRule.check(request, auth);
Expand All @@ -129,27 +150,25 @@ void rejectSecurityRuleResultTest() {

@Test
void unknownSecurityRuleResultIfRouteMatchIsNotAnInstance() {

final HttpRequest<?> request = HttpRequest.POST("/", null)
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE);

Publisher<SecurityRuleResult> result = permissionSecurityRule.check(request, null);

assertNotNull(result);
StepVerifier.create(result)
.expectNext(SecurityRuleResult.UNKNOWN)
.expectComplete()
.verify();
.expectNext(SecurityRuleResult.UNKNOWN)
.expectComplete()
.verify();
}

@Test
void unknownSecurityRuleResultIfMethodBasedRouteMatchFailsToHaveAnnotation() {

final HttpRequest<?> request = HttpRequest.POST("/", null)
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE)
.setAttribute(HttpAttributes.ROUTE_MATCH, mockMethodBasedRouteMatch);

when(mockMethodBasedRouteMatch.hasAnnotation(RequiredPermission.class)).thenReturn(false);
when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.empty());

Publisher<SecurityRuleResult> result = permissionSecurityRule.check(request, null);

Expand All @@ -167,8 +186,8 @@ void unknownSecurityRuleResultIfRequiredPermissionIsNull() {
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE)
.setAttribute(HttpAttributes.ROUTE_MATCH, mockMethodBasedRouteMatch);

when(mockMethodBasedRouteMatch.hasAnnotation(RequiredPermission.class)).thenReturn(true);
when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.empty());
when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.of(mockRequiredPermissionAnnotation));
when(mockRequiredPermissionAnnotation.stringValue()).thenReturn(Optional.empty());

Publisher<SecurityRuleResult> result = permissionSecurityRule.check(request, null);

Expand All @@ -186,9 +205,8 @@ void unknownSecurityRuleResultIfClaimsIsNull() {
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE)
.setAttribute(HttpAttributes.ROUTE_MATCH, mockMethodBasedRouteMatch);

when(mockMethodBasedRouteMatch.hasAnnotation(RequiredPermission.class)).thenReturn(true);
when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.of(mockRequiredPermissionAnnotation));
when(mockRequiredPermissionAnnotation.stringValue("value")).thenReturn(Optional.of("CAN_CREATE_ORGANIZATION_MEMBERS"));
when(mockRequiredPermissionAnnotation.stringValue()).thenReturn(Optional.of("CAN_CREATE_ORGANIZATION_MEMBERS"));

Publisher<SecurityRuleResult> result = permissionSecurityRule.check(request, null);

Expand All @@ -206,12 +224,10 @@ void unknownSecurityRuleResultIfClaimsDoesNotContainPermissions() {
.basicAuth("test.email.address", RoleType.Constants.ADMIN_ROLE)
.setAttribute(HttpAttributes.ROUTE_MATCH, mockMethodBasedRouteMatch);

when(mockMethodBasedRouteMatch.hasAnnotation(RequiredPermission.class)).thenReturn(true);
when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.of(mockRequiredPermissionAnnotation));
when(mockRequiredPermissionAnnotation.stringValue("value")).thenReturn(Optional.of("CAN_CREATE_ORGANIZATION_MEMBERS"));
when(mockRequiredPermissionAnnotation.stringValue()).thenReturn(Optional.of("CAN_CREATE_ORGANIZATION_MEMBERS"));

Map<String, Object> attributes = new HashMap<>();
attributes.put("email", "test.email.address");
Map<String, Object> attributes = Map.of("email", "test.email.address");

Authentication auth = Authentication.build("test.email.address", attributes);

Expand All @@ -223,4 +239,26 @@ void unknownSecurityRuleResultIfClaimsDoesNotContainPermissions() {
.expectComplete()
.verify();
}

@Test
void unknownWhenRequestIsNull() {
Map<String, Object> attributes = Map.of(
"permissions", USER_PERMISSIONS,
"roles", USER_ROLES,
"email", "test.email.address"
);

when(mockMethodBasedRouteMatch.findAnnotation(RequiredPermission.class)).thenReturn(Optional.of(mockRequiredPermissionAnnotation));
when(mockRequiredPermissionAnnotation.stringValue()).thenReturn(Optional.of("CAN_VIEW_FEEDBACK_REQUEST"));

Authentication auth = Authentication.build("test.email.address", attributes);

Publisher<SecurityRuleResult> result = permissionSecurityRule.check(null, auth);

assertNotNull(result);
StepVerifier.create(result)
.expectNext(SecurityRuleResult.UNKNOWN)
.expectComplete()
.verify();
}
}

0 comments on commit 471f110

Please sign in to comment.