From c4fd082098b01228cd0e719cd94743ac87b594bc Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 09:39:07 +0200 Subject: [PATCH 1/8] fix the lookup of artifacts with the same lookup key, e.g. SHA1 hash if multiple artifacts with key exists for different software modules Signed-off-by: Michael Hirsch --- .../amqp/AmqpMessageHandlerService.java | 14 +++-- .../repository/ControllerManagement.java | 27 +++++++++ .../specifications/ActionSpecifications.java | 60 +++++++++++++++++++ 3 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java diff --git a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java index 41e70f91f..e05ddf71d 100644 --- a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java +++ b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java @@ -166,6 +166,8 @@ public class AmqpMessageHandlerService extends BaseAmqpService { final LocalArtifact localArtifact = findLocalArtifactByFileResource(fileResource); if (localArtifact == null) { + LOG.info("target {} requested file resource which does not exists to download", + secruityToken.getControllerId(), fileResource); throw new EntityNotFoundException(); } @@ -175,10 +177,14 @@ public class AmqpMessageHandlerService extends BaseAmqpService { // assigned to this controller. Otherwise no controllerId is set = // anonymous download if (secruityToken.getControllerId() != null) { - final Action action = controllerManagement.getActionForDownloadByTargetAndSoftwareModule( - secruityToken.getControllerId(), localArtifact.getSoftwareModule()); - LOG.info("Found action for download authentication request action: {}, resource: {}", action, - secruityToken.getFileResource()); + LOG.debug("no anonymous download request, doing authentication check for target {} and artifact {}", + secruityToken.getControllerId(), localArtifact); + if (!controllerManagement.hasTargetArtifactAssigned(secruityToken.getControllerId(), localArtifact)) { + LOG.info("target {} tried to download artifact {} which is not assigned to the target"); + throw new EntityNotFoundException(); + } + LOG.info("download security check for target {} and artifact {} granted", + secruityToken.getControllerId(), localArtifact); } final Artifact artifact = convertDbArtifact(artifactManagement.loadLocalArtifactBinary(localArtifact)); diff --git a/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java index 0bb94bd72..9510ae899 100644 --- a/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java +++ b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java @@ -28,12 +28,14 @@ import org.eclipse.hawkbit.repository.model.Action.Status; import org.eclipse.hawkbit.repository.model.ActionStatus; import org.eclipse.hawkbit.repository.model.ActionStatus_; import org.eclipse.hawkbit.repository.model.DistributionSet; +import org.eclipse.hawkbit.repository.model.LocalArtifact; import org.eclipse.hawkbit.repository.model.SoftwareModule; import org.eclipse.hawkbit.repository.model.Target; import org.eclipse.hawkbit.repository.model.TargetInfo; import org.eclipse.hawkbit.repository.model.TargetUpdateStatus; import org.eclipse.hawkbit.repository.model.Target_; import org.eclipse.hawkbit.repository.model.TenantConfiguration; +import org.eclipse.hawkbit.repository.specifications.ActionSpecifications; import org.eclipse.hawkbit.security.HawkbitSecurityProperties; import org.eclipse.hawkbit.tenancy.configuration.TenantConfigurationKey; import org.hibernate.validator.constraints.NotEmpty; @@ -166,6 +168,31 @@ public class ControllerManagement { return action.get(0); } + /** + * Checks if a given target has currently or has even been assigned to the + * given artifact through the action history list. This can e.g. indicate if + * a target is allowed to download a given artifact because it has currently + * assigned or had ever been assigned to the target and so it's visible to a + * specific target e.g. for downloading. + * + * @param targetId + * the ID of the target to check + * @param localArtifact + * the artifact to verify if the given target had even been + * assigned to + * @return {@code true} if the given target has currently or had ever a + * relation to the given artifact through the action history, + * otherwise {@code false} + */ + public boolean hasTargetArtifactAssigned(@NotNull final String targetId, + @NotNull final LocalArtifact localArtifact) { + final Target target = targetRepository.findByControllerId(targetId); + if (target == null) { + return false; + } + return actionRepository.count(ActionSpecifications.hasTargetArtifactAssigned(target, localArtifact)) > 0; + } + /** * Refreshes the time of the last time the controller has been connected to * the server. diff --git a/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java new file mode 100644 index 000000000..ef8c97e22 --- /dev/null +++ b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java @@ -0,0 +1,60 @@ +/** + * Copyright (c) 2015 Bosch Software Innovations GmbH and others. + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + */ +package org.eclipse.hawkbit.repository.specifications; + +import javax.persistence.criteria.Join; +import javax.persistence.criteria.ListJoin; +import javax.persistence.criteria.SetJoin; + +import org.eclipse.hawkbit.repository.model.Action; +import org.eclipse.hawkbit.repository.model.Action_; +import org.eclipse.hawkbit.repository.model.DistributionSet; +import org.eclipse.hawkbit.repository.model.DistributionSet_; +import org.eclipse.hawkbit.repository.model.LocalArtifact; +import org.eclipse.hawkbit.repository.model.LocalArtifact_; +import org.eclipse.hawkbit.repository.model.SoftwareModule; +import org.eclipse.hawkbit.repository.model.SoftwareModule_; +import org.eclipse.hawkbit.repository.model.Target; +import org.springframework.data.jpa.domain.Specification; + +/** + * Utility class for {@link Action}s {@link Specification}s. The class provides + * Spring Data JPQL Specifications. + * + */ +public class ActionSpecifications { + + private ActionSpecifications() { + // utility class + } + + /** + * Specification which joins all necessary tables to retrieve the dependency + * between a target and a local file assignment through the assigen action + * of the target. All actions are included, not only active actions. + * + * @param target + * the target to verfiy if the given artifact is currently + * assigned or had been assigned + * @param localArtifact + * the local artifact to check wherever the target had ever been + * assigned + * @return a specification to use with spring JPA + */ + public static Specification hasTargetArtifactAssigned(final Target target, + final LocalArtifact localArtifact) { + return (actionRoot, query, cb) -> { + final Join dsJoin = actionRoot.join(Action_.distributionSet); + final SetJoin modulesJoin = dsJoin.join(DistributionSet_.modules); + final ListJoin artifactsJoin = modulesJoin.join(SoftwareModule_.artifacts); + return cb.and(cb.equal(artifactsJoin.get(LocalArtifact_.id), localArtifact.getId()), + cb.equal(actionRoot.get(Action_.target), target)); + }; + } +} From 0b5286299c7a2453a635f8a7d64eac33cb99d46d Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 10:03:19 +0200 Subject: [PATCH 2/8] fix unit tests after changing the lookup way Signed-off-by: Michael Hirsch --- .../eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java b/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java index 930ca02ad..0faa0a8a6 100644 --- a/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java +++ b/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java @@ -313,11 +313,9 @@ public class AmqpMessageHandlerServiceTest { // mock final LocalArtifact localArtifactMock = mock(LocalArtifact.class); - final Action actionMock = mock(Action.class); final DbArtifact dbArtifactMock = mock(DbArtifact.class); when(artifactManagementMock.findFirstLocalArtifactsBySHA1(anyString())).thenReturn(localArtifactMock); - when(controllerManagementMock.getActionForDownloadByTargetAndSoftwareModule(anyObject(), anyObject())) - .thenReturn(actionMock); + when(controllerManagementMock.hasTargetArtifactAssigned(anyObject(), anyObject())).thenReturn(true); when(artifactManagementMock.loadLocalArtifactBinary(localArtifactMock)).thenReturn(dbArtifactMock); when(dbArtifactMock.getArtifactId()).thenReturn("artifactId"); when(dbArtifactMock.getSize()).thenReturn(1L); From b11a0bac5db51e38685d77ca15669e4042069ba4 Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 12:18:59 +0200 Subject: [PATCH 3/8] reduce method cyclomatic complexity by extracting security check into own method Signed-off-by: Michael Hirsch --- .../amqp/AmqpMessageHandlerService.java | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java index e05ddf71d..47807cd4d 100644 --- a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java +++ b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java @@ -171,21 +171,7 @@ public class AmqpMessageHandlerService extends BaseAmqpService { throw new EntityNotFoundException(); } - // check action for this download purposes, the method will throw an - // EntityNotFoundException in case the controller is not allowed to - // download this file because it's not assigned to an action and not - // assigned to this controller. Otherwise no controllerId is set = - // anonymous download - if (secruityToken.getControllerId() != null) { - LOG.debug("no anonymous download request, doing authentication check for target {} and artifact {}", - secruityToken.getControllerId(), localArtifact); - if (!controllerManagement.hasTargetArtifactAssigned(secruityToken.getControllerId(), localArtifact)) { - LOG.info("target {} tried to download artifact {} which is not assigned to the target"); - throw new EntityNotFoundException(); - } - LOG.info("download security check for target {} and artifact {} granted", - secruityToken.getControllerId(), localArtifact); - } + checkIfArtifactIsAssignedToTarget(secruityToken, localArtifact); final Artifact artifact = convertDbArtifact(artifactManagement.loadLocalArtifactBinary(localArtifact)); if (artifact == null) { @@ -219,6 +205,25 @@ public class AmqpMessageHandlerService extends BaseAmqpService { return getMessageConverter().toMessage(authentificationResponse, messageProperties); } + private void checkIfArtifactIsAssignedToTarget(final TenantSecurityToken secruityToken, + final LocalArtifact localArtifact) { + // check action for this download purposes, the method will throw an + // EntityNotFoundException in case the controller is not allowed to + // download this file because it's not assigned to an action and not + // assigned to this controller. Otherwise no controllerId is set = + // anonymous download + if (secruityToken.getControllerId() != null) { + LOG.debug("no anonymous download request, doing authentication check for target {} and artifact {}", + secruityToken.getControllerId(), localArtifact); + if (!controllerManagement.hasTargetArtifactAssigned(secruityToken.getControllerId(), localArtifact)) { + LOG.info("target {} tried to download artifact {} which is not assigned to the target"); + throw new EntityNotFoundException(); + } + LOG.info("download security check for target {} and artifact {} granted", secruityToken.getControllerId(), + localArtifact); + } + } + private LocalArtifact findLocalArtifactByFileResource(final FileResource fileResource) { if (fileResource.getSha1() != null) { return artifactManagement.findFirstLocalArtifactsBySHA1(fileResource.getSha1()); From 369d9da25a3c4d6d3cf703d0013567ce2eff867b Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 14:35:59 +0200 Subject: [PATCH 4/8] source code improvements Signed-off-by: Michael Hirsch --- .../amqp/AmqpMessageHandlerService.java | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java index 47807cd4d..994ef244a 100644 --- a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java +++ b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java @@ -205,23 +205,32 @@ public class AmqpMessageHandlerService extends BaseAmqpService { return getMessageConverter().toMessage(authentificationResponse, messageProperties); } + /** + * check action for this download purposes, the method will throw an + * EntityNotFoundException in case the controller is not allowed to download + * this file because it's not assigned to an action and not assigned to this + * controller. Otherwise no controllerId is set = anonymous download + * + * @param secruityToken + * the security token which holds the target ID to check on + * @param localArtifact + * the local artifact to verify if the given target is allowed to + * download this artifact + */ private void checkIfArtifactIsAssignedToTarget(final TenantSecurityToken secruityToken, final LocalArtifact localArtifact) { - // check action for this download purposes, the method will throw an - // EntityNotFoundException in case the controller is not allowed to - // download this file because it's not assigned to an action and not - // assigned to this controller. Otherwise no controllerId is set = - // anonymous download - if (secruityToken.getControllerId() != null) { - LOG.debug("no anonymous download request, doing authentication check for target {} and artifact {}", - secruityToken.getControllerId(), localArtifact); - if (!controllerManagement.hasTargetArtifactAssigned(secruityToken.getControllerId(), localArtifact)) { - LOG.info("target {} tried to download artifact {} which is not assigned to the target"); - throw new EntityNotFoundException(); - } - LOG.info("download security check for target {} and artifact {} granted", secruityToken.getControllerId(), - localArtifact); + final String controllerId = secruityToken.getControllerId(); + if (controllerId == null) { + LOG.info("anonymous download no authentication check for artifact {}", localArtifact); + return; } + LOG.debug("no anonymous download request, doing authentication check for target {} and artifact {}", + controllerId, localArtifact); + if (!controllerManagement.hasTargetArtifactAssigned(controllerId, localArtifact)) { + LOG.info("target {} tried to download artifact {} which is not assigned to the target"); + throw new EntityNotFoundException(); + } + LOG.info("download security check for target {} and artifact {} granted", controllerId, localArtifact); } private LocalArtifact findLocalArtifactByFileResource(final FileResource fileResource) { From cc8c6743415be5b01cd80cffd8da669611acd547 Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 15:47:21 +0200 Subject: [PATCH 5/8] correct usage of logging, missing parameters Signed-off-by: Michael Hirsch --- .../org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java index 994ef244a..2ee0a0f45 100644 --- a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java +++ b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java @@ -227,7 +227,8 @@ public class AmqpMessageHandlerService extends BaseAmqpService { LOG.debug("no anonymous download request, doing authentication check for target {} and artifact {}", controllerId, localArtifact); if (!controllerManagement.hasTargetArtifactAssigned(controllerId, localArtifact)) { - LOG.info("target {} tried to download artifact {} which is not assigned to the target"); + LOG.info("target {} tried to download artifact {} which is not assigned to the target", controllerId, + localArtifact); throw new EntityNotFoundException(); } LOG.info("download security check for target {} and artifact {} granted", controllerId, localArtifact); From 30e8230b8aaa55e1e3834c1d9eeed019454d5f99 Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 15:48:19 +0200 Subject: [PATCH 6/8] correct usage of logging, missing parameters Signed-off-by: Michael Hirsch --- .../org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java index 2ee0a0f45..447a8ffca 100644 --- a/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java +++ b/hawkbit-dmf-amqp/src/main/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerService.java @@ -166,7 +166,7 @@ public class AmqpMessageHandlerService extends BaseAmqpService { final LocalArtifact localArtifact = findLocalArtifactByFileResource(fileResource); if (localArtifact == null) { - LOG.info("target {} requested file resource which does not exists to download", + LOG.info("target {} requested file resource {} which does not exists to download", secruityToken.getControllerId(), fileResource); throw new EntityNotFoundException(); } From d903be57b7a9913c29db8e8729efa88f5fab4666 Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 15:50:48 +0200 Subject: [PATCH 7/8] use correct mock parameters to ensure test Signed-off-by: Michael Hirsch --- .../eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java b/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java index 0faa0a8a6..19a7be3bc 100644 --- a/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java +++ b/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/amqp/AmqpMessageHandlerServiceTest.java @@ -315,7 +315,8 @@ public class AmqpMessageHandlerServiceTest { final LocalArtifact localArtifactMock = mock(LocalArtifact.class); final DbArtifact dbArtifactMock = mock(DbArtifact.class); when(artifactManagementMock.findFirstLocalArtifactsBySHA1(anyString())).thenReturn(localArtifactMock); - when(controllerManagementMock.hasTargetArtifactAssigned(anyObject(), anyObject())).thenReturn(true); + when(controllerManagementMock.hasTargetArtifactAssigned(securityToken.getControllerId(), localArtifactMock)) + .thenReturn(true); when(artifactManagementMock.loadLocalArtifactBinary(localArtifactMock)).thenReturn(dbArtifactMock); when(dbArtifactMock.getArtifactId()).thenReturn("artifactId"); when(dbArtifactMock.getSize()).thenReturn(1L); From d704ef99fd1a3ee0f8152f933ee7c9f8ed610901 Mon Sep 17 00:00:00 2001 From: Michael Hirsch Date: Tue, 12 Apr 2016 16:22:43 +0200 Subject: [PATCH 8/8] source code hygiene, better method naming and variable naming Signed-off-by: Michael Hirsch --- .../eclipse/hawkbit/repository/ControllerManagement.java | 2 +- .../repository/specifications/ActionSpecifications.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java index 9510ae899..dc097a7e3 100644 --- a/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java +++ b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/ControllerManagement.java @@ -190,7 +190,7 @@ public class ControllerManagement { if (target == null) { return false; } - return actionRepository.count(ActionSpecifications.hasTargetArtifactAssigned(target, localArtifact)) > 0; + return actionRepository.count(ActionSpecifications.hasTargetAssignedArtifact(target, localArtifact)) > 0; } /** diff --git a/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java index ef8c97e22..1ac3afb67 100644 --- a/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java +++ b/hawkbit-repository/src/main/java/org/eclipse/hawkbit/repository/specifications/ActionSpecifications.java @@ -47,14 +47,14 @@ public class ActionSpecifications { * assigned * @return a specification to use with spring JPA */ - public static Specification hasTargetArtifactAssigned(final Target target, + public static Specification hasTargetAssignedArtifact(final Target target, final LocalArtifact localArtifact) { - return (actionRoot, query, cb) -> { + return (actionRoot, query, criteriaBuilder) -> { final Join dsJoin = actionRoot.join(Action_.distributionSet); final SetJoin modulesJoin = dsJoin.join(DistributionSet_.modules); final ListJoin artifactsJoin = modulesJoin.join(SoftwareModule_.artifacts); - return cb.and(cb.equal(artifactsJoin.get(LocalArtifact_.id), localArtifact.getId()), - cb.equal(actionRoot.get(Action_.target), target)); + return criteriaBuilder.and(criteriaBuilder.equal(artifactsJoin.get(LocalArtifact_.id), localArtifact.getId()), + criteriaBuilder.equal(actionRoot.get(Action_.target), target)); }; } }