kkemple / graphql-auth

🔒 GraphQL authentication and authorization middleware
MIT License
278 stars 19 forks source link

Dynamic CB Not Firing When Hit By Differently Scoped User #8

Closed dcurletti closed 6 years ago

dcurletti commented 6 years ago

Dev setup:

I am using graphql-auth with:

What you did:

I have an endpoint that is using the dynamic scope cb to change the scoped based on the params.

    Mutation: {
        sendMessage: withAuth(
            (obj, { input }, context) => {
                console.log('params input: ', input)
                return input.A ? ["scope:A"] : ["scope:B"];
            }, 
            async (
                obj,
                { input },
                context
            ) => {...code}

I log in to my app as User A with scope A I hit the above mutation I see the console log I receive the correct result I log out I log in to my app as User B with scope B I hit the above mutation I do not see a console log I get a permission denied error I terminate the server process I start the server process (I am still logged in with B from the previous session) I hit the above mutation I see the console log I receive the correct result I log out (with User B) I log in to my app as User A with scope A I hit the above mutation I do not see a console log I get a permission denied error

What happened:

In my context creation function, the one that puts auth on the context, I can see that the correct scopes are being applied. However, I believe the error has something to do with the dynamic cb being cached or not executed again (because I do not see the console log)

AuthorizationError: Permission Denied!
    at AuthorizationError (/Users/test/workspace/petigree/code/node_modules/graphql-auth/index.js:11:5)
    at /Users/test/workspace/petigree/code/node_modules/graphql-auth/index.js:51:14
    at next (native)
    at step (/Users/test/workspace/petigree/code/node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
    at /Users/test/workspace/petigree/code/node_modules/babel-runtime/helpers/asyncToGenerator.js:35:14
    at Promise.F (/Users/test/workspace/petigree/code/node_modules/core-js/library/modules/_export.js:35:28)
    at /Users/test/workspace/petigree/code/node_modules/babel-runtime/helpers/asyncToGenerator.js:14:12
    at /Users/test/workspace/petigree/code/node_modules/graphql-auth/index.js:35:3
    at /Users/test/workspace/petigree/code/node_modules/graphql-tools/src/schemaGenerator.ts:536:22
    at resolveFieldValueOrError (/Users/test/workspace/petigree/code/node_modules/graphql/execution/execute.js:498:12)
    at resolveField (/Users/test/workspace/petigree/code/node_modules/graphql/execution/execute.js:462:16)
    at /Users/test/workspace/petigree/code/node_modules/graphql/execution/execute.js:284:20
    at process._tickCallback (internal/process/next_tick.js:109:7)
dcurletti commented 6 years ago

The issue is due to this line: https://github.com/kkemple/graphql-auth/blob/master/index.js#L41

It overrides the dynamic scope cb meaning that the scope is forever set to whatever it gets evaled to the very first time it is called.

kkemple commented 6 years ago

hey @dcurletti! thanks for reporting the issue! is this something you would like to try and submit a PR for? if not I'll try to get to it in the next few days :smile:

dcurletti commented 6 years ago

Yea, I'll try to get it done today.

kkemple commented 6 years ago

no rush! just wanted to see if you were interested in tackling it! also be sure to add yourself to contributors list!! 🙌

kkemple commented 6 years ago

@dcurletti just wanted to follow up on this and see if you needed a second set of eyes, happy to help out in any way i can!

sakhmedbayev commented 6 years ago

same thing here :-(

sakhmedbayev commented 6 years ago

@dcurletti have you managed to fix this issue?

BipinBhandari commented 6 years ago

Guys I am actively using this package on my project. I also faced this problem and actually dynamic callback doesn't work at all except the very first time. Here is the pull request I created to fix this : https://github.com/kkemple/graphql-auth/pull/9

dcurletti commented 6 years ago

@BipinBhandari nice- I did the same as you, expect i only moved where the variable was being defined.

BipinBhandari commented 6 years ago

Can anyone explain me significance of Promise.resolve().then() in https://github.com/kkemple/graphql-auth/blob/master/index.js#L41-L43

EDIT: I think it was just for performance. I have restored it to its original version in my PR.

dcurletti commented 6 years ago

puts that requiredScope function at the end of the event loop, thereby freeing other functions to run before it gets called.

I'm assuming it is to protect against anyone doing a DB call, or any other long-running function, in their auth scope function.

kkemple commented 6 years ago

just published 0.2.1 which includes #9, let me know if that resolves the issue

mitjade commented 6 years ago

@kkemple Something seems to be wrong with the 0.2.1 npm package. There is no build folder.

kkemple commented 6 years ago

Will fix in about 30 min and push out to NPM!

On Thu, Apr 12, 2018, 1:44 PM mitjade notifications@github.com wrote:

@kkemple https://github.com/kkemple Something seems to be wrong with the 0.2.1 npm package. There is no build folder.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/kkemple/graphql-auth/issues/8#issuecomment-380888224, or mute the thread https://github.com/notifications/unsubscribe-auth/ADdjNJBWzgeL9-k8s2YjCcb7mpMnGxNSks5tn5JggaJpZM4Rt7xv .

mitjade commented 6 years ago

@kkemple Still a problem. Will there be a new version or will you fix 0.2.1?

kkemple commented 6 years ago

I have deployed 0.2.2 to resolve this issue!

mitjade commented 6 years ago

@kkemple All ok now. Thank you!

kkemple commented 6 years ago

great! thanks for your patience! going to close this issue, re-open if issue is not fully resolved!!