-
-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Cognito (IAM) auth for subscriptions #103
Conversation
@michalkvasnicak yeah, I had a look at that while I was debugging, but I couldn't find the right data in the context object for some reason. actually I think the problem was onConnect never got called for $connect, only on $default. I couldn't figure out why, so I figured it might be a cognito vs custom authorizer thing. I didn't look too much into it after I realized it wasn't being called, tbh I'll give that another try, as it would be a much cleaner solution. I'll let you know when I have more info |
@jcane86 Then if everything is OK, server will return The problem is that Apollo client doesn't wait for ACK if I remember correctly. So maybe the problem is, that your client implementation fires subscriptions or operations in the meantime between INIT and ACK, so your context doesn't have data necessary to perform auth. But in that case you should see errors in logs or receive errors in responses 🤔 |
I think the problem is cognito data is only passed in the websocket's
$connect route, so before any gql messages, even before gql ack.
even though I'm signing all websocket messages with my cognito credentials,
apigw only forwards the data during websocket $connect for some reason. I'm
pretty sure I read it in AWS docs too
…On Thu, 6 Aug 2020, 15:12 Michal Kvasničák, ***@***.***> wrote:
@jcane86 <https://github.com/jcane86> onConnect is called when server
receives GQL_CONNECTION_INIT message from a client (
https://github.com/michalkvasnicak/aws-lambda-graphql/blob/master/packages/aws-lambda-graphql/src/Server.ts#L300
).
Then if everything is OK, server will return GQL_CONNECTION_ACK (
https://github.com/michalkvasnicak/aws-lambda-graphql/blob/master/packages/aws-lambda-graphql/src/Server.ts#L349
).
The problem is that Apollo client doesn't wait for ACK if I remember
correctly. So maybe the problem is, that your client implementation fires
subscriptions or operations in the meantime between INIT and ACK, so your
context doesn't have data necessary to perform auth. But in that case you
should see errors in logs or receive errors in responses 🤔
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFSHCWLRXKLBO2NGGP25SLR7JCU5ANCNFSM4PVRHYWQ>
.
|
Could you point me to the documentation where I can verify it? If that's the case, then the problem is that I'd appreciate a link to docs and maybe some easy example of how cognito is treated by Apigw for websockect connections. |
this is the clearest definition I could find in the docs:
https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api-route-keys-connect-disconnect.html
second paragraph
I'll try to provide some more detailed data or an example when I get a
minute to work on this
…On Thu, 6 Aug 2020, 15:22 Michal Kvasničák, ***@***.***> wrote:
Could you point me to the documentation where I can verify it? If that's
the case, then the problem is that $connect handling does nothing except
registers the connection. If we're limited to use it only during $connect
then we'd need to provide some sort of handler (like onConnect) that can
be called during $connect phase and store something on connection.
I'd appreciate a link to docs and maybe some easy example of how cognito
is treated by Apigw for websockect connections.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFSHCUSDEUJLWY4OXKYNA3R7JD3NANCNFSM4PVRHYWQ>
.
|
in fact, if you look at the apigateway console, the $connect route is the
only one that allows you to set up authorization in the route request
…On Thu, 6 Aug 2020, 15:39 Pepe Cane, ***@***.***> wrote:
this is the clearest definition I could find in the docs:
https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api-route-keys-connect-disconnect.html
second paragraph
I'll try to provide some more detailed data or an example when I get a
minute to work on this
On Thu, 6 Aug 2020, 15:22 Michal Kvasničák, ***@***.***>
wrote:
> Could you point me to the documentation where I can verify it? If that's
> the case, then the problem is that $connect handling does nothing except
> registers the connection. If we're limited to use it only during $connect
> then we'd need to provide some sort of handler (like onConnect) that can
> be called during $connect phase and store something on connection.
>
> I'd appreciate a link to docs and maybe some easy example of how cognito
> is treated by Apigw for websockect connections.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#103 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFSHCUSDEUJLWY4OXKYNA3R7JD3NANCNFSM4PVRHYWQ>
> .
>
|
Thank you very much! In that case it seems we need to add a way how to set up a data on connection during We have Then we can introduce another event handler that can return a data, that will be stored on connection. I'm not sure what name of an event handler to use, maybe |
OK sounds good... I'll have a go at that and let you know...
Thanks for the quick feedback!
…On Thu, 6 Aug 2020, 16:49 Michal Kvasničák, ***@***.***> wrote:
Thank you very much! In that case it seems we need to add a way how to set
up a data on connection during $connect phase that can be leveraged for
any authorizer. I think your approach is correct but let's do it more
"generic" if you agree.
We have onConnect handler already occupied (
https://github.com/michalkvasnicak/aws-lambda-graphql/blob/master/packages/aws-lambda-graphql/src/Server.ts#L70).
I think it'd be great to add a sentence or two to it's type definition so
developers can easily find out when in lifecycle this event handler is
called.
Then we can introduce another event handler that can return a data, that
will be stored on connection. I'm not sure what name of an event handler to
use, maybe onConnectionReceived or something better. Set up connection
data in $connect phase only and only if the handler is specified and
resolves to {[key: string]: any }.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFSHCWLGC3QZAEPPIBZOQTR7JOBHANCNFSM4PVRHYWQ>
.
|
Hi @michalkvasnicak , I added the option onWebsocketConnect now, see if that makes sense. I copied some code from onConnect to have it work kind of the same, although I'm not sure if I can do this: https://github.com/jcane86/aws-lambda-graphql/blob/42b6896f66c08699e1c8b0b73d93fb59de592992/packages/aws-lambda-graphql/src/Server.ts#L286-L290 , as the gql connection is not really initialized there yet. |
@@ -48,7 +48,8 @@ All options from Apollo Lambda Server and | |||
- **onError** (`(err: any) => void`, `optional`) - use to log errors from websocket handler on unknown error | |||
- **subscriptionManager** (`ISubscriptionManager`, `required`) | |||
- **subscriptions** (`optional`) | |||
- **`onConnect(messagePayload: object, connection: IConnection, event: APIGatewayWebSocketEvent, context: LambdaContext): Promise<boolean|object> | object | boolean`** (`optional`) - Return an object to set a context to your connection object saved in the database e.g. for saving authentication details | |||
- **`onWebsocketConnect(connection: IConnection, event: APIGatewayWebSocketEvent, context: LambdaContext): Promise<boolean|object> | object | boolean`** (`optional`) - onWebsocketConnect is called when the Websocket connection is initialized ($connect route). Return an object to set a context to your connection object saved in the database e.g. for saving authentication details. This is especially useful to get authentication details (API GW authorizers only run in $connect route) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this name is good! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a test for this? There are tests for connect phase in Server.test.ts
.
payload: { message: err.message }, | ||
}); | ||
|
||
await this.connectionManager.sendToConnection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's not possible to return anything in $connect
phase. So this should be removed I think.
connection, | ||
errorResponse, | ||
); | ||
await this.connectionManager.closeConnection(connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should be replaced with this.connectionManager.unregisterConnecition
which will delete the connection from Connection store but won't call ApiGatewayManager
.
@@ -336,7 +406,7 @@ export class Server< | |||
// set connection context which will be available during graphql execution | |||
const connectionData = { | |||
...connection.data, | |||
context: newConnectionContext, | |||
context: {...connection.data.context, ...newConnectionContext}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format the code with prettier 🙂
OK, I think that should do it. Tests were green on my machine at least. Let me know what you think Cheers |
HI jcane86, |
@ThomasP1988 I believe that @jcane86 uses |
correct! I get the Auth data from IAM in onWebsocketConnect, and build an user object from it, using some more data stored in my dB... I then include that object in the return of onWebsocketConnect and I can access it from context everywhere else |
Thank you for fast answering, so are you sending AWS v4 sign from apollo or a jwt token ? do you use a ApolloLink ? |
Ah, yes, thats the tricky bit... I kinda cobbled together an ApolloLink to sign all requests using the library aws4... I made a copy for you to check out here: https://gist.github.com/jcane86/9a51a49d1b88f7c9e5f822b4050b3759 Just keep in mind that's a quick and dirty implementation for my usecase, please check it and make sure you understand it before using it in anything close to production, I'll make no claims to its correctness. You'll find some lines dealing with clock drift, that was a pretty nasty one to find and debug, but basically its a way to make the signatures work when the user's clock is not perfectly in sync with AWS's. I may release this Link publicly one day, or submit a PR to aws4 to handle the drift problem more elegantly, but thats all I got for now... hope it helps. PS: That is what I'm doing for the websockets link in my apollo client, because you can't set headers in websockets. The one for the http link is similar, but using header based auth. |
Thank you very much, very helpful |
Hi guys, NB: this uses Apollo client v3. https://gist.github.com/ThomasP1988/9f4becae3b9fd8a8646f2918898b92da |
Hey..
We use IAM for our auth, and have been using it with this lib for normal queries and mutations without issues. Getting started with subscriptions now, and had to do a little hack to be able to access the IAM info. Thought I'd share it with you in case you find it useful.
I wasn't sure if I could put this info inside connection.data.context, so I just left it in connection.data.identity
The way I am using this is in onConnect, I access the identity data from the connection object, I build my user context, and return it from onConnect, then I have it accessible in the resolvers.
Not sure if that's the most straightforward way to weave the identity data down to resolvers, happy to hear of better ways to do it.
PS: Thanks for the great library, it's saved our team a lot of work and time, and it's just a pleasure to use.
Cheers!
Pepe