Closed Eveliyse closed 2 years ago
Right, after more investigation, the more "root problem" seems to stem from the fact that graphql_headers
looks like this:
{
"accept": "*/*",
"content-type": "application/json; charset=UTF-8",
"host": "XXXXX",
"x-amz-date": "XXXXX",
"X-Amz-Security-Token": "XXXXX",
"Authorization": "AWS4-HMAC-SHA256 Credential=XXXXX, SignedHeaders=accept;content-type;host;x-amz-date;x-amz-security-token, Signature=XXXXX",
"x-amz-user-agent": "aws-amplify/3.0.4"
}
and _awsRealTimeIAMHeader
returns something like:
{
"accept": "application/json, text/javascript",
"content-encoding": "amz-1.0",
"content-type": "application/json; charset=UTF-8",
"host": "XXXXX",
"x-amz-date": "XXXXX",
"X-Amz-Security-Token": "XXXXX",
"Authorization": "AWS4-HMAC-SHA256 Credential=XXXXX, SignedHeaders=accept;content-encoding;content-type;host;x-amz-date;x-amz-security-token, Signature=XXXXX"
}
The breaking differences seem to be:
_awsRealTimeIAMHeader
includes the field content-encoding
which is required for authorization. https://docs.aws.amazon.com/appsync/latest/devguide/real-time-websocket-client.html#iam This field is missing from the graphql_headers
graphql_headers
is "/" but should be "application/json, text/javascript" as per the document I just linked.As a result of this I think actually maybe the correct approach to this fix is actually to not use the graphql_headers
I have updated this PR to reflect this
This is affecting me as well, and I've confirmed that commenting out the graphql_headers works. Could we get some feedback on whether this PR is acceptable or not?
I'm seeing a similar issue
@Eveliyse or @chrislim have you been able to get a fork working with these changes? I'm not quite sure how it would work with the lerna setup.
@manueliglesias has this been discussed at all? Subscription functionality is completely broken for all of our automated tests
I was poking around the codebase and found that this line:
corresponds to this one: https://github.com/awslabs/aws-mobile-appsync-sdk-js/blob/master/packages/aws-appsync-subscription-link/src/realtime-subscription-handshake-link.ts#L237
The graphql_headers method itself is called here: https://github.com/awslabs/aws-mobile-appsync-sdk-js/blob/master/packages/aws-appsync-subscription-link/src/realtime-subscription-handshake-link.ts#L269
In any case, L110 looks like a code smell:
graphql_headers: () => (headers),
Why would headers have parentheses around it?
I suspected that what was intended was:
graphql_headers: function () { return ({headers}); }
When I made that code change, subscriptions worked for me. (Note: I was editing the file directly in my node_modules folder, I'm figuring out a way to be able to include it in a production build...it seems like I could fork this repo, but I'd have to make two changes, one for where aws-appsync gets its aws-appsync-subscription-link from and then change aws-appsync-subscription-link as well)
UPDATE: eh I think having the headers as an attribute of an object may have worked as a fluke. attempting to expand the attributes of the headers fails, so the original solution is preferable.
I ended up directly modifying the prototype method so that it won't send graphql_headers to unblock me from having to wait for this PR to be merged.
// HACK since this PR {@link https://github.com/awslabs/aws-mobile-appsync-sdk-js/pull/633/files} has not been accepted
// after several months, we go ahead and modify the method to drop the graphql_headers that are causing subscription problems.
const {
AppSyncRealTimeSubscriptionHandshakeLink,
} = require('aws-appsync/node_modules/aws-appsync-subscription-link/lib/realtime-subscription-handshake-link');
const oldStartSubscription =
AppSyncRealTimeSubscriptionHandshakeLink.prototype
._startSubscriptionWithAWSAppSyncRealTime;
AppSyncRealTimeSubscriptionHandshakeLink.prototype._startSubscriptionWithAWSAppSyncRealTime =
function (a) {
if (a.options) {
delete a.options.graphql_headers;
}
return oldStartSubscription.call(this, a);
};
Nice workaround @chrislim!
Hi @Eveliyse! We discussed these changes further today, and though this PR is certainly in the right direction, we will need to maintain the GraphQL headers for auth types other than IAM so that we can continue to support custom headers for our customers. Because of this, the approach in this PR won't work for all use-cases. However, we have a PR in progress right now that will solve this issue, and should be merged this week. Thank you for your help in identifying and solving this issue! Please reach out if you have further questions 😄
Issue #, if available: https://github.com/awslabs/aws-mobile-appsync-sdk-js/issues/619
Description of changes: I believe that this issue stems from the fact that
graphql_headers
seem to be an option passed in and used here to create the "start subscription" message: https://github.com/awslabs/aws-mobile-appsync-sdk-js/blob/master/packages/aws-appsync-subscription-link/src/realtime-subscription-handshake-link.ts#L256 We are creating the options here: https://github.com/awslabs/aws-mobile-appsync-sdk-js/blob/master/packages/aws-appsync-subscription-link/src/realtime-subscription-handshake-link.ts#L104but these
graphql_headers
are not used to initialize the websocket connection: https://github.com/awslabs/aws-mobile-appsync-sdk-js/blob/master/packages/aws-appsync-subscription-link/src/realtime-subscription-handshake-link.ts#L366I've opted to fix this issue by passing the
graphql_headers
to_initializeWebSocketConnection
so that the headers match.This change seems to have been introduced here: https://github.com/awslabs/aws-mobile-appsync-sdk-js/blame/master/packages/aws-appsync-subscription-link/src/realtime-subscription-handshake-link.ts#L104 In a PR which made progress towards making the subscription link work like the amplify one. However I noted that the amplify use of
_startSubscriptionWithAWSAppSyncRealTime
does not pass ingraphql_headers
so maybe the actual fix is for us to not set thegraphql_headers
option? https://github.com/aws-amplify/amplify-js/blob/5a8ea9c5b736e49d0cbfa06e0f0e9405a942c48d/packages/pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts#L185If the preferred fix is to actually remove the
graphql_headers
options being passed to_startSubscriptionWithAWSAppSyncRealTime
then I can make that change.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.