Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,14 @@
}

private static List<SoftwareSchedulePackage> toPackages(List<SoftwareSchedulePackageInput> packages) {
List<String> brewMissingType = packages.stream()
.filter(p -> p.getPackageManager() == PackageManagerType.BREW && p.getBrewPackageType() == null)

Check notice on line 281 in openframe-api-lib/src/main/java/com/openframe/api/service/rmm/software/SoftwareScheduleService.java

View check run for this annotation

Flamingo / Flamingo Code Review / tests and mutation

No test reaches this line: NegateConditionalsMutator

negated conditional (in lambda$toPackages$0). No test runs this line.

Check notice on line 281 in openframe-api-lib/src/main/java/com/openframe/api/service/rmm/software/SoftwareScheduleService.java

View check run for this annotation

Flamingo / Flamingo Code Review / tests and mutation

No test reaches this line: NegateConditionalsMutator

negated conditional (in lambda$toPackages$0). No test runs this line.

Check notice on line 281 in openframe-api-lib/src/main/java/com/openframe/api/service/rmm/software/SoftwareScheduleService.java

View check run for this annotation

Flamingo / Flamingo Code Review / tests and mutation

No test reaches this line: BooleanTrueReturnValsMutator

replaced boolean return with true for com/openframe/api/service/rmm/software/SoftwareScheduleService::lambda$toPackages$0 (in lambda$toPackages$0). No test runs this line.
.map(SoftwareSchedulePackageInput::getPackageName)
.toList();
if (!brewMissingType.isEmpty()) {

Check notice on line 284 in openframe-api-lib/src/main/java/com/openframe/api/service/rmm/software/SoftwareScheduleService.java

View check run for this annotation

Flamingo / Flamingo Code Review / tests and mutation

No test reaches this line: NegateConditionalsMutator

negated conditional (in toPackages). No test runs this line.
throw new BadRequestException("brewPackageType (CASK or FORMULA) is required for brew packages: "
+ brewMissingType);
}
return packages.stream()
.map(p -> SoftwareSchedulePackage.builder()
.packageManager(p.getPackageManager())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import graphql.GraphQLError;
import graphql.execution.DataFetcherExceptionHandlerParameters;
import graphql.execution.DataFetcherExceptionHandlerResult;
import graphql.execution.ResultPath;
import graphql.execution.SimpleDataFetcherExceptionHandler;
import jakarta.validation.ConstraintViolationException;
import java.util.stream.Collectors;
Expand All @@ -32,19 +33,20 @@ public CompletableFuture<DataFetcherExceptionHandlerResult> handleException(
DataFetcherExceptionHandlerParameters handlerParameters) {

Throwable exception = handlerParameters.getException();
ResultPath path = handlerParameters.getPath();
if (exception instanceof AccessDeniedException) {
ErrorCode code = AccessDeniedErrorCode.forCurrentCaller();
log.warn("GraphQL access denied ({}): {}", code.getCode(), exception.getMessage());
return result(buildError(AccessDeniedErrorCode.MESSAGE, code));
return result(buildError(AccessDeniedErrorCode.MESSAGE, code), path);
}
if (exception instanceof InvalidRelayIdException) {
String message = exception.getMessage();
log.warn("GraphQL invalid id: {}", message);
return result(buildError(message, ErrorCode.INVALID_ID));
return result(buildError(message, ErrorCode.INVALID_ID), path);
}
log.error("GraphQL error occurred", exception);

GraphQLError error;
GraphQLError.Builder<?> error;

if (exception instanceof PinotQueryException) {
error = buildError("Query failed. Please try again later.", ErrorCode.PINOT_QUERY_ERROR);
Expand Down Expand Up @@ -73,13 +75,14 @@ public CompletableFuture<DataFetcherExceptionHandlerResult> handleException(
error = buildError("An unexpected error occurred. Please try again later.", ErrorCode.INTERNAL_ERROR);
}

return result(error);
return result(error, path);
}

private static CompletableFuture<DataFetcherExceptionHandlerResult> result(GraphQLError error) {
// The path is how graphql-java knows a null non-null field already has its error; without it it adds a second one.
private static CompletableFuture<DataFetcherExceptionHandlerResult> result(GraphQLError.Builder<?> error, ResultPath path) {
return CompletableFuture.completedFuture(
DataFetcherExceptionHandlerResult.newResult()
.error(error)
.error(error.path(path).build())
.build()
);
}
Expand All @@ -91,14 +94,13 @@ private static String validationMessage(ConstraintViolationException exception)
.collect(Collectors.joining("; "));
}

private GraphQLError buildError(String message, ErrorCode errorCode) {
private GraphQLError.Builder<?> buildError(String message, ErrorCode errorCode) {
return GraphQLError.newError()
.message(message)
.extensions(Map.of(
"code", errorCode.getCode(),
"httpStatus", errorCode.getHttpStatus(),
"timestamp", System.currentTimeMillis()
))
.build();
));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,18 @@ void handleException_callerWithTheRole_isAnswered() {
assertThat(result.<Map<String, Object>>getData()).containsEntry("adminOnly", "granted");
}

@Test
void handleException_onANonNullField_answersOneErrorAtItsPath() {
signIn("ADMIN");

ExecutionResult result = queryExecutor.execute("{ required }");

assertThat(result.getErrors())
.singleElement()
.returns("countdownSeconds must be positive", GraphQLError::getMessage)
.returns(List.of("required"), GraphQLError::getPath);
}

private static void signIn(String role) {
Jwt jwt = Jwt.withTokenValue("token")
.header("alg", "none")
Expand All @@ -102,5 +114,10 @@ static class AdminOnlyDataFetcher {
String adminOnly() {
return "granted";
}

@DgsQuery
String required() {
throw new IllegalArgumentException("countdownSeconds must be positive");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@
import com.openframe.data.loki.client.LokiQueryRejectedException;
import com.openframe.data.pinot.repository.exception.PinotQueryException;
import graphql.GraphQLError;
import graphql.Scalars;
import graphql.execution.DataFetcherExceptionHandlerParameters;
import graphql.execution.ExecutionStepInfo;
import graphql.execution.ResultPath;
import graphql.schema.DataFetchingEnvironment;
import jakarta.validation.ConstraintViolationException;
import jakarta.validation.Validation;
import jakarta.validation.Validator;
Expand Down Expand Up @@ -39,6 +43,8 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.InstanceOfAssertFactories.MAP;
import static org.junit.jupiter.params.provider.Arguments.arguments;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@ExtendWith(OutputCaptureExtension.class)
class GraphQLExceptionHandlerTest {
Expand Down Expand Up @@ -226,8 +232,14 @@ private static void signIn() {
}

private List<GraphQLError> handle(Throwable exception) {
DataFetchingEnvironment environment = mock(DataFetchingEnvironment.class);
when(environment.getExecutionStepInfo()).thenReturn(ExecutionStepInfo.newExecutionStepInfo()
.type(Scalars.GraphQLString)
.path(ResultPath.rootPath())
.build());
DataFetcherExceptionHandlerParameters parameters = DataFetcherExceptionHandlerParameters
.newExceptionParameters()
.dataFetchingEnvironment(environment)
.exception(exception)
.build();
return handler.handleException(parameters).join().getErrors();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,34 @@ void createBadRepeatRejected() {
.isInstanceOf(BadRequestException.class);
}

@Test
@DisplayName("create: a brew package without a type is rejected, a winget one needs none")
void createBrewWithoutTypeRejected() {
createInput.setPackages(List.of(pkg(PackageManagerType.WINGET, "vscode", null), pkg(PackageManagerType.BREW, "slack", null)));

assertThatThrownBy(() -> service.create(createInput, ACTOR))
.isInstanceOf(BadRequestException.class)
.hasMessageContaining("[slack]");
verify(scheduleRepository, never()).save(any());
}

@Test
@DisplayName("update: a brew package without a type is rejected")
void updateBrewWithoutTypeRejected() {
when(scheduleRepository.findByTenantIdAndId(TENANT_ID, SCHEDULE_ID)).thenReturn(Optional.of(existingActive()));
UpdateSoftwareScheduleInput input = new UpdateSoftwareScheduleInput();
input.setId(SCHEDULE_ID);
input.setName("Renamed");
input.setAction(SoftwareAction.UPDATE);
input.setPackages(List.of(pkg(PackageManagerType.BREW, "slack", null)));
input.setStartAt(Instant.parse("2026-09-16T03:30:00Z"));

assertThatThrownBy(() -> service.update(input, ACTOR))
.isInstanceOf(BadRequestException.class)
.hasMessageContaining("[slack]");
verify(scheduleRepository, never()).save(any());
}

@Test
@DisplayName("create: empty machineIds clears assignment without saving rows")
void createNoDevices() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
type Query {
adminOnly: String
required: String!
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
// failures that can pass by themselves; any other stays FAILED until the next version
private static final Set<String> RETRIED_REASONS = Set.of(Failed.Reason.MANAGER_ERROR.name(), Failed.Reason.TIMEOUT.name(),
Failed.Reason.CLOSE_FAILED.name(), PackageUpdateDeviceStateReducer.NOT_DELIVERED);
private static final String TURNED_OFF_BY = "package-auto-update";

private final PackageAutoUpdatePolicyRepository policyRepository;
private final DeliveryDispatcher deliveryDispatcher;
Expand All @@ -54,6 +55,18 @@
request.getRequestId(), machineId));
}

public void turnOffIfPackageRemoved(PackageUpdateDeviceState state, PackageUpdateRequest request) {
if (request == null || request.getSource() != PackageUpdateSource.AUTO
|| !Failed.Reason.NOT_INSTALLED.name().equals(state.getFailedReason())) {
return;
}
currentPolicy(request, state.getMachineId()).ifPresent(policy -> {
policyRepository.disable(policy.getId(), TURNED_OFF_BY, Instant.now());
log.info("[package-update] auto-update turned off, the package is not on the device requestId={} machineId={} package={}/{}",
request.getRequestId(), state.getMachineId(), request.getPackageManager(), request.getPackageName());
});
}

Check warning on line 68 in openframe-client-core/src/main/java/com/openframe/client/listener/packageupdate/PackageAutoUpdateRetry.java

View check run for this annotation

Flamingo / Flamingo Code Review / tests and mutation

Auto-update turns off silently on a single NOT_INSTALLED answer

One NOT_INSTALLED answer to an AUTO order disables the device's policy, and the only trace is a log line and updatedBy. Whether the agent's probe can report NOT_INSTALLED for a package that is installed (for example for another user) needs verification on a device. Risk: A false answer stops updates for that package on that device with no notice to the admin beyond Auto-update showing OFF, so the device can quietly fall behind on security updates. Fix: Check the probe on brew and winget during BE-7 before this reaches stage or prod, and surface the turn-off to admins when the software notifications land.
Comment thread
andrii-flamingo marked this conversation as resolved.
Resolved

private void schedule(PackageUpdateDeviceState state, PackageUpdateRequest request, PackageAutoUpdatePolicy policy) {
String machineId = state.getMachineId();
String requestId = request.getRequestId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ public void process(String machineId, PackageUpdateStatusMessage event) {
PackageUpdateDeviceStateReducer.apply(state, request, event);
driveDeadlineForce(state, request, deadlineBefore, machineId);
autoUpdateRetry.scheduleIfDue(state, request);
autoUpdateRetry.turnOffIfPackageRemoved(state, request);
executionRecorder.record(request, machineId, event);
deviceStateRepository.save(state);
log.info("[package-update] status applied requestId={} machineId={} eventId={} state={} status={} "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.within;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -178,6 +179,61 @@ void noRequest_noRetry() {
nothingScheduled();
}

@Test
void packageNotOnTheDevice_policyTurnedOff() {
retry.turnOffIfPackageRemoved(failed("NOT_INSTALLED"), request);

verify(policyRepository).disable(eq("p1"), eq("package-auto-update"), any());
}

@ParameterizedTest
@NullSource
@ValueSource(strings = {"NOT_UPGRADABLE", "MANAGER_ERROR", "TIMEOUT"})
void otherFailure_policyStaysOn(String reason) {
retry.turnOffIfPackageRemoved(failed(reason), request);

policyNotTurnedOff();
}

@Test
void anAdminsOrderFindingNoPackage_policyStaysOn() {
request.setSource(PackageUpdateSource.ADMIN);

retry.turnOffIfPackageRemoved(failed("NOT_INSTALLED"), request);

policyNotTurnedOff();
}

@Test
void olderVersionFindingNoPackage_policyStaysOn() {
// the package may be back by the time a newer version was ordered
policy.setLastVersion("153.0.0.1");

retry.turnOffIfPackageRemoved(failed("NOT_INSTALLED"), request);

policyNotTurnedOff();
}

@Test
void policyAlreadyOff_notWrittenAgain() {
policy.setEnabled(false);

retry.turnOffIfPackageRemoved(failed("NOT_INSTALLED"), request);

policyNotTurnedOff();
}

@Test
void noRequest_policyStaysOn() {
retry.turnOffIfPackageRemoved(failed("NOT_INSTALLED"), null);

policyNotTurnedOff();
}

private void policyNotTurnedOff() {
verify(policyRepository, never()).disable(any(), any(), any());
}

private void nothingScheduled() {
verifyNoInteractions(deliveryDispatcher);
verify(policyRepository, never()).markRetryScheduled(any(), any());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,21 @@ void failedStatus_retryDecidedBeforeTheStateIsSaved() {
order.verify(deviceStateRepository).save(st);
}

@Test
void failedStatus_policyDecidedBeforeTheStateIsSaved() {
machineResolves();
PackageUpdateDeviceState st = state(SoftwareActionStatus.WAITING);
PackageUpdateRequest req = request(PackageUpdateSource.AUTO, PackageUpdateMode.WAIT, null);
when(deviceStateRepository.findByTenantIdAndRequestIdAndMachineId("t1", "req-1", "m1")).thenReturn(Optional.of(st));
when(requestRepository.findByTenantIdAndRequestId("t1", "req-1")).thenReturn(Optional.of(req));

processor.process("m1", failed("e2", PackageUpdateStatusMessage.Failed.Reason.NOT_INSTALLED));

InOrder order = inOrder(autoUpdateRetry, deviceStateRepository);
order.verify(autoUpdateRetry).turnOffIfPackageRemoved(st, req);
order.verify(deviceStateRepository).save(st);
}

@Test
void schedulingTheRetryFails_stateIsNotSaved_soTheRedeliveryDecidesAgain() {
machineResolves();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import org.springframework.data.mongodb.repository.Update;
import org.springframework.stereotype.Repository;

import java.time.Instant;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -34,4 +35,8 @@ Optional<PackageAutoUpdatePolicy> findByTenantIdAndMachineIdAndPackageManagerAnd
@Query("{ '_id': ?0 }")
@Update("{ '$set': { 'retryRequestId': ?1 } }")
long markRetryScheduled(String policyId, String requestId);

@Query("{ '_id': ?0 }")
@Update("{ '$set': { 'enabled': false, 'updatedBy': ?1, 'updatedAt': ?2 } }")
long disable(String policyId, String actor, Instant at);
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ public class PackageAutoUpdatePolicyService {
private final PackageAutoUpdatePolicyRepository repository;
private final PackageCatalogVersions catalogVersions;

// A run with Auto-update off leaves an earlier policy as it is: only Turn Off Auto-Update turns one off.
// A run with Auto-update off leaves an earlier policy as it is: Turn Off Auto-Update turns one off, and so does a
// device reporting that the package is gone.
// The run itself brings the package to the catalog's latest, so the trigger starts from the next release.
public void turnOn(RunningExecutionRows execution, BrewPackageType brewPackageType, String softwareId,
SoftwareAutoUpdate autoUpdate, PackageUpdateSettings updateSettings) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.springframework.data.mongodb.core.MongoTemplate;
import org.springframework.data.mongodb.core.query.Query;

import java.time.Instant;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
Expand Down Expand Up @@ -154,6 +155,24 @@ void retryRequestId_setByARetry_clearedByTheNextOrder() {
assertThat(ordered.getLastVersion()).isEqualTo("152.0.7977.65");
}

@Test
@DisplayName("disable: turns off that one policy, the device's other policies and the other devices stay on")
void disable_turnsOffOnePolicy() {
repository.enable(template("google-chrome", "title-7", WAIT, "admin@x"), List.of("m1", "m2"));
repository.enable(template("slack", "title-9", WAIT, "admin@x"), List.of("m1"));
String id = byMachine("google-chrome").get("m1").getId();
Instant at = Instant.parse("2026-11-02T10:00:00Z");

repository.disable(id, "package-auto-update", at);

PackageAutoUpdatePolicy off = byMachine("google-chrome").get("m1");
assertThat(off.isEnabled()).isFalse();
assertThat(off.getUpdatedBy()).isEqualTo("package-auto-update");
assertThat(off.getUpdatedAt()).isEqualTo(at);
assertThat(repository.findByTenantIdAndEnabledTrue(TENANT)).hasSize(2)
.noneMatch(policy -> policy.getId().equals(id));
}

private Map<String, PackageAutoUpdatePolicy> byMachine(String packageName) {
return repository.findAll().stream()
.filter(p -> packageName.equals(p.getPackageName()))
Expand Down
Loading