WithUser refactoring (#2944)

Signed-off-by: Avgustin Marinov <Avgustin.Marinov@bosch.com>

hawkbit-dmf/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java

@@ -169,11 +169,11 @@ public class AmqpMessageHandlerService extends BaseAmqpService {
         SecurityContextHolder.setContext(securityContextImpl);
     }
 
-    private static void setTenantSecurityContext(final String tenantId) {
+    private static void setTenantSecurityContext(final String tenant) {
         final AnonymousAuthenticationToken authenticationToken = new AnonymousAuthenticationToken(
                 UUID.randomUUID().toString(), "AMQP-Controller",
                 List.of(new SimpleGrantedAuthority(SpRole.CONTROLLER_ROLE_ANONYMOUS)));
-        authenticationToken.setDetails(new TenantAwareAuthenticationDetails(tenantId, true));
+        authenticationToken.setDetails(new TenantAwareAuthenticationDetails(tenant, true));
         setSecurityContext(authenticationToken);
     }
 

hawkbit-mgmt/hawkbit-mgmt-resource/src/test/java/org/eclipse/hawkbit/mgmt/rest/resource/MgmtRolloutResourceTest.java

@@ -41,7 +41,6 @@ import org.eclipse.hawkbit.mgmt.json.model.rollout.MgmtRolloutResponseBody;
 import org.eclipse.hawkbit.mgmt.rest.api.MgmtRestConstants;
 import org.eclipse.hawkbit.mgmt.rest.api.MgmtRolloutRestApi;
 import org.eclipse.hawkbit.mgmt.rest.resource.mapper.MgmtRestModelMapper;
-import org.eclipse.hawkbit.repository.DistributionSetManagement;
 import org.eclipse.hawkbit.repository.RolloutGroupManagement;
 import org.eclipse.hawkbit.repository.RolloutManagement;
 import org.eclipse.hawkbit.repository.RolloutManagement.Create;
@@ -345,6 +344,37 @@ class MgmtRolloutResourceTest extends AbstractManagementApiIntegrationTest {
                 .andExpect(jsonPath("errorCode", equalTo("hawkbit.server.error.rest.body.notReadable")));
     }
 
+    /**
+     * Testing that creating rollout with insufficient permission returns forbidden
+     */
+    @Test
+    @WithUser(principal = "bumlux", authorities = {SpRole.TARGET_ADMIN, SpRole.REPOSITORY_ADMIN, "CREATE_ROLLOUT"})
+    void createRolloutWithSufficientPermissionsIsOk() throws Exception {
+        createRolloutWithPermissions("rollout-suff", 201);
+    }
+
+    @Test
+    @WithUser(authorities = {SpRole.TARGET_ADMIN, SpRole.REPOSITORY_ADMIN})
+    void createRolloutWithInsufficientPermissionReturnsForbidden() throws Exception {
+        createRolloutWithPermissions("rollout-insuff", 403);
+    }
+
+    private void createRolloutWithPermissions(final String name, final int expectedStatus) throws Exception {
+        testdataFactory.createTargets(20, "target", "rollout");
+        final DistributionSet dsA = testdataFactory.createDistributionSet("");
+        final String actionType = MgmtRestModelMapper.convertActionType(Action.ActionType.FORCED).getName();
+        final String rollout = JsonBuilder.rollout(name, "desc", 5, dsA.getId(), "id==target*",
+                new RolloutGroupConditionBuilder().withDefaults().build(), null, actionType, null, null, null,
+                null, false, null, 0);
+
+        mvc.perform(post("/rest/v1/rollouts")
+                        .contentType(MediaType.APPLICATION_JSON).accept(MediaType.APPLICATION_JSON)
+                        .content(rollout))
+                .andDo(MockMvcResultPrinter.print())
+                .andExpect(status().is(expectedStatus))
+                .andReturn();
+    }
+
     /**
      * Testing that creating rollout with not existing distribution set returns not found
      */
@@ -377,9 +407,7 @@ class MgmtRolloutResourceTest extends AbstractManagementApiIntegrationTest {
      */
     @Test
     void missingTargetFilterQueryInRollout() throws Exception {
-
         final String targetFilterQuery = null;
-
         final DistributionSet dsA = testdataFactory.createDistributionSet("");
         mvc.perform(post("/rest/v1/rollouts")
                         .content(JsonBuilder.rollout("rollout1", "desc", 10, dsA.getId(), targetFilterQuery, null))
@@ -397,7 +425,6 @@ class MgmtRolloutResourceTest extends AbstractManagementApiIntegrationTest {
     @Test
     void createRollout() throws Exception {
         testdataFactory.createTargets(20, "target", "rollout");
-
         final DistributionSet dsA = testdataFactory.createDistributionSet("");
         postRollout("rollout1", 5, dsA.getId(), "id==target*", 20, Action.ActionType.FORCED);
     }

hawkbit-mgmt/hawkbit-mgmt-resource/src/test/java/org/eclipse/hawkbit/mgmt/rest/resource/MgmtTargetResourceTest.java

@@ -415,7 +415,6 @@ class MgmtTargetResourceTest extends AbstractManagementApiIntegrationTest {
     @Test
     @WithUser(authorities = { SpPermission.READ_TARGET, SpPermission.CREATE_TARGET })
     void securityTokenIsNotInResponseIfMissingPermission() throws Exception {
-
         final String knownControllerId = "knownControllerId";
         testdataFactory.createTarget(knownControllerId);
         mvc.perform(get(TARGETS_V1 + "/{targetId}", knownControllerId))

hawkbit-repository/hawkbit-repository-api/src/main/java/org/eclipse/hawkbit/repository/TenantStatsManagement.java

@@ -10,7 +10,6 @@
 package org.eclipse.hawkbit.repository;
 
 import org.eclipse.hawkbit.auth.SpRole;
-import org.eclipse.hawkbit.auth.SpringEvalExpressions;
 import org.eclipse.hawkbit.repository.model.report.TenantUsage;
 import org.springframework.security.access.prepost.PreAuthorize;
 

hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/ArtifactManagementTest.java

@@ -50,7 +50,6 @@ import org.eclipse.hawkbit.repository.model.SoftwareModule;
 import org.eclipse.hawkbit.repository.test.matcher.Expect;
 import org.eclipse.hawkbit.repository.test.matcher.ExpectEvents;
 import org.eclipse.hawkbit.repository.test.util.SecurityContextSwitch;
-import org.eclipse.hawkbit.repository.test.util.WithUser;
 import org.junit.jupiter.api.Test;
 
 /**
@@ -411,14 +410,6 @@ class ArtifactManagementTest extends AbstractJpaIntegrationTest {
         }
     }
 
-    @Test
-    @WithUser(authorities = {})
-    void getArtifactBinaryWithoutDownloadArtifactThrowsPermissionDenied() {
-        assertThatExceptionOfType(InsufficientPermissionException.class)
-                .as("Should not have worked with missing permission.")
-                .isThrownBy(() -> artifactManagement.getArtifactStream("123", 1, false));
-    }
-
     /**
      * Verifies that creation of an artifact with none matching hashes fails.
      */
@@ -537,7 +528,7 @@ class ArtifactManagementTest extends AbstractJpaIntegrationTest {
     }
 
     private <T> T runAsTenant(final String tenant, final Callable<T> callable) throws Exception {
-        return SecurityContextSwitch.callAs(SecurityContextSwitch.withUserAndTenantAllSpPermissions("user", tenant), callable);
+        return SecurityContextSwitch.callAs(SecurityContextSwitch.withTenantAndUserAndAllPermissions(tenant, "user"), callable);
     }
 
     private SoftwareModule createSoftwareModuleForTenant(final String tenant) throws Exception {

hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/management/SystemManagementTest.java

@@ -10,7 +10,7 @@
 package org.eclipse.hawkbit.repository.jpa.management;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.eclipse.hawkbit.repository.test.util.SecurityContextSwitch.withUserAndTenant;
+import static org.eclipse.hawkbit.repository.test.util.SecurityContextSwitch.withTenantAndUser;
 
 import java.io.ByteArrayInputStream;
 import java.util.ArrayList;
@@ -57,7 +57,7 @@ class SystemManagementTest extends AbstractJpaIntegrationTest {
         for (int i = 0; i < tenants; i++) {
             final String tenantname = "TENANT" + i;
             SecurityContextSwitch.getAs(
-                    withUserAndTenant(tenantname, "bumlux", new String[] { SpRole.SYSTEM_ROLE }, true, true),
+                    withTenantAndUser(tenantname, "bumlux", new String[] { SpRole.SYSTEM_ROLE }, true, true),
                     () -> {
                         systemManagement.getTenantMetadataWithoutDetails();
                         if (artifactSize > 0) {

hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/tenancy/MultiTenancyEntityTest.java

@@ -71,7 +71,7 @@ class MultiTenancyEntityTest extends AbstractJpaIntegrationTest {
      * Ensures that targets created by a tenant are not visible by another tenant.
      */
     @Test
-    @WithUser(tenantId = "mytenant", authorities = SpRole.TENANT_ADMIN)
+    @WithUser(tenant = "mytenant", authorities = SpRole.TENANT_ADMIN)
     void queryTargetFromDifferentTenantIsNotVisible() throws Exception {
         // create target for another tenant
         final String anotherTenant = "anotherTenant";
@@ -93,7 +93,7 @@ class MultiTenancyEntityTest extends AbstractJpaIntegrationTest {
      * Ensures that tenant with proper permissions can read and delete other tenants.
      */
     @Test
-    @WithUser(tenantId = "mytenant", authorities = SpRole.TENANT_ADMIN)
+    @WithUser(tenant = "mytenant", authorities = SpRole.TENANT_ADMIN)
     void deleteAnotherTenantNotPossibleWithTenantPermissions() throws Exception {
         // create target for another tenant
         final String anotherTenant = "anotherTenant";
@@ -106,7 +106,7 @@ class MultiTenancyEntityTest extends AbstractJpaIntegrationTest {
     }
 
     @Test
-    @WithUser(tenantId = "mytenant", authorities = { SpRole.SYSTEM_ROLE })
+    @WithUser(tenant = "mytenant", authorities = { SpRole.SYSTEM_ROLE })
     void deleteAnotherTenantPossibleWithSystemRole() throws Exception {
         // create target for another tenant
         final String anotherTenant = "anotherTenant";
@@ -122,7 +122,7 @@ class MultiTenancyEntityTest extends AbstractJpaIntegrationTest {
      * Ensures that tenant metadata is retrieved for the current tenant.
      */
     @Test
-    @WithUser(tenantId = "mytenant", autoCreateTenant = false, authorities = SpRole.TENANT_ADMIN)
+    @WithUser(tenant = "mytenant", autoCreateTenant = false, authorities = SpRole.TENANT_ADMIN)
     void getTenantMetdata() throws Exception {
         // logged in tenant mytenant - check if tenant default data is autogenerated
         assertThat(distributionSetTypeManagement.findAll(PAGE)).isEmpty();
@@ -133,7 +133,7 @@ class MultiTenancyEntityTest extends AbstractJpaIntegrationTest {
 
         // check that the cache is not getting in the way, i.e. "bumlux" results in bumlux and not mytenant
         assertThat(SecurityContextSwitch.getAs(
-                SecurityContextSwitch.withUserAndTenantAllSpPermissions("user", "bumlux"),
+                SecurityContextSwitch.withTenantAndUserAndAllPermissions("bumlux", "user"),
                 () -> systemManagement.getTenantMetadataWithoutDetails().getTenant().toUpperCase()))
                 .isEqualTo("bumlux".toUpperCase());
     }
@@ -142,7 +142,7 @@ class MultiTenancyEntityTest extends AbstractJpaIntegrationTest {
      * Ensures that targets created from a different tenant cannot be deleted from other tenants
      */
     @Test
-    @WithUser(tenantId = "mytenant", authorities = SpRole.TENANT_ADMIN)
+    @WithUser(tenant = "mytenant", authorities = SpRole.TENANT_ADMIN)
     void deleteTargetFromOtherTenantIsNotPossible() throws Exception {
         // create target for another tenant
         final String anotherTenant = "anotherTenant";
@@ -191,7 +191,7 @@ class MultiTenancyEntityTest extends AbstractJpaIntegrationTest {
     }
 
     private <T> T runAsTenant(final String tenant, final Callable<T> callable) throws Exception {
-        return SecurityContextSwitch.callAs(SecurityContextSwitch.withUserAndTenantAllSpPermissions("user", tenant), callable);
+        return SecurityContextSwitch.callAs(SecurityContextSwitch.withTenantAndUserAndAllPermissions(tenant, "user"), callable);
     }
 
     private Target createTargetForTenant(final String controllerId, final String tenant) throws Exception {

hawkbit-repository/hawkbit-repository-test/src/main/java/org/eclipse/hawkbit/repository/test/util/AbstractIntegrationTest.java

@@ -38,7 +38,6 @@ import org.awaitility.Awaitility;
 import org.awaitility.core.ConditionFactory;
 import org.eclipse.hawkbit.artifact.ArtifactStorage;
 import org.eclipse.hawkbit.artifact.exception.ArtifactStoreException;
-import org.eclipse.hawkbit.auth.SpRole;
 import org.eclipse.hawkbit.repository.ArtifactManagement;
 import org.eclipse.hawkbit.repository.ConfirmationManagement;
 import org.eclipse.hawkbit.repository.ControllerManagement;

hawkbit-repository/hawkbit-repository-test/src/main/java/org/eclipse/hawkbit/repository/test/util/SecurityContextSwitch.java

@@ -67,7 +67,7 @@ public class SecurityContextSwitch {
         setSecurityContext(withUser);
         try {
             if (withUser.autoCreateTenant()) {
-                createTenant(withUser.tenantId());
+                createTenant(withUser.tenant());
             }
             return callable.call();
         } finally {
@@ -93,18 +93,18 @@ public class SecurityContextSwitch {
     }
 
     public static WithUser withController(final String principal, final String... authorities) {
-        return withUserAndTenant(DEFAULT_TENANT, principal, authorities, true, true);
+        return withTenantAndUser(DEFAULT_TENANT, principal, authorities, true, true);
     }
 
     public static WithUser withUser(final String principal, final String... authorities) {
-        return withUserAndTenant(DEFAULT_TENANT, principal, authorities, false, true);
+        return withTenantAndUser(DEFAULT_TENANT, principal, authorities, false, true);
     }
 
-    public static WithUser withUserAndTenantAllSpPermissions(final String principal, final String tenant) {
+    public static WithUser withTenantAndUserAndAllPermissions(final String tenant, final String principal) {
-        return withUserAndTenant(tenant, principal, new String[] { SpRole.TENANT_ADMIN }, false, true);
+        return withTenantAndUser(tenant, principal, new String[] { SpRole.TENANT_ADMIN }, false, true);
     }
 
-    public static WithUser withUserAndTenant(
+    public static WithUser withTenantAndUser(
             final String tenant, final String principal, final String[] authorities,
             final boolean controller, final boolean autoCreateTenant) {
         return new WithUserImpl(tenant, principal, authorities, controller, autoCreateTenant);
@@ -114,11 +114,11 @@ public class SecurityContextSwitch {
         SecurityContextHolder.setContext(new WithUserSecurityContext(annotation));
     }
 
-    private static void createTenant(final String tenantId) {
+    private static void createTenant(final String tenant) {
         final SecurityContext oldContext = SecurityContextHolder.getContext();
         setSecurityContext(PRIVILEGED_USER);
         try {
-            systemManagement.createTenantMetadata(tenantId);
+            systemManagement.createTenantMetadata(tenant);
         } finally {
             SecurityContextHolder.setContext(oldContext);
         }
@@ -140,17 +140,17 @@ public class SecurityContextSwitch {
         WithUserSecurityContext(final WithUser annotation) {
             this.annotation = annotation;
             if (annotation.autoCreateTenant()) {
-                createTenant(annotation.tenantId());
+                createTenant(annotation.tenant());
             }
         }
 
         @Override
         public Authentication getAuthentication() {
             final TestingAuthenticationToken testingAuthenticationToken = new TestingAuthenticationToken(
-                    new TenantAwareUser(annotation.principal(), "***", null, annotation.tenantId()),
+                    new TenantAwareUser(annotation.principal(), "***", null, annotation.tenant()),
                     annotation.credentials(), annotation.authorities());
             testingAuthenticationToken.setDetails(
-                    new TenantAwareAuthenticationDetails(annotation.tenantId(), annotation.controller()));
+                    new TenantAwareAuthenticationDetails(annotation.tenant(), annotation.controller()));
             return testingAuthenticationToken;
         }
 
@@ -201,7 +201,7 @@ public class SecurityContextSwitch {
         }
 
         @Override
-        public String tenantId() {
+        public String tenant() {
             return tenant;
         }
 

hawkbit-repository/hawkbit-repository-test/src/main/java/org/eclipse/hawkbit/repository/test/util/WithUser.java

@@ -33,7 +33,7 @@ public @interface WithUser {
      *
      * @return test tenant id
      */
-    String tenantId() default "DEFAULT";
+    String tenant() default "DEFAULT";
 
     /**
      * Gets the test principal.

hawkbit-sdk/hawkbit-sdk-dmf/src/main/java/org/eclipse/hawkbit/sdk/dmf/amqp/VHost.java

@@ -119,7 +119,12 @@ public final class VHost extends DmfSender implements MessageListener {
         }
     }
 
-    protected void handleAttributeUpdateRequest(final Message message, final String controllerId) {
+    void stop() {
+        container.stop();
+        rabbitTemplate.destroy();
+    }
+
+    private void handleAttributeUpdateRequest(final Message message, final String controllerId) {
         final String tenantId = getTenant(message);
         Optional.ofNullable(dmfTenants.get(tenantId))
                 .flatMap(dmfTenant -> dmfTenant.getController(controllerId))
@@ -127,12 +132,12 @@ public final class VHost extends DmfSender implements MessageListener {
                         updateAttributes(tenantId, controllerId, DmfUpdateMode.MERGE, controller.getAttributes()));
     }
 
-    protected void handleCancelDownloadAction(final Message message, final String thingId) {
+    private void handleCancelDownloadAction(final Message message, final String thingId) {
         final Long actionId = extractActionIdFrom(message);
         processCancelDownloadAction(thingId, actionId);
     }
 
-    protected void handleUpdateProcess(final Message message, final String controllerId, final EventTopic actionType) {
+    private void handleUpdateProcess(final Message message, final String controllerId, final EventTopic actionType) {
         final String tenant = getTenant(message);
         final DmfDownloadAndUpdateRequest downloadAndUpdateRequest = convertMessage(message,
                 DmfDownloadAndUpdateRequest.class);
@@ -141,11 +146,6 @@ public final class VHost extends DmfSender implements MessageListener {
         processUpdate(tenant, controllerId, actionType, downloadAndUpdateRequest);
     }
 
-    void stop() {
-        container.stop();
-        rabbitTemplate.destroy();
-    }
-
     private static String getTenant(final Message message) {
         final MessageProperties messageProperties = message.getMessageProperties();
         final Map<String, Object> headers = messageProperties.getHeaders();
@@ -252,9 +252,9 @@ public final class VHost extends DmfSender implements MessageListener {
         openActions.remove(actionId);
     }
 
-    private void processUpdate(final String tenantId, final String controllerId, final EventTopic actionType,
+    private void processUpdate(final String tenant, final String controllerId, final EventTopic actionType,
             final DmfDownloadAndUpdateRequest updateRequest) {
-        Optional.ofNullable(dmfTenants.get(tenantId))
+        Optional.ofNullable(dmfTenants.get(tenant))
                 .flatMap(dmfTenant -> dmfTenant.getController(controllerId))
                 .ifPresent(controller -> controller.processUpdate(actionType, updateRequest));
     }