-
-
Notifications
You must be signed in to change notification settings - Fork 570
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
Conversation
… from any location in the JWT, defaults to 'role' on the root of the JWT object
There was a problem hiding this 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 = '' | ||
} | ||
} |
There was a problem hiding this comment.
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);
Don't merge yet -- accidentally hit update branch. Changes not made yet. |
… from any location in the JWT, defaults to 'role' on the root of the JWT object
@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
@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. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
… 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
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
And it's in 👍 |
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. |
* 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
… 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.