From 68c40e19c57253067bb8196de2ac73b2f0f0f421 Mon Sep 17 00:00:00 2001 From: Matt Creaser Date: Wed, 10 Jul 2024 11:06:46 -0300 Subject: [PATCH] fix(api): Don't prevent subscribing with API_KEY when there is also an owner-based rule (#2828) --- .../aws/MultiAuthSubscriptionOperation.java | 11 +-- .../aws/auth/AuthRuleRequestDecorator.java | 21 +++++- .../auth/AuthRuleRequestDecoratorTest.java | 72 +++++++++++++++++++ 3 files changed, 98 insertions(+), 6 deletions(-) diff --git a/aws-api/src/main/java/com/amplifyframework/api/aws/MultiAuthSubscriptionOperation.java b/aws-api/src/main/java/com/amplifyframework/api/aws/MultiAuthSubscriptionOperation.java index 9ee37b4625..a904fb139a 100644 --- a/aws-api/src/main/java/com/amplifyframework/api/aws/MultiAuthSubscriptionOperation.java +++ b/aws-api/src/main/java/com/amplifyframework/api/aws/MultiAuthSubscriptionOperation.java @@ -80,7 +80,7 @@ public synchronized void start() { )); return; } - subscriptionFuture = executorService.submit(this::dispatchRequest); + queueDispatchRequest(); } private void dispatchRequest() { @@ -101,7 +101,7 @@ private void dispatchRequest() { // For ApiAuthExceptions, just queue up a dispatchRequest call. If there are no // other auth types left, it will emit the error to the client's callback // because authTypes.hasNext() will be false. - subscriptionFuture = executorService.submit(this::dispatchRequest); + queueDispatchRequest(); return; } catch (ApiException apiException) { LOG.warn("Unable to automatically add an owner to the request.", apiException); @@ -119,7 +119,7 @@ private void dispatchRequest() { response -> { if (response.hasErrors() && hasAuthRelatedErrors(response) && authTypes.hasNext()) { // If there are auth-related errors queue up a retry with the next authType - executorService.submit(this::dispatchRequest); + queueDispatchRequest(); } else { // Otherwise, we just want to dispatch it as a next item and // let callers deal with the errors. @@ -129,7 +129,7 @@ private void dispatchRequest() { apiException -> { LOG.warn("A subscription error occurred.", apiException); if (apiException instanceof ApiAuthException && authTypes.hasNext()) { - executorService.submit(this::dispatchRequest); + queueDispatchRequest(); } else { emitErrorAndCancelSubscription(apiException); } @@ -142,7 +142,10 @@ private void dispatchRequest() { "Check your application logs for detail." )); } + } + private void queueDispatchRequest() { + subscriptionFuture = executorService.submit(this::dispatchRequest); } @Override diff --git a/aws-api/src/main/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecorator.java b/aws-api/src/main/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecorator.java index fe8a0ced27..591c873d90 100644 --- a/aws-api/src/main/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecorator.java +++ b/aws-api/src/main/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecorator.java @@ -83,13 +83,19 @@ public GraphQLRequest decorate( AppSyncGraphQLRequest appSyncRequest = (AppSyncGraphQLRequest) request; AuthRule ownerRuleWithReadRestriction = null; Map> readAuthorizedGroupsMap = new HashMap<>(); + boolean publicSubscribeAllowed = false; // Note that we are intentionally supporting only one owner rule with a READ operation at this time. // If there is more than one, the operation will fail because AppSync generates a parameter for each // one. The question then is which one do we pass. JavaScript currently doesn't support this use case // and it's not clear what a good solution would be until AppSync supports real time filters. for (AuthRule authRule : appSyncRequest.getModelSchema().getAuthRules()) { - if (isReadRestrictingOwner(authRule)) { + if (doesRuleAllowPublicSubscribe(authRule, authType)) { + // This rule allows subscribing with the current authMode without adding the owner field, so there + // is no need to continue checking the other rules. + publicSubscribeAllowed = true; + break; + } else if (isReadRestrictingOwner(authRule)) { if (ownerRuleWithReadRestriction == null) { ownerRuleWithReadRestriction = authRule; } else { @@ -114,7 +120,8 @@ public GraphQLRequest decorate( // We only add the owner parameter to the subscription if there is an owner rule with a READ restriction // and either there are no group auth rules with read access or there are but the user isn't in any of // them. - if (ownerRuleWithReadRestriction != null + if (!publicSubscribeAllowed && + ownerRuleWithReadRestriction != null && userNotInReadRestrictingGroups(readAuthorizedGroupsMap, authType)) { String idClaim = ownerRuleWithReadRestriction.getIdentityClaimOrDefault(); String key = ownerRuleWithReadRestriction.getOwnerFieldOrDefault(); @@ -135,6 +142,16 @@ && userNotInReadRestrictingGroups(readAuthorizedGroupsMap, authType)) { return request; } + private boolean doesRuleAllowPublicSubscribe(AuthRule authRule, AuthorizationType authMode) { + AuthorizationType typeForRule = AuthorizationType.from(authRule.getAuthProvider()); + AuthStrategy strategy = authRule.getAuthStrategy(); + List operations = authRule.getOperationsOrDefault(); + return strategy == AuthStrategy.PUBLIC + && typeForRule == AuthorizationType.API_KEY + && authMode == AuthorizationType.API_KEY + && (operations.contains(ModelOperation.LISTEN) || operations.contains(ModelOperation.READ)); + } + private boolean isReadRestrictingOwner(AuthRule authRule) { return AuthStrategy.OWNER.equals(authRule.getAuthStrategy()) && authRule.getOperationsOrDefault().contains(ModelOperation.READ); diff --git a/aws-api/src/test/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecoratorTest.java b/aws-api/src/test/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecoratorTest.java index 0ad91a73c1..8906b2dd19 100644 --- a/aws-api/src/test/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecoratorTest.java +++ b/aws-api/src/test/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecoratorTest.java @@ -293,6 +293,66 @@ public void ownerArgumentNotAddedIfOwnerIsInCustomGroup() throws AmplifyExceptio } } + /** + * Verify owner argument is NOT added if model contains both public key and owner-based authorization and the + * requested auth type is API_KEY. + * @throws AmplifyException if a ModelSchema can't be derived from the Model class. + */ + @Test + public void doesNotAddOwnerWhenMultiAuthWithPublicKey() throws AmplifyException { + final AuthorizationType mode = AuthorizationType.API_KEY; + + // PublicAndOwner combines public and owner-based auth + for (SubscriptionType subscriptionType : SubscriptionType.values()) { + GraphQLRequest originalRequest = createRequest(PublicAndOwner.class, subscriptionType); + GraphQLRequest modifiedRequest = decorator.decorate(originalRequest, mode); + assertNull(getOwnerField(modifiedRequest)); + } + + // PublicAndOwnerOidc combines public and owner-based auth with an OIDC claim + for (SubscriptionType subscriptionType : SubscriptionType.values()) { + GraphQLRequest originalRequest = + createRequest(PublicAndOwnerOidc.class, subscriptionType); + GraphQLRequest modifiedRequest = decorator.decorate(originalRequest, mode); + assertNull(getOwnerField(modifiedRequest)); + } + } + + /** + * Verify owner argument is added if model contains both owner-based and public-key + * authorization and the auth mode is cognito. + * @throws AmplifyException if a ModelSchema can't be derived from the Model class. + */ + @Test + public void addsOwnerWhenMultiAuthWithCognito() throws AmplifyException { + final AuthorizationType mode = AuthorizationType.AMAZON_COGNITO_USER_POOLS; + final String expectedOwner = FakeCognitoAuthProvider.USERNAME; + + for (SubscriptionType subscriptionType : SubscriptionType.values()) { + GraphQLRequest originalRequest = createRequest(PublicAndOwner.class, subscriptionType); + GraphQLRequest modifiedRequest = decorator.decorate(originalRequest, mode); + assertEquals(expectedOwner, getOwnerField(modifiedRequest)); + } + } + + /** + * Verify owner argument is added if model contains both owner-based and public-key + * authorization and the auth mode is oidc. + * @throws AmplifyException if a ModelSchema can't be derived from the Model class. + */ + @Test + public void addsOwnerWhenMultiAuthWithOidc() throws AmplifyException { + final AuthorizationType mode = AuthorizationType.OPENID_CONNECT; + final String expectedOwner = FakeOidcAuthProvider.SUB; + + for (SubscriptionType subscriptionType : SubscriptionType.values()) { + GraphQLRequest originalRequest = + createRequest(PublicAndOwnerOidc.class, subscriptionType); + GraphQLRequest modifiedRequest = decorator.decorate(originalRequest, mode); + assertEquals(expectedOwner, getOwnerField(modifiedRequest)); + } + } + private String getOwnerField(GraphQLRequest request) { if (request.getVariables().containsKey("owner")) { return (String) request.getVariables().get("owner"); @@ -412,4 +472,16 @@ private abstract static class OwnerInCustomGroup implements Model {} ) }) private abstract static class OwnerNotInCustomGroup implements Model {} + + @ModelConfig(authRules = { + @AuthRule(allow = AuthStrategy.PUBLIC, operations = ModelOperation.READ), + @AuthRule(allow = AuthStrategy.OWNER) + }) + private abstract static class PublicAndOwner implements Model {} + + @ModelConfig(authRules = { + @AuthRule(allow = AuthStrategy.PUBLIC, operations = ModelOperation.READ), + @AuthRule(allow = AuthStrategy.OWNER, identityClaim = "sub") + }) + private abstract static class PublicAndOwnerOidc implements Model {} }