fix(api): Don't prevent subscribing with API_KEY when there is also an owner-based rule (#2828)

Author: mattcreaser

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

aws-api/src/main/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecorator.java

@@ -83,13 +83,19 @@ public <R> GraphQLRequest<R> decorate(
         AppSyncGraphQLRequest<R> appSyncRequest = (AppSyncGraphQLRequest<R>) request;
         AuthRule ownerRuleWithReadRestriction = null;
         Map<String, Set<String>> 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 <R> GraphQLRequest<R> 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<ModelOperation> 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);

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<PublicAndOwner> originalRequest = createRequest(PublicAndOwner.class, subscriptionType);
+            GraphQLRequest<PublicAndOwner> 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<PublicAndOwnerOidc> originalRequest =
+                createRequest(PublicAndOwnerOidc.class, subscriptionType);
+            GraphQLRequest<PublicAndOwnerOidc> 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<PublicAndOwner> originalRequest = createRequest(PublicAndOwner.class, subscriptionType);
+            GraphQLRequest<PublicAndOwner> 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<PublicAndOwnerOidc> originalRequest =
+                createRequest(PublicAndOwnerOidc.class, subscriptionType);
+            GraphQLRequest<PublicAndOwnerOidc> modifiedRequest = decorator.decorate(originalRequest, mode);
+            assertEquals(expectedOwner, getOwnerField(modifiedRequest));
+        }
+    }
+
     private <M extends Model> String getOwnerField(GraphQLRequest<M> 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 {}
 }