Skip to content

Commit 68c40e1

Browse files
mattcreasermattcreaser
authored
fix(api): Don't prevent subscribing with API_KEY when there is also an owner-based rule (#2828)
1 parent 6dbb771 commit 68c40e1

File tree

3 files changed

+98
-6
lines changed

3 files changed

+98
-6
lines changed

‎aws-api/src/main/java/com/amplifyframework/api/aws/MultiAuthSubscriptionOperation.java

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ public synchronized void start() {
8080
));
8181
return;
8282
}
83-
subscriptionFuture = executorService.submit(this::dispatchRequest);
83+
queueDispatchRequest();
8484
}
8585

8686
private void dispatchRequest() {
@@ -101,7 +101,7 @@ private void dispatchRequest() {
101101
// For ApiAuthExceptions, just queue up a dispatchRequest call. If there are no
102102
// other auth types left, it will emit the error to the client's callback
103103
// because authTypes.hasNext() will be false.
104-
subscriptionFuture = executorService.submit(this::dispatchRequest);
104+
queueDispatchRequest();
105105
return;
106106
} catch (ApiException apiException) {
107107
LOG.warn("Unable to automatically add an owner to the request.", apiException);
@@ -119,7 +119,7 @@ private void dispatchRequest() {
119119
response -> {
120120
if (response.hasErrors() && hasAuthRelatedErrors(response) && authTypes.hasNext()) {
121121
// If there are auth-related errors queue up a retry with the next authType
122-
executorService.submit(this::dispatchRequest);
122+
queueDispatchRequest();
123123
} else {
124124
// Otherwise, we just want to dispatch it as a next item and
125125
// let callers deal with the errors.
@@ -129,7 +129,7 @@ private void dispatchRequest() {
129129
apiException -> {
130130
LOG.warn("A subscription error occurred.", apiException);
131131
if (apiException instanceof ApiAuthException && authTypes.hasNext()) {
132-
executorService.submit(this::dispatchRequest);
132+
queueDispatchRequest();
133133
} else {
134134
emitErrorAndCancelSubscription(apiException);
135135
}
@@ -142,7 +142,10 @@ private void dispatchRequest() {
142142
"Check your application logs for detail."
143143
));
144144
}
145+
}
145146

147+
private void queueDispatchRequest() {
148+
subscriptionFuture = executorService.submit(this::dispatchRequest);
146149
}
147150

148151
@Override

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,13 +83,19 @@ public <R> GraphQLRequest<R> decorate(
8383
AppSyncGraphQLRequest<R> appSyncRequest = (AppSyncGraphQLRequest<R>) request;
8484
AuthRule ownerRuleWithReadRestriction = null;
8585
Map<String, Set<String>> readAuthorizedGroupsMap = new HashMap<>();
86+
boolean publicSubscribeAllowed = false;
8687

8788
// Note that we are intentionally supporting only one owner rule with a READ operation at this time.
8889
// If there is more than one, the operation will fail because AppSync generates a parameter for each
8990
// one. The question then is which one do we pass. JavaScript currently doesn't support this use case
9091
// and it's not clear what a good solution would be until AppSync supports real time filters.
9192
for (AuthRule authRule : appSyncRequest.getModelSchema().getAuthRules()) {
92-
if (isReadRestrictingOwner(authRule)) {
93+
if (doesRuleAllowPublicSubscribe(authRule, authType)) {
94+
// This rule allows subscribing with the current authMode without adding the owner field, so there
95+
// is no need to continue checking the other rules.
96+
publicSubscribeAllowed = true;
97+
break;
98+
} else if (isReadRestrictingOwner(authRule)) {
9399
if (ownerRuleWithReadRestriction == null) {
94100
ownerRuleWithReadRestriction = authRule;
95101
} else {
@@ -114,7 +120,8 @@ public <R> GraphQLRequest<R> decorate(
114120
// We only add the owner parameter to the subscription if there is an owner rule with a READ restriction
115121
// and either there are no group auth rules with read access or there are but the user isn't in any of
116122
// them.
117-
if (ownerRuleWithReadRestriction != null
123+
if (!publicSubscribeAllowed &&
124+
ownerRuleWithReadRestriction != null
118125
&& userNotInReadRestrictingGroups(readAuthorizedGroupsMap, authType)) {
119126
String idClaim = ownerRuleWithReadRestriction.getIdentityClaimOrDefault();
120127
String key = ownerRuleWithReadRestriction.getOwnerFieldOrDefault();
@@ -135,6 +142,16 @@ && userNotInReadRestrictingGroups(readAuthorizedGroupsMap, authType)) {
135142
return request;
136143
}
137144

145+
private boolean doesRuleAllowPublicSubscribe(AuthRule authRule, AuthorizationType authMode) {
146+
AuthorizationType typeForRule = AuthorizationType.from(authRule.getAuthProvider());
147+
AuthStrategy strategy = authRule.getAuthStrategy();
148+
List<ModelOperation> operations = authRule.getOperationsOrDefault();
149+
return strategy == AuthStrategy.PUBLIC
150+
&& typeForRule == AuthorizationType.API_KEY
151+
&& authMode == AuthorizationType.API_KEY
152+
&& (operations.contains(ModelOperation.LISTEN) || operations.contains(ModelOperation.READ));
153+
}
154+
138155
private boolean isReadRestrictingOwner(AuthRule authRule) {
139156
return AuthStrategy.OWNER.equals(authRule.getAuthStrategy())
140157
&& authRule.getOperationsOrDefault().contains(ModelOperation.READ);

‎aws-api/src/test/java/com/amplifyframework/api/aws/auth/AuthRuleRequestDecoratorTest.java

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,66 @@ public void ownerArgumentNotAddedIfOwnerIsInCustomGroup() throws AmplifyExceptio
293293
}
294294
}
295295

296+
/**
297+
* Verify owner argument is NOT added if model contains both public key and owner-based authorization and the
298+
* requested auth type is API_KEY.
299+
* @throws AmplifyException if a ModelSchema can't be derived from the Model class.
300+
*/
301+
@Test
302+
public void doesNotAddOwnerWhenMultiAuthWithPublicKey() throws AmplifyException {
303+
final AuthorizationType mode = AuthorizationType.API_KEY;
304+
305+
// PublicAndOwner combines public and owner-based auth
306+
for (SubscriptionType subscriptionType : SubscriptionType.values()) {
307+
GraphQLRequest<PublicAndOwner> originalRequest = createRequest(PublicAndOwner.class, subscriptionType);
308+
GraphQLRequest<PublicAndOwner> modifiedRequest = decorator.decorate(originalRequest, mode);
309+
assertNull(getOwnerField(modifiedRequest));
310+
}
311+
312+
// PublicAndOwnerOidc combines public and owner-based auth with an OIDC claim
313+
for (SubscriptionType subscriptionType : SubscriptionType.values()) {
314+
GraphQLRequest<PublicAndOwnerOidc> originalRequest =
315+
createRequest(PublicAndOwnerOidc.class, subscriptionType);
316+
GraphQLRequest<PublicAndOwnerOidc> modifiedRequest = decorator.decorate(originalRequest, mode);
317+
assertNull(getOwnerField(modifiedRequest));
318+
}
319+
}
320+
321+
/**
322+
* Verify owner argument is added if model contains both owner-based and public-key
323+
* authorization and the auth mode is cognito.
324+
* @throws AmplifyException if a ModelSchema can't be derived from the Model class.
325+
*/
326+
@Test
327+
public void addsOwnerWhenMultiAuthWithCognito() throws AmplifyException {
328+
final AuthorizationType mode = AuthorizationType.AMAZON_COGNITO_USER_POOLS;
329+
final String expectedOwner = FakeCognitoAuthProvider.USERNAME;
330+
331+
for (SubscriptionType subscriptionType : SubscriptionType.values()) {
332+
GraphQLRequest<PublicAndOwner> originalRequest = createRequest(PublicAndOwner.class, subscriptionType);
333+
GraphQLRequest<PublicAndOwner> modifiedRequest = decorator.decorate(originalRequest, mode);
334+
assertEquals(expectedOwner, getOwnerField(modifiedRequest));
335+
}
336+
}
337+
338+
/**
339+
* Verify owner argument is added if model contains both owner-based and public-key
340+
* authorization and the auth mode is oidc.
341+
* @throws AmplifyException if a ModelSchema can't be derived from the Model class.
342+
*/
343+
@Test
344+
public void addsOwnerWhenMultiAuthWithOidc() throws AmplifyException {
345+
final AuthorizationType mode = AuthorizationType.OPENID_CONNECT;
346+
final String expectedOwner = FakeOidcAuthProvider.SUB;
347+
348+
for (SubscriptionType subscriptionType : SubscriptionType.values()) {
349+
GraphQLRequest<PublicAndOwnerOidc> originalRequest =
350+
createRequest(PublicAndOwnerOidc.class, subscriptionType);
351+
GraphQLRequest<PublicAndOwnerOidc> modifiedRequest = decorator.decorate(originalRequest, mode);
352+
assertEquals(expectedOwner, getOwnerField(modifiedRequest));
353+
}
354+
}
355+
296356
private <M extends Model> String getOwnerField(GraphQLRequest<M> request) {
297357
if (request.getVariables().containsKey("owner")) {
298358
return (String) request.getVariables().get("owner");
@@ -412,4 +472,16 @@ private abstract static class OwnerInCustomGroup implements Model {}
412472
)
413473
})
414474
private abstract static class OwnerNotInCustomGroup implements Model {}
475+
476+
@ModelConfig(authRules = {
477+
@AuthRule(allow = AuthStrategy.PUBLIC, operations = ModelOperation.READ),
478+
@AuthRule(allow = AuthStrategy.OWNER)
479+
})
480+
private abstract static class PublicAndOwner implements Model {}
481+
482+
@ModelConfig(authRules = {
483+
@AuthRule(allow = AuthStrategy.PUBLIC, operations = ModelOperation.READ),
484+
@AuthRule(allow = AuthStrategy.OWNER, identityClaim = "sub")
485+
})
486+
private abstract static class PublicAndOwnerOidc implements Model {}
415487
}

0 commit comments

Comments
 (0)