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

Authorization and authentication #26

Merged
merged 5 commits into from
May 1, 2016
Merged

Authorization and authentication #26

merged 5 commits into from
May 1, 2016

Conversation

calebmer
Copy link
Collaborator

This PR's goal is to add authorization and optional authentication to PostGraphQL. As of the opening of this issue I have only added authorization support using JWTs. Look at the tests, look at the readme, and look at this specification and tell me if you like this authorization schema. Authentication (creating tokens) will be decided later on.

@hardchor
Copy link

I like the spec, but would you just parse all claims globally or, or namespace them, e.g.

{
  "roles": {
    "sub": "postgraphql",
    "role": "user",
    "user_id": 2
  }
}

We've used JWTs in the past to store arbitrary data (that needed to be trusted), which would get picked up and set as a role with your solution.

@calebmer
Copy link
Collaborator Author

Only the top level role would be used to set the role. If there was a nested object it would be stringified. So role inside the roles object would not be used to set the role. Does this still cause conflicts?

If so we could have a flag in the JWT to switch it off.

@hardchor
Copy link

Ah sorry, I wasn't aware it's only the role field that gets set, everything else can get discarded if not needed. If it's just role, then chances for collisions are relatively low.

@calebmer
Copy link
Collaborator Author

@hardchor is there a way I could improve the documentation/specification to make this more clear?

@hardchor
Copy link

I just wasn't quite aware of how authorisation in Postgres works (do very little DB related these days). After reading the spec again, it does make sense!

@hardchor
Copy link

hardchor commented May 1, 2016

Are you going to pass the token / claims in through the GraphQL context? I'm doing the request handling myself (and just use createGraphqlSchema as discussed) and would also do JWT decoding / verification.

@calebmer
Copy link
Collaborator Author

calebmer commented May 1, 2016

Currently, the token is used to run some queries on the PostgreSQL client and is unused by resolvers so it isn't needed in the context. I also don't want to be setting the local variables more than once so putting token serialization in the resolver isn't optimal. Take a look at createServer.js for the code I'm using currently to run a request.

I think what I'll probably do is make a small module (pg-jwt probably) which will implement the specification so you can serialize a token from anywhere.

I could also add a helper function, executeGraphqlQuery or getGraphqlContext to help setup the context.

@calebmer
Copy link
Collaborator Author

calebmer commented May 1, 2016

I was going to wait to also implement authentication before merging this PR, but I don't have any strong ideas at the moment. How would everyone feel if I merged and released this and work on an authentication solution in another PR?

Releasing this would not be a breaking change, servers would still work even if they never use tokens. People using PostGraphQL as a library would not be affected at all by this PR as it only changes code in the server.

@hardchor
Copy link

hardchor commented May 1, 2016

I'd say handle authorisation and authentication separately and get it in!

@calebmer calebmer merged commit 1388542 into master May 1, 2016
@calebmer calebmer deleted the feat/auth branch May 1, 2016 21:23
@calebmer
Copy link
Collaborator Author

calebmer commented May 1, 2016

Authorization support has been added in 1.2.0! Documentation on the feature isn't very rich, I'm going to wait for authentication before writing comprehensive documentation on authorization/authentication

benjie added a commit that referenced this pull request Jan 27, 2020
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