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

feat(postgraphql): extract pgRole from arbitrary JWT path #480

Merged
merged 9 commits into from
Jun 8, 2017
Merged

feat(postgraphql): extract pgRole from arbitrary JWT path #480

merged 9 commits into from
Jun 8, 2017

Conversation

angelosarto
Copy link
Collaborator

… from any location in the JWT, defaults to 'role' on the root of the JWT object.

When working with Auth0 as the source of the JWT it can be difficult to write directly to the root of the jwt (their rules engine allows writing to the jwt but it has to be namespaced with a url-like property name.)

I created this PR that allows you to specify a path in the jwt from which to set the pgRole; if unset it defaults to the current 'role' property on the root of the jwt.

Since this is my first commit I probably missed something, though I did add unit tests where appropriate, updated docs, ran all tests, (ran the linter) and tried to follow the commit guidelines.

… from any location in the JWT, defaults to 'role' on the root of the JWT object
@benjie benjie changed the title feat(postgraphql) Added an option to allow the pgRole to be extracted… feat(postgraphql): allow the pgRole to be extracted from arbitrary JWT path Jun 6, 2017
@benjie benjie changed the title feat(postgraphql): allow the pgRole to be extracted from arbitrary JWT path feat(postgraphql): extract pgRole from arbitrary JWT path Jun 6, 2017
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Really good, just one slight refactor request. Just push additional commits to this branch, we use squash-merge so they'll all be rolled up into one commit ultimately.

i = rolePath.length
roleClaim = ''
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather see the logic here extracted to utility function; e.g. the below is a slightly modified version of lodash's get:

function getPath(inObject, path) {
  let object = inObject
  // From https://github.com/lodash/lodash/blob/master/.internal/baseGet.js
  let index = 0
  const length = path.length

  while (object && index < length) {
    object = object[path[index++]]
  }
  return (index && index === length) ? object : undefined
}

// ...

const roleClaim = getPath(jwtClaims, jwtRole);

@angelosarto
Copy link
Collaborator Author

Don't merge yet -- accidentally hit update branch. Changes not made yet.

Angelo Sarto added 2 commits June 8, 2017 10:24
@benjie
Copy link
Member

benjie commented Jun 8, 2017

@angelosarto Cool; let me know when you're ready for me to re-review.

… from any location in the JWT, defaults to 'role' on the root of the JWT object
@angelosarto
Copy link
Collaborator Author

angelosarto commented Jun 8, 2017

@benjie Ok - I extracted the function as suggested - it actually works great as is so I added it as a private method in the same file. all the tests still passed so I think we are good to go.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Couple slight tweaks; looking good!


/**
* Safely extract a nested object or undefined where inObject is any object and path is
* an array of indexes into an object
Copy link
Member

Choose a reason for hiding this comment

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

Safely gets the value at `path` (array of keys) of `inObject`.

*
* @private
*/
function getPath(inObject: any, path: any): any {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like (inObject: mixed, path: Array<string>)?

docs/library.md Outdated
@@ -59,6 +59,7 @@ Arguments include:
- `pgDefaultRole`: The default Postgres role to use. If no role was provided in a provided JWT token, this role will be used.
- `jwtSecret`: The secret for your JSON web tokens. This will be used to verify tokens in the `Authorization` header, and signing JWT tokens you return in procedures.
- `jwtAudiences`: The audiences to use when verifing the JWT token. If not set the audience will be `['postgraphql']`.
- `jwtRole`: a comma seperated list of strings that create a path in the jwt from which to extract the postgres role. if none is provided it will use the key `role` on the root of the jwt.
Copy link
Member

@benjie benjie Jun 8, 2017

Choose a reason for hiding this comment

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

if none is provided < missing capital following fullstop

Angelo Sarto added 2 commits June 8, 2017 14:12
… from any location in the JWT, defaults to 'role' on the root of the JWT object
… from any location in the JWT, defaults to 'role' on the root of the JWT object
@angelosarto
Copy link
Collaborator Author

angelosarto commented Jun 8, 2017

@benjie Thanks for the review! Added the fixes to docs, and the typescript types on the function. Also removed optional on the signature as a default is provided instead of undefined. Oops typo in the docs and an initial capital on the 'A' added

… from any location in the JWT, defaults to 'role' on the root of the JWT object
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

🙏

@benjie benjie merged commit 607f662 into graphile:master Jun 8, 2017
@benjie
Copy link
Member

benjie commented Jun 8, 2017

And it's in 👍

@doeg
Copy link

doeg commented Jul 16, 2017

Hey @angelosarto,

Thank you for implementing this feature. I'm implementing Auth0 alongside postgraphql and your PR saved me a ton of time and frustration (and hacky code). 😄 I'm running a build-from-source version to pick up your recent change and it's working like a charm.

And thanks @benjie for merging it. I really appreciate this project.

Belline pushed a commit to Belline/postgraphql that referenced this pull request Dec 18, 2017
* feat(postgraphql) Added an option to allow the pgRole to be extracted from any location in the JWT, defaults to 'role' on the root of the JWT object

* feat(postgraphql) Added an option to allow the pgRole to be extracted from any location in the JWT, defaults to 'role' on the root of the JWT object

* feat(postgraphql) Added an option to allow the pgRole to be extracted from any location in the JWT, defaults to 'role' on the root of the JWT object

* feat(postgraphql) Added an option to allow the pgRole to be extracted from any location in the JWT, defaults to 'role' on the root of the JWT object

* feat(postgraphql) Added an option to allow the pgRole to be extracted from any location in the JWT, defaults to 'role' on the root of the JWT object

* feat(postgraphql) Added an option to allow the pgRole to be extracted from any location in the JWT, defaults to 'role' on the root of the JWT object
benjie added a commit that referenced this pull request Sep 16, 2023
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.

None yet

3 participants