From 9d9703dd03eb6bc7ca317c5703aef56d3835f8bd Mon Sep 17 00:00:00 2001 From: Matt Creaser Date: Tue, 14 May 2024 09:33:50 -0300 Subject: [PATCH] Don't try to subscribe with owner rule when using a different authType --- .../aws/auth/AuthRuleRequestDecorator.java | 19 ++++- .../auth/AuthRuleRequestDecoratorTest.java | 72 +++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) 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..2d7e8304e2 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,16 @@ public GraphQLRequest decorate( AppSyncGraphQLRequest appSyncRequest = (AppSyncGraphQLRequest) request; AuthRule ownerRuleWithReadRestriction = null; Map> readAuthorizedGroupsMap = new HashMap<>(); + boolean subscribeAllowedForNonOwner = 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 (doesRuleAllowNonOwnerSubscribe(authRule, authType)) { + subscribeAllowedForNonOwner = true; + } else if (isReadRestrictingOwner(authRule)) { if (ownerRuleWithReadRestriction == null) { ownerRuleWithReadRestriction = authRule; } else { @@ -114,7 +117,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 (!subscribeAllowedForNonOwner && + ownerRuleWithReadRestriction != null && userNotInReadRestrictingGroups(readAuthorizedGroupsMap, authType)) { String idClaim = ownerRuleWithReadRestriction.getIdentityClaimOrDefault(); String key = ownerRuleWithReadRestriction.getOwnerFieldOrDefault(); @@ -135,6 +139,17 @@ && userNotInReadRestrictingGroups(readAuthorizedGroupsMap, authType)) { return request; } + private boolean doesRuleAllowNonOwnerSubscribe(AuthRule authRule, AuthorizationType authMode) { + AuthorizationType typeForRule = AuthorizationType.from(authRule.getAuthProvider()); + AuthStrategy strategy = authRule.getAuthStrategy(); + List operations = authRule.getOperationsOrDefault(); + return strategy != AuthStrategy.OWNER && strategy != AuthStrategy.GROUPS + && typeForRule != AuthorizationType.AMAZON_COGNITO_USER_POOLS + && typeForRule != AuthorizationType.OPENID_CONNECT + && typeForRule == authMode + && (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 {} }