Skip to content
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

Add authorization for user GraphQL endpoints #14

Merged
merged 1 commit into from
Jun 7, 2021

Conversation

e-wai
Copy link
Collaborator

@e-wai e-wai commented Jun 3, 2021

Notion ticket link

Add authorization checks to Graphql resolvers

Implementation description

  • Added new middleware method require_authorization_by_role_gql that performs the same checks as its non-GraphQL equivalent. Needed to change the arguments and how it accessed headers (request object is in info.context rather than just request)
  • Added the decorator to the appropriate mutations and queries. Justification for adding it on the business logic layer rather than the GraphQL layer (in schema.py) based on https://graphql.org/learn/authorization/

Also, I added the method into the existing auth.py instead of creating a new file because when we eliminate the REST code, the file name still makes sense and now requires a single method name refactor.

Steps to test

Setup -> I added users manually (see #11). Not really necessary, but makes for slightly more interesting testing 🤷

  1. NOT AUTHORIZED case: Open localhost:5000/graphql and mess around with any of the queries and any of the mutations except for resetPassword. Should respond that "You are not authorized to make this request.", eg. below. Can also produce this in Postman if you have no/an incorrect Authorization header.
{
  "errors": [
    {
      "message": "You are not authorized to make this request.",
      "locations": [
        {
          "line": 2,
          "column": 2
        }
      ],
      "path": [
        "entities"
      ]
    }
  ],
  "data": {
    "entities": null
  }
}
  1. AUTHORIZED case: Find access token with console.log(window.localStorage.getItem("localhost:AUTHENTICATED_USER")); in front-end console. Use this (formatted as Bearer <access-token>) as the Authorization header and try making any of the above failing requests. Should return as expected, no errors. I used Postman for this.

What should reviewers focus on?

  • Should this ticket include a GraphQL verison of require_authorization_by_user_id? No way to test without logout GraphQL endpoint being established
  • I don't know how to accomplish the headers in GraphiQL so I used Postman instead. I know Postman can hide CORS errors -- is this a flawed way of testing?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • For backend changes, I have run the appropriate linters: docker exec -it <backend-container-id> /bin/bash -c "black . && isort --profile black ."
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@e-wai e-wai requested a review from gaoxk June 3, 2021 05:42
@e-wai
Copy link
Collaborator Author

e-wai commented Jun 4, 2021

Did some front-end testing by adding @require_authorization_by_role_gql({"Admin"}) to wrap this reset password mutation method, and then attempting the reset password flow in different cases (not logged in, logged in as user, logged in as admin)

def mutate(root, info, email):
try:
auth_service.reset_password(email)
return ResetPassword(ok=True)
except Exception as e:
error_message = getattr(e, "message", None)
raise Exception(error_message if error_message else str(e))

  • Expected: I expected that this would produce Error: You are not authorized to make this request. when not logged in or when logged in as an user, and complete successfully if logged in as an admin.
  • Actual: Behaviour when logged out was fine. When logged in as admin and as user, request always returned bad request ({"errors":[{"message":"Cannot query field \"refresh\" on type \"Mutation\".","locations":[{"line":3,"column":5}]}]})

Copy link
Contributor

@gaoxk gaoxk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Fun fact: you can also get the jwt token via curl request, see Jenn's file PR (#8 )

Regarding using Headers for graphql requests, unfortunately, you're correct that graphiql doesn't support this. As far as I know, Postman should be fine. Personally, I installed Altair on to chrome and set the header Authorization to Bearer <token> with no problems.

Let's skip require_authorization_by_user_id for now.

@gaoxk
Copy link
Contributor

gaoxk commented Jun 4, 2021

Can you docker-compose down and up and try the front-end testing with resetPassword again? I tested the graphql endpoint and couldnt recreate the issues
reset_password doesn't require authorization so we don't need to worry about this too much, but it's good to double check. I also suspect this has something to do with the refresh gql mutation being incomplete, so we may resolve the problem relatively soon.

@e-wai
Copy link
Collaborator Author

e-wai commented Jun 5, 2021

hm I tried again and got the same behaviour. The GraphQL endpoint wasn't causing the problem, it's that it's querying refresh so I can't fully test it.

What happened for you when you clicked the "Reset Password" button when not logged in?

@gaoxk
Copy link
Contributor

gaoxk commented Jun 5, 2021

Only tested the endpoint from the backend. Good to know that this is an issue - I think you're good to rebase and squash and we can test again once the refresh endpoint is done.
{"errors":[{"message":"Cannot query field \"refresh\" on type \"Mutation\".","locations":[{"line":3,"column":5}]}]} this error occurs when graphql is unable to find the mutation named refresh`. If you go into graphiql and give it a random mutation name, you'll get the same problem.

@e-wai e-wai force-pushed the graphql-migration/authorization branch from 85498c4 to 3707ec4 Compare June 7, 2021 00:21
@e-wai e-wai merged commit 2fe41e8 into main Jun 7, 2021
@e-wai e-wai deleted the graphql-migration/authorization branch June 7, 2021 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants