From b5c02af349ddad943157cfe459f4240cde118791 Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Fri, 15 Nov 2024 13:23:37 +0800 Subject: [PATCH 1/9] Fix #1771: Enforce presence of user ID before operation approval --- docs/WebServices-Methods.md | 46 +++++++++++++- .../powerauth/client/PowerAuthClient.java | 18 ++++++ .../model/request/OperationClaimRequest.java | 41 ++++++++++++ .../model/request/OperationDetailRequest.java | 4 -- .../OperationClaimRequestValidator.java | 46 ++++++++++++++ .../controller/api/OperationsController.java | 15 +++++ .../tasks/OperationServiceBehavior.java | 41 +++++++++++- .../fido2/PowerAuthAssertionProvider.java | 11 ++-- .../api/PowerAuthControllerTest.java | 1 - .../tasks/OperationServiceBehaviorTest.java | 63 ++----------------- .../rest/client/PowerAuthRestClient.java | 11 +++- 11 files changed, 225 insertions(+), 72 deletions(-) create mode 100644 powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java create mode 100644 powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java diff --git a/docs/WebServices-Methods.md b/docs/WebServices-Methods.md index ac57aefab..b26fab9cd 100644 --- a/docs/WebServices-Methods.md +++ b/docs/WebServices-Methods.md @@ -88,6 +88,7 @@ The following `v3` methods are published using the service: - Operations - [createOperation](#method-createoperation) - [operationDetail](#method-operationdetail) + - [operationClaim](#method-operationclaim) - [findPendingOperationsForUser](#method-findpendingoperationsforuser) - [findAllOperationsForUser](#method-findalloperationsforuser) - [findAllOperationsByExternalId](#method-findalloperationsbyexternalid) @@ -2232,10 +2233,49 @@ REST endpoint: `POST /rest/v3/operation/detail` `OperationDetailRequest` -| Type | Name | Description | -|------|------|-------------| +| Type | Name | Description | +|----------|---------------|---------------------------------| | `String` | `operationId` | The identifier of the operation | -| `String` | `userId` | Optional user identifier of the user, used for operation claim. | + +#### Response + +`OperationDetailResponse` + +| Type | Name | Description | +|-----------------------|----------------------|----------------------------------------------------------------------------------------------------------------------------------| +| `String` | `id` | The operation ID | +| `String` | `userId` | The identifier of the user | +| `String` | `applicationId` | The identifier of the application | +| `String` | `externalId` | External identifier of the operation, i.e., ID from transaction system | +| `String` | `operationType` | Type of the operation created based on the template | +| `String` | `data` | Operation data | +| `Map` | `parameters` | Parameters of the operation, will be filled to the operation data | +| `OperationStatus` | `status` | Status of the operation | +| `String` | `statusReason` | Optional details why the status changed. The value should be sent in the form of a computer-readable code, not a free-form text. | +| `List` | `signatureType` | Allowed types of signature | +| `Long` | `failureCount` | The current number of the failed approval attempts | +| `Long` | `maxFailureCount` | The maximum allowed number of the failed approval attempts | +| `Date` | `timestampCreated` | Timestamp of when the operation was created | +| `Date` | `timestampExpires` | Timestamp of when the operation will expires / expired | +| `Date` | `timestampFinalized` | Timestamp of when the operation was switched to a terminating status | +| `String` | `riskFlags` | Risk flags for offline QR code. Uppercase letters without separator, e.g. `XFC`. | +| `String` | `proximityOtp` | TOTP for proximity check (if enabled) valid for the current time step. | +| `String` | `activationId` | Activation Id of the activation scoped for the operation | + +### Method 'operationClaim' + +Claim the operation for a user. + +#### Request + +REST endpoint: `POST /rest/v3/operation/claim` + +`OperationClaimRequest` + +| Type | Name | Description | +|----------|---------------|--------------------------------------------------------| +| `String` | `operationId` | The identifier of the operation | +| `String` | `userId` | User identifier of the user, used for operation claim. | #### Response diff --git a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/PowerAuthClient.java b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/PowerAuthClient.java index dadc5e49e..e26c4facd 100644 --- a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/PowerAuthClient.java +++ b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/PowerAuthClient.java @@ -1976,6 +1976,24 @@ RecoveryCodeActivationResponse createActivationUsingRecoveryCode(String recovery */ OperationDetailResponse operationDetail(OperationDetailRequest request, MultiValueMap queryParams, MultiValueMap httpHeaders) throws PowerAuthClientException; + /** + * Claim operation for a user. + * @param request Operation detail request. + * @return Operation detail response. + * @throws PowerAuthClientException In case REST API call fails. + */ + OperationDetailResponse operationClaim(OperationClaimRequest request) throws PowerAuthClientException; + + /** + * Claim operation for a user. + * @param request Operation detail request. + * @param queryParams HTTP query parameters. + * @param httpHeaders HTTP headers. + * @return Operation detail response. + * @throws PowerAuthClientException In case REST API call fails. + */ + OperationDetailResponse operationClaim(OperationClaimRequest request, MultiValueMap queryParams, MultiValueMap httpHeaders) throws PowerAuthClientException; + /** * Get list with all operations for provided user. * @param request Get operation list request. diff --git a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java new file mode 100644 index 000000000..b7a910391 --- /dev/null +++ b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java @@ -0,0 +1,41 @@ +/* + * PowerAuth Server and related software components + * Copyright (C) 2024 Wultra s.r.o. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package com.wultra.security.powerauth.client.model.request; + +import lombok.Data; + +/** + * Request for operation claim. + * + * @author Roman Strobl, roman.strobl@wultra.com + */ +@Data +public class OperationClaimRequest { + + /** + * Operation identifier. + */ + private String operationId; + + /** + * Optional user identifier of the user who is claiming the operation. + */ + private String userId; + +} diff --git a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationDetailRequest.java b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationDetailRequest.java index 8499ab0d4..a59fffd2c 100644 --- a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationDetailRequest.java +++ b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationDetailRequest.java @@ -30,8 +30,4 @@ public class OperationDetailRequest { private String operationId; - /** - * Optional user identifier of the user who is requesting the operation. - */ - private String userId; } diff --git a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java new file mode 100644 index 000000000..9fb7e7031 --- /dev/null +++ b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java @@ -0,0 +1,46 @@ +/* + * PowerAuth Server and related software components + * Copyright (C) 2024 Wultra s.r.o. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package com.wultra.security.powerauth.client.model.validator; + +import com.wultra.security.powerauth.client.model.request.OperationClaimRequest; + +/** + * Validator for OperationClaimRequest class. + * + * @author Roman Strobl, roman.strobl@wultra.com + */ +public class OperationClaimRequestValidator { + + public static String validate(OperationClaimRequest source) { + if (source == null) { + return "Operation claim request must not be null"; + } + if (source.getOperationId() == null) { + return "Operation ID must not be null when requesting operation claim"; + } + if (source.getOperationId().isEmpty()) { + return "Operation ID must not be empty when requesting operation claim"; + } + if (source.getUserId() == null || source.getUserId().isEmpty()) { + return "User ID must be specified when requesting operation claim"; + } + return null; + } + +} diff --git a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/controller/api/OperationsController.java b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/controller/api/OperationsController.java index 646d273f7..25cfe9a7a 100644 --- a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/controller/api/OperationsController.java +++ b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/controller/api/OperationsController.java @@ -81,6 +81,21 @@ public ObjectResponse operationDetail(@RequestBody Obje return response; } + /** + * Claim operation for a user. + * + * @param request Claim operation for a user. + * @return Get operation response. + * @throws Exception In case the service throws exception. + */ + @PostMapping("/claim") + public ObjectResponse operationClaim(@RequestBody ObjectRequest request) throws Exception { + logger.info("OperationClaimRequest received: {}", request); + final ObjectResponse response = new ObjectResponse<>(service.operationClaim(request.getRequestObject())); + logger.info("OperationClaimRequest succeeded: {}", response); + return response; + } + /** * Find all operations for given user. * diff --git a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java index 08506b256..2ac63646d 100644 --- a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java +++ b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java @@ -369,13 +369,18 @@ public OperationUserActionResponse attemptApproveOperation(OperationApproveReque throw localizationProvider.buildExceptionForCode(ServiceError.OPERATION_APPROVE_FAILURE); } + final String expectedUserId = operationEntity.getUserId(); + if (expectedUserId == null) { + logger.warn("Operation cannot be approved, because user ID is not set: {}.", operationId); + throw localizationProvider.buildExceptionForCode(ServiceError.OPERATION_APPROVE_FAILURE); + } + // Check the operation properties match the request final PowerAuthSignatureTypes factorEnum = PowerAuthSignatureTypes.getEnumFromString(signatureType.toString()); final ProximityCheckResult proximityCheckResult = fetchProximityCheckResult(operationEntity, request, currentInstant); - final String expectedUserId = operationEntity.getUserId(); final boolean activationIdMatches = activationIdMatches(request, operationEntity.getActivationId()); final boolean operationShouldFail = operationApprovalCustomizer.operationShouldFail(operationEntity, request); - if ((expectedUserId == null || expectedUserId.equals(userId)) // correct user approved the operation + if (expectedUserId.equals(userId) // correct user approved the operation && operationEntity.getApplications().contains(application.get()) // operation is approved by the expected application && isDataEqual(operationEntity, data) // operation data matched the expected value && factorsAcceptable(operationEntity, factorEnum) // auth factors are acceptable @@ -763,6 +768,38 @@ public OperationDetailResponse operationDetail(OperationDetailRequest request) t return localizationProvider.buildExceptionForCode(ServiceError.OPERATION_NOT_FOUND); }); + final OperationEntity operationEntity = expireOperation(operation, currentTimestamp); + final OperationDetailResponse operationDetailResponse = convertFromEntityAndFillOtp(operationEntity); + extendAndSetOperationDetailData(operationDetailResponse); + return operationDetailResponse; + } catch (GenericServiceException ex) { + // already logged + throw ex; + } catch (RuntimeException ex) { + logger.error("Runtime exception or error occurred, transaction will be rolled back", ex); + throw ex; + } catch (Exception ex) { + logger.error("Unknown error occurred", ex); + throw new GenericServiceException(ServiceError.UNKNOWN_ERROR, ex.getMessage()); + } + } + + @Transactional // operation is modified when expiration happens + public OperationDetailResponse operationClaim(OperationClaimRequest request) throws GenericServiceException { + try { + final String error = OperationClaimRequestValidator.validate(request); + if (error != null) { + throw new GenericServiceException(ServiceError.INVALID_REQUEST, error); + } + + final Date currentTimestamp = new Date(); + final String operationId = request.getOperationId(); + + final OperationEntity operation = operationQueryService.findOperationForUpdate(operationId).orElseThrow(() -> { + logger.warn("Operation was not found for ID: {}", operationId); + return localizationProvider.buildExceptionForCode(ServiceError.OPERATION_NOT_FOUND); + }); + final String userId = request.getUserId(); final OperationEntity operationEntity = expireOperation( claimOperation(operation, userId, currentTimestamp), diff --git a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/fido2/PowerAuthAssertionProvider.java b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/fido2/PowerAuthAssertionProvider.java index 7661b5eaa..017146bef 100644 --- a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/fido2/PowerAuthAssertionProvider.java +++ b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/fido2/PowerAuthAssertionProvider.java @@ -31,10 +31,7 @@ import com.wultra.security.powerauth.client.model.enumeration.OperationStatus; import com.wultra.security.powerauth.client.model.enumeration.SignatureType; import com.wultra.security.powerauth.client.model.enumeration.UserActionResult; -import com.wultra.security.powerauth.client.model.request.OperationApproveRequest; -import com.wultra.security.powerauth.client.model.request.OperationCreateRequest; -import com.wultra.security.powerauth.client.model.request.OperationDetailRequest; -import com.wultra.security.powerauth.client.model.request.OperationFailApprovalRequest; +import com.wultra.security.powerauth.client.model.request.*; import com.wultra.security.powerauth.client.model.response.OperationDetailResponse; import com.wultra.security.powerauth.client.model.response.OperationUserActionResponse; import com.wultra.security.powerauth.fido2.model.entity.AuthenticatorDetail; @@ -133,6 +130,12 @@ public AssertionChallenge approveAssertion(String challengeValue, AuthenticatorD final String operationId = split[0]; final String operationData = split[1]; + // Claim operation to set user ID for this operation before approval + final OperationClaimRequest claimRequest = new OperationClaimRequest(); + claimRequest.setOperationId(operationId); + claimRequest.setUserId(authenticatorDetail.getUserId()); + operationServiceBehavior.operationClaim(claimRequest); + final OperationApproveRequest operationApproveRequest = new OperationApproveRequest(); operationApproveRequest.setOperationId(operationId); operationApproveRequest.setData(operationData); diff --git a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/controller/api/PowerAuthControllerTest.java b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/controller/api/PowerAuthControllerTest.java index 13791d7df..ad9ec6b93 100644 --- a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/controller/api/PowerAuthControllerTest.java +++ b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/controller/api/PowerAuthControllerTest.java @@ -454,7 +454,6 @@ void testGetOperationDetail() throws Exception { final OperationDetailRequest detailRequest = new OperationDetailRequest(); final String operationId = operation.getId(); detailRequest.setOperationId(operationId); - detailRequest.setUserId(PowerAuthControllerTestConfig.USER_ID); final OperationDetailResponse detailResponse = powerAuthClient.operationDetail(detailRequest); assertEquals(OperationStatus.PENDING, detailResponse.getStatus()); diff --git a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java index 98425b76d..e8731edd6 100644 --- a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java +++ b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java @@ -610,11 +610,11 @@ void testOperationClaim() throws Exception { final String operationId = createLoginOperation(); final String userId = "user_" + UUID.randomUUID(); - final OperationDetailRequest detailRequest = new OperationDetailRequest(); - detailRequest.setOperationId(operationId); - detailRequest.setUserId(userId); + final OperationClaimRequest claimRequest = new OperationClaimRequest(); + claimRequest.setOperationId(operationId); + claimRequest.setUserId(userId); // Check operation claim - assertEquals(userId, operationService.operationDetail(detailRequest).getUserId()); + assertEquals(userId, operationService.operationClaim(claimRequest).getUserId()); } @Test @@ -700,7 +700,7 @@ void testParsingDeviceOperationCancelDetail() throws Exception { } @Test - void testAnonymousOperationApprovedUserChanged() throws GenericServiceException { + void testAnonymousOperationApprovalNotAllowed() throws GenericServiceException { final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); operationCreateRequest.setApplications(List.of("PA_Tests")); operationCreateRequest.setTemplateName("test-template"); @@ -711,58 +711,7 @@ void testAnonymousOperationApprovedUserChanged() throws GenericServiceException approveRequest.setData("A2"); approveRequest.setApplicationId("PA_Tests"); approveRequest.setSignatureType(SignatureType.POSSESSION_KNOWLEDGE); - final OperationUserActionResponse response = operationService.attemptApproveOperation(approveRequest); - assertEquals(UserActionResult.APPROVED, response.getResult()); - final OperationDetailRequest detailRequest = new OperationDetailRequest(); - detailRequest.setOperationId(operation.getId()); - final OperationDetailResponse operationDetail = operationService.operationDetail(detailRequest); - assertEquals("test_user", operationDetail.getUserId()); - } - - @Test - void testAnonymousOperationFailedApproveUserNotChanged() throws GenericServiceException { - final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); - operationCreateRequest.setApplications(List.of("PA_Tests")); - operationCreateRequest.setTemplateName("test-template"); - final OperationDetailResponse operation = operationService.createOperation(operationCreateRequest); - final OperationApproveRequest approveRequest = new OperationApproveRequest(); - approveRequest.setOperationId(operation.getId()); - approveRequest.setUserId("invalid_user"); - approveRequest.setData("invalid_data"); - approveRequest.setApplicationId("PA_Tests"); - approveRequest.setSignatureType(SignatureType.POSSESSION_KNOWLEDGE); - final OperationUserActionResponse response = operationService.attemptApproveOperation(approveRequest); - assertEquals(UserActionResult.APPROVAL_FAILED, response.getResult()); - final OperationDetailRequest detailRequest = new OperationDetailRequest(); - detailRequest.setOperationId(operation.getId()); - final OperationDetailResponse operationDetail = operationService.operationDetail(detailRequest); - assertNull(operationDetail.getUserId()); - } - - @Test - void testAnonymousOperationFailedOperationUserNotChanged() throws GenericServiceException { - final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); - operationCreateRequest.setApplications(List.of("PA_Tests")); - operationCreateRequest.setTemplateName("test-template"); - final OperationDetailResponse operation = operationService.createOperation(operationCreateRequest); - for (int i = 0; i < 5; i++) { - final OperationApproveRequest approveRequest = new OperationApproveRequest(); - approveRequest.setOperationId(operation.getId()); - approveRequest.setUserId("invalid_user"); - approveRequest.setData("invalid_data"); - approveRequest.setApplicationId("PA_Tests"); - approveRequest.setSignatureType(SignatureType.POSSESSION_KNOWLEDGE); - final OperationUserActionResponse response = operationService.attemptApproveOperation(approveRequest); - if (i == 4) { - assertEquals(UserActionResult.OPERATION_FAILED, response.getResult()); - } else { - assertEquals(UserActionResult.APPROVAL_FAILED, response.getResult()); - } - } - final OperationDetailRequest detailRequest = new OperationDetailRequest(); - detailRequest.setOperationId(operation.getId()); - final OperationDetailResponse operationDetail = operationService.operationDetail(detailRequest); - assertNull(operationDetail.getUserId()); + assertThrows(GenericServiceException.class, () -> operationService.attemptApproveOperation(approveRequest)); } @Test diff --git a/powerauth-rest-client-spring/src/main/java/com/wultra/security/powerauth/rest/client/PowerAuthRestClient.java b/powerauth-rest-client-spring/src/main/java/com/wultra/security/powerauth/rest/client/PowerAuthRestClient.java index 9a70fbc4d..c6879029c 100644 --- a/powerauth-rest-client-spring/src/main/java/com/wultra/security/powerauth/rest/client/PowerAuthRestClient.java +++ b/powerauth-rest-client-spring/src/main/java/com/wultra/security/powerauth/rest/client/PowerAuthRestClient.java @@ -42,7 +42,6 @@ import org.springframework.http.HttpStatus; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; -import org.springframework.util.StringUtils; import java.io.IOException; import java.time.Duration; @@ -1388,6 +1387,16 @@ public OperationDetailResponse operationDetail(OperationDetailRequest request, M return callV3RestApi("/operation/detail", request, queryParams, httpHeaders, OperationDetailResponse.class); } + @Override + public OperationDetailResponse operationClaim(OperationClaimRequest request) throws PowerAuthClientException { + return operationClaim(request, EMPTY_MULTI_MAP, EMPTY_MULTI_MAP); + } + + @Override + public OperationDetailResponse operationClaim(OperationClaimRequest request, MultiValueMap queryParams, MultiValueMap httpHeaders) throws PowerAuthClientException { + return callV3RestApi("/operation/claim", request, queryParams, httpHeaders, OperationDetailResponse.class); + } + @Override public OperationListResponse operationList(OperationListForUserRequest request) throws PowerAuthClientException { return operationList(request, EMPTY_MULTI_MAP, EMPTY_MULTI_MAP); From 0128dbe5cd18dc9aed05d741b37905429c51a371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roman=20=C5=A0trobl?= Date: Fri, 15 Nov 2024 14:15:19 +0800 Subject: [PATCH 2/9] Update eror message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Luboš Račanský --- .../server/service/behavior/tasks/OperationServiceBehavior.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java index 2ac63646d..9dccd3372 100644 --- a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java +++ b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java @@ -371,7 +371,7 @@ public OperationUserActionResponse attemptApproveOperation(OperationApproveReque final String expectedUserId = operationEntity.getUserId(); if (expectedUserId == null) { - logger.warn("Operation cannot be approved, because user ID is not set: {}.", operationId); + logger.warn("Operation ID: {} cannot be approved, because user ID is not set.", operationId); throw localizationProvider.buildExceptionForCode(ServiceError.OPERATION_APPROVE_FAILURE); } From 542a19fb0d40e347a67e81aef3e836dc58777ff5 Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Fri, 15 Nov 2024 14:17:22 +0800 Subject: [PATCH 3/9] Fix comment --- .../powerauth/client/model/request/OperationClaimRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java index b7a910391..025723c1e 100644 --- a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java +++ b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/request/OperationClaimRequest.java @@ -34,7 +34,7 @@ public class OperationClaimRequest { private String operationId; /** - * Optional user identifier of the user who is claiming the operation. + * User identifier of the user who is claiming the operation. */ private String userId; From 1cf7657e9e5ce6bda0ca144e7afc31b0b1c793ea Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Mon, 18 Nov 2024 13:44:17 +0800 Subject: [PATCH 4/9] Add test for failed claim in case user ID changes --- .../tasks/OperationServiceBehaviorTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java index e8731edd6..13670b646 100644 --- a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java +++ b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java @@ -617,6 +617,25 @@ void testOperationClaim() throws Exception { assertEquals(userId, operationService.operationClaim(claimRequest).getUserId()); } + @Test + void testOperationClaimFailDifferentUserId() throws Exception { + final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); + operationCreateRequest.setApplications(Collections.singletonList(APP_ID)); + operationCreateRequest.setTemplateName(TEMPLATE_NAME); + operationCreateRequest.setTimestampExpires(new Date(Instant.now() + .plusSeconds(TimeUnit.MINUTES.toSeconds(60)).toEpochMilli())); + operationCreateRequest.setUserId("test_user_id"); + final String operationId = operationService.createOperation(operationCreateRequest).getId(); + final String userId = "user_" + UUID.randomUUID(); + final OperationClaimRequest claimRequest = new OperationClaimRequest(); + claimRequest.setOperationId(operationId); + claimRequest.setUserId(userId); + final Exception exception = assertThrows(GenericServiceException.class, () -> { + operationService.operationClaim(claimRequest); + }); + assertEquals("Operation was not found.", exception.getMessage()); + } + @Test void testOperationApproveWithValidProximityOtp() throws Exception { final OperationDetailResponse operation = createOperation(true); From 71f9f208fcaa75f1c0954ba88206e05321901833 Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Mon, 18 Nov 2024 15:50:46 +0800 Subject: [PATCH 5/9] Add test for failed approval in case user ID changes --- .../tasks/OperationServiceBehaviorTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java index 13670b646..8d562ab1e 100644 --- a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java +++ b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java @@ -733,6 +733,23 @@ void testAnonymousOperationApprovalNotAllowed() throws GenericServiceException { assertThrows(GenericServiceException.class, () -> operationService.attemptApproveOperation(approveRequest)); } + @Test + void testOperationApprovalFailDifferentUserId() throws GenericServiceException { + final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); + operationCreateRequest.setApplications(List.of("PA_Tests")); + operationCreateRequest.setTemplateName("test-template"); + operationCreateRequest.setUserId("test_user_1"); + final OperationDetailResponse operation = operationService.createOperation(operationCreateRequest); + final OperationApproveRequest approveRequest = new OperationApproveRequest(); + approveRequest.setOperationId(operation.getId()); + approveRequest.setUserId("test_user_2"); + approveRequest.setData("A2"); + approveRequest.setApplicationId("PA_Tests"); + approveRequest.setSignatureType(SignatureType.POSSESSION_KNOWLEDGE); + final OperationUserActionResponse response = operationService.attemptApproveOperation(approveRequest); + assertEquals(UserActionResult.APPROVAL_FAILED, response.getResult()); + } + @Test void testAnonymousOperationRejectUserChanged() throws GenericServiceException { final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); From f79fa57f0f0fa24d7f72e1b95d2927acea6fa455 Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Tue, 19 Nov 2024 12:44:00 +0800 Subject: [PATCH 6/9] Unify error handling --- .../model/validator/OperationClaimRequestValidator.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java index 9fb7e7031..a56a26812 100644 --- a/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java +++ b/powerauth-client-model/src/main/java/com/wultra/security/powerauth/client/model/validator/OperationClaimRequestValidator.java @@ -31,11 +31,8 @@ public static String validate(OperationClaimRequest source) { if (source == null) { return "Operation claim request must not be null"; } - if (source.getOperationId() == null) { - return "Operation ID must not be null when requesting operation claim"; - } - if (source.getOperationId().isEmpty()) { - return "Operation ID must not be empty when requesting operation claim"; + if (source.getOperationId() == null || source.getOperationId().isEmpty()) { + return "Operation ID must be specified when requesting operation claim"; } if (source.getUserId() == null || source.getUserId().isEmpty()) { return "User ID must be specified when requesting operation claim"; From 4c086bd8d5f5bf4d1b0066793d23c48195d21ba4 Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Tue, 19 Nov 2024 12:55:15 +0800 Subject: [PATCH 7/9] Use similar logic for anonymous operations during reject as during approval --- .../tasks/OperationServiceBehavior.java | 9 ++++++--- .../tasks/OperationServiceBehaviorTest.java | 20 ++++++------------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java index 9dccd3372..f5d00e337 100644 --- a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java +++ b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java @@ -390,7 +390,6 @@ && proximityCheckPassed(proximityCheckResult) && !operationShouldFail) { // operation customizer can change the approval status by an external impulse // Approve the operation - operationEntity.setUserId(userId); operationEntity.setStatus(OperationStatusDo.APPROVED); operationEntity.setTimestampFinalized(currentTimestamp); operationEntity.setAdditionalData(mapMerge(operationEntity.getAdditionalData(), additionalData)); @@ -539,11 +538,15 @@ public OperationUserActionResponse rejectOperation(OperationRejectRequest reques } final String expectedUserId = operationEntity.getUserId(); - if ((expectedUserId == null || expectedUserId.equals(userId)) // correct user rejects the operation + if (expectedUserId == null) { + logger.warn("Operation ID: {} cannot be rejected, because user ID is not set.", operationId); + throw localizationProvider.buildExceptionForCode(ServiceError.OPERATION_APPROVE_FAILURE); + } + + if ((expectedUserId.equals(userId)) // correct user rejects the operation && operationEntity.getApplications().contains(application.get())) { // operation is rejected by the expected application // Reject the operation - operationEntity.setUserId(userId); operationEntity.setStatus(OperationStatusDo.REJECTED); operationEntity.setTimestampFinalized(currentTimestamp); operationEntity.setAdditionalData(mapMerge(operationEntity.getAdditionalData(), additionalData)); diff --git a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java index 8d562ab1e..b5142b962 100644 --- a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java +++ b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java @@ -751,7 +751,7 @@ void testOperationApprovalFailDifferentUserId() throws GenericServiceException { } @Test - void testAnonymousOperationRejectUserChanged() throws GenericServiceException { + void testAnonymousOperationRejectError() throws GenericServiceException { final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); operationCreateRequest.setApplications(List.of("PA_Tests")); operationCreateRequest.setTemplateName("test-template"); @@ -760,30 +760,22 @@ void testAnonymousOperationRejectUserChanged() throws GenericServiceException { rejectRequest.setOperationId(operation.getId()); rejectRequest.setUserId("test_user"); rejectRequest.setApplicationId("PA_Tests"); - final OperationUserActionResponse response = operationService.rejectOperation(rejectRequest); - assertEquals(UserActionResult.REJECTED, response.getResult()); - final OperationDetailRequest detailRequest = new OperationDetailRequest(); - detailRequest.setOperationId(operation.getId()); - final OperationDetailResponse operationDetail = operationService.operationDetail(detailRequest); - assertEquals("test_user", operationDetail.getUserId()); + assertThrows(GenericServiceException.class, () -> operationService.rejectOperation(rejectRequest)); } @Test - void testAnonymousOperationRejectFailedUserNotChanged() throws GenericServiceException { + void testOperationRejectFailedDifferentUserId() throws GenericServiceException { final OperationCreateRequest operationCreateRequest = new OperationCreateRequest(); operationCreateRequest.setApplications(List.of("PA_Tests")); operationCreateRequest.setTemplateName("test-template"); + operationCreateRequest.setUserId("test_user_1"); final OperationDetailResponse operation = operationService.createOperation(operationCreateRequest); final OperationRejectRequest rejectRequest = new OperationRejectRequest(); rejectRequest.setOperationId(operation.getId()); - rejectRequest.setUserId("test_user"); - rejectRequest.setApplicationId(APP_ID); + rejectRequest.setUserId("test_user_2"); + rejectRequest.setApplicationId("PA_Tests"); final OperationUserActionResponse response = operationService.rejectOperation(rejectRequest); assertEquals(UserActionResult.REJECT_FAILED, response.getResult()); - final OperationDetailRequest detailRequest = new OperationDetailRequest(); - detailRequest.setOperationId(operation.getId()); - final OperationDetailResponse operationDetail = operationService.operationDetail(detailRequest); - assertNull(operationDetail.getUserId()); } private void createApplication() throws GenericServiceException { From b0cd8109aeca2450b80143ce07389d3f9c256ca1 Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Tue, 19 Nov 2024 12:59:58 +0800 Subject: [PATCH 8/9] Improve exception checks --- .../behavior/tasks/OperationServiceBehaviorTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java index b5142b962..4f6c3e122 100644 --- a/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java +++ b/powerauth-java-server/src/test/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehaviorTest.java @@ -730,7 +730,8 @@ void testAnonymousOperationApprovalNotAllowed() throws GenericServiceException { approveRequest.setData("A2"); approveRequest.setApplicationId("PA_Tests"); approveRequest.setSignatureType(SignatureType.POSSESSION_KNOWLEDGE); - assertThrows(GenericServiceException.class, () -> operationService.attemptApproveOperation(approveRequest)); + final Exception exception = assertThrows(GenericServiceException.class, () -> operationService.attemptApproveOperation(approveRequest)); + assertEquals("Operation cannot be approved.", exception.getMessage()); } @Test @@ -760,7 +761,8 @@ void testAnonymousOperationRejectError() throws GenericServiceException { rejectRequest.setOperationId(operation.getId()); rejectRequest.setUserId("test_user"); rejectRequest.setApplicationId("PA_Tests"); - assertThrows(GenericServiceException.class, () -> operationService.rejectOperation(rejectRequest)); + final Exception exception = assertThrows(GenericServiceException.class, () -> operationService.rejectOperation(rejectRequest)); + assertEquals("Operation cannot be approved.", exception.getMessage()); } @Test From fefd75df3f4306b57bc36c987a239c168542ae99 Mon Sep 17 00:00:00 2001 From: Roman Strobl Date: Tue, 19 Nov 2024 13:05:12 +0800 Subject: [PATCH 9/9] Improve code readability --- .../service/behavior/tasks/OperationServiceBehavior.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java index f5d00e337..0c78af0fe 100644 --- a/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java +++ b/powerauth-java-server/src/main/java/io/getlime/security/powerauth/app/server/service/behavior/tasks/OperationServiceBehavior.java @@ -804,11 +804,9 @@ public OperationDetailResponse operationClaim(OperationClaimRequest request) thr }); final String userId = request.getUserId(); - final OperationEntity operationEntity = expireOperation( - claimOperation(operation, userId, currentTimestamp), - currentTimestamp - ); - final OperationDetailResponse operationDetailResponse = convertFromEntityAndFillOtp(operationEntity); + final OperationEntity operationClaimed = claimOperation(operation, userId, currentTimestamp); + final OperationEntity operationUpdated = expireOperation(operationClaimed, currentTimestamp); + final OperationDetailResponse operationDetailResponse = convertFromEntityAndFillOtp(operationUpdated); extendAndSetOperationDetailData(operationDetailResponse); return operationDetailResponse; } catch (GenericServiceException ex) {