Btpc 139 authentication #28
Conversation
aced0
left a comment
aced0
left a comment
There was a problem hiding this comment.
I'm not sure about the cookie authentication :> Besides from some pedantic suggestions this looks good.
server/validatedGraphqlProxy.ts
Outdated
| dest: string; | ||
| } | ||
|
|
||
| const validatedGraphqlProxy = (appApiSecretKey: string) => { |
There was a problem hiding this comment.
pedantic mode: wouldn't it be a validatingGraphqlProxy? also what is it validating?
There was a problem hiding this comment.
It's validating the JWT and also calling the graphqlProxy. Should I name it "validateTokenGraphqlProxy"? Or would you like to split it again? This current way we don't need to tamper with session fields and can just pass the shop / auth token over.
server/validatedGraphqlProxy.ts
Outdated
|
|
||
| const shopOriginCookie = ctx.cookies.get('shopOrigin'); | ||
| if (shopOriginCookie) { | ||
| // TODO Is cookie already validated by KOA? Or just use https://shopify.dev/tutorials/authenticate-your-app-using-session-tokens instead? |
There was a problem hiding this comment.
AFAIIK: The cookie gets validated by the graphQLProxy from shopify...
AFAIK: When coming from the embedded app, there is no session token available (or is there?)
AFAIK: If you submit your own password, i think then the graphQLProxy cookie validation is skipped...
server/validatedGraphqlProxy.ts
Outdated
| const jwtFromHeader = ctx.headers['auth-token']; | ||
|
|
||
| if (jwtFromHeader) { | ||
| const decoded: WebTokenObject = jwt.verify( |
There was a problem hiding this comment.
I think we should wrap this token verification in a try catch and return a custom message/log a custom message and stop the execution of the request to make it more expressive. Also some tests would be nice that further illustrate this behavior.
(would be nice, but if you aren't motivated to write some I would totally agree ^^)
Edit nice library you have found. The verify method makes this whole validation stuff really straight forward 👍
There was a problem hiding this comment.
I still would go with authenticatedGraphQLProxy and not validated, since validated could mean anything.
And to make it clear for the reader
let shopUrl;
if (jwtFromHeader) {
validateToken(jwtFromHeader);
shopUrl = getShopUrl(jwtFromHeader);
else {
validateCookie(cookie); //or make comment that cookie is always validated
shopUrl = ctx.cookies.get('shopOrigin');
}
if (!shopOrigin) throw
Edit Sorry I was reviewing a pull request from work and got in the habit of adding example snippets 🙄
server/validatedGraphqlProxy.ts
Outdated
| @@ -20,15 +20,23 @@ const validatedGraphqlProxy = (appApiSecretKey: string) => { | |||
| const jwtFromHeader = ctx.headers['auth-token']; | |||
|
|
|||
| if (jwtFromHeader) { | |||
There was a problem hiding this comment.
If the auth-token is not provided, we should make sure the cookie is present and validated (if - else).
|
@aced0 I removed the cookie completeley, see Shopify/shopify-demo-app-node-react#325 |
aced0
left a comment
aced0
left a comment
There was a problem hiding this comment.
now it makes a really good impression 💯
| { | ||
| plan: { | ||
| appRecurringPricingDetails: { | ||
| price: { amount: 10, currencyCode: USD } |
There was a problem hiding this comment.
too cheap for such a great app... but what can you do ^^
No description provided.