From 3143f27b6c5e9e878121f4fb34c688d652530a65 Mon Sep 17 00:00:00 2001
From: Michael Hoennig <michael@hoennig.de>
Date: Fri, 10 May 2019 17:37:36 +0200
Subject: [PATCH] refactored ugly and risky code in role filter / removed
Role.isIndependent()
---
.../accessfilter/JSonAccessFilter.java | 6 ++-
.../hsadminng/service/accessfilter/Role.java | 23 ++++++-----
.../service/accessfilter/RoleUnitTest.java | 39 +++++--------------
3 files changed, 24 insertions(+), 44 deletions(-)
diff --git a/src/main/java/org/hostsharing/hsadminng/service/accessfilter/JSonAccessFilter.java b/src/main/java/org/hostsharing/hsadminng/service/accessfilter/JSonAccessFilter.java
index 8a29f645..7d76f6b3 100644
--- a/src/main/java/org/hostsharing/hsadminng/service/accessfilter/JSonAccessFilter.java
+++ b/src/main/java/org/hostsharing/hsadminng/service/accessfilter/JSonAccessFilter.java
@@ -59,8 +59,10 @@ abstract class JSonAccessFilter<T> {
*/
Set<Role> getLoginUserRoles() {
final Set<Role> independentRoles = Arrays.stream(Role.values())
- // TODO mhoennig: ugly and risky filter condition => refactor!
- .filter(role -> role.isIndependent() && SecurityUtils.isCurrentUserInRole(role.asAuthority()))
+ .filter(
+ role -> role.getAuthority()
+ .map(authority -> SecurityUtils.isCurrentUserInRole(authority))
+ .orElse(false))
.collect(Collectors.toSet());
final Set<Role> rolesOnThis = getId() != null ? getLoginUserDirectRolesFor(dto.getClass(), getId()) : EMPTY_SET;
diff --git a/src/main/java/org/hostsharing/hsadminng/service/accessfilter/Role.java b/src/main/java/org/hostsharing/hsadminng/service/accessfilter/Role.java
index 0c98c322..742d9ed9 100644
--- a/src/main/java/org/hostsharing/hsadminng/service/accessfilter/Role.java
+++ b/src/main/java/org/hostsharing/hsadminng/service/accessfilter/Role.java
@@ -6,6 +6,7 @@ import static com.google.common.base.Verify.verify;
import org.hostsharing.hsadminng.security.AuthoritiesConstants;
import java.lang.reflect.Field;
+import java.util.Optional;
/**
* These enum values are on the one hand used to define the minimum role required to grant access to resources,
@@ -94,21 +95,21 @@ public enum Role {
IGNORED;
private final Integer level;
- private final String authority;
+ private final Optional<String> authority;
Role(final int level, final String authority) {
this.level = level;
- this.authority = authority;
+ this.authority = Optional.of(authority);
}
Role(final int level) {
this.level = level;
- this.authority = AuthoritiesConstants.USER;
+ this.authority = Optional.empty();
}
Role() {
this.level = null;
- this.authority = null;
+ this.authority = Optional.empty();
}
/**
@@ -124,7 +125,12 @@ public enum Role {
return updateAccessFor.length == 1 && updateAccessFor[0].isIgnored();
}
- public String asAuthority() {
+ /**
+ * @return the independent authority related 1:1 to this Role or empty if no independent authority is related 1:1
+ *
+ * @see AuthoritiesConstants
+ */
+ public Optional<String> getAuthority() {
return authority;
}
@@ -135,13 +141,6 @@ public enum Role {
return this == Role.IGNORED;
}
- /**
- * @return true if this role is independent of a target object, false otherwise.
- */
- public boolean isIndependent() {
- return this != NOBODY && (this == ANYBODY || covers(Role.SUPPORTER));
- }
-
/**
* @return the role with the broadest access rights
*/
diff --git a/src/test/java/org/hostsharing/hsadminng/service/accessfilter/RoleUnitTest.java b/src/test/java/org/hostsharing/hsadminng/service/accessfilter/RoleUnitTest.java
index 9fe65dae..74d6f195 100644
--- a/src/test/java/org/hostsharing/hsadminng/service/accessfilter/RoleUnitTest.java
+++ b/src/test/java/org/hostsharing/hsadminng/service/accessfilter/RoleUnitTest.java
@@ -4,6 +4,8 @@ package org.hostsharing.hsadminng.service.accessfilter;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.ThrowableAssert.catchThrowable;
+import org.hostsharing.hsadminng.security.AuthoritiesConstants;
+
import com.google.common.base.VerifyException;
import org.junit.Test;
@@ -95,22 +97,6 @@ public class RoleUnitTest {
assertThat(catchThrowable(() -> Role.HOSTMASTER.coversAny((Role[]) null))).isInstanceOf(VerifyException.class);
}
- @Test
- public void isNdependend() {
- assertThat(Role.NOBODY.isIndependent()).isFalse();
-
- assertThat(Role.HOSTMASTER.isIndependent()).isTrue();
- assertThat(Role.ADMIN.isIndependent()).isTrue();
- assertThat(Role.SUPPORTER.isIndependent()).isTrue();
-
- assertThat(Role.CONTRACTUAL_CONTACT.isIndependent()).isFalse();
- assertThat(Role.FINANCIAL_CONTACT.isIndependent()).isFalse();
- assertThat(Role.ACTUAL_CUSTOMER_USER.isIndependent()).isFalse();
- assertThat(Role.ANY_CUSTOMER_USER.isIndependent()).isFalse();
-
- assertThat(Role.ANYBODY.isIndependent()).isTrue();
- }
-
@Test
public void isIgnored() {
for (Role role : Role.values()) {
@@ -131,20 +117,13 @@ public class RoleUnitTest {
}
@Test
- public void isIndependent() {
- assertThat(Role.HOSTMASTER.isIndependent()).isTrue();
- assertThat(Role.SUPPORTER.isIndependent()).isTrue();
-
- assertThat(Role.CONTRACTUAL_CONTACT.isIndependent()).isFalse();
- assertThat(Role.ANY_CUSTOMER_USER.isIndependent()).isFalse();
- }
-
- @Test
- public void asAuthority() {
- assertThat(Role.HOSTMASTER.asAuthority()).isEqualTo("ROLE_HOSTMASTER");
- assertThat(Role.ADMIN.asAuthority()).isEqualTo("ROLE_ADMIN");
- assertThat(Role.SUPPORTER.asAuthority()).isEqualTo("ROLE_SUPPORTER");
- assertThat(Role.CONTRACTUAL_CONTACT.asAuthority()).isEqualTo("ROLE_USER");
+ public void getAuthority() {
+ assertThat(Role.NOBODY.getAuthority()).isEmpty();
+ assertThat(Role.HOSTMASTER.getAuthority()).hasValue(AuthoritiesConstants.HOSTMASTER);
+ assertThat(Role.ADMIN.getAuthority()).hasValue(AuthoritiesConstants.ADMIN);
+ assertThat(Role.SUPPORTER.getAuthority()).hasValue(AuthoritiesConstants.SUPPORTER);
+ assertThat(Role.CONTRACTUAL_CONTACT.getAuthority()).isEmpty();
+ assertThat(Role.ANYBODY.getAuthority()).hasValue(AuthoritiesConstants.ANONYMOUS);
}
@Test