-
Notifications
You must be signed in to change notification settings - Fork 38
Allow user to override getToken
function.
#164
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
base: main
Are you sure you want to change the base?
Conversation
Hi @TheCynosure, Can you please sign the commits ? Currently the commits do not have verified signatures. Reference: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
* Function to get token from request. | ||
* Defaults to getToken from 'oauth2-bearer'. | ||
*/ | ||
getToken?: typeof getToken; |
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.
Do we really need to completely override the getToken method?
Wouldn't it be possible to simply extract the token from the specified location and return it instead?
interface AuthOptions {
tokenLocations?: ('header' | 'query' | 'body')[];
// other options...
}
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 updated the PR @nandan-bhat to allow a user to specify a singular tokenLocation. Multiple token locations seemed to get confusing as it could apply to multiple locations.
@@ -84,7 +89,7 @@ export const auth = (opts: AuthOptions = {}): Handler => { | |||
|
|||
return async (req: Request, res: Response, next: NextFunction) => { | |||
try { | |||
const jwt = getToken( | |||
const jwt = (opts.getToken ?? getToken)( |
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.
How about pass the tokenLocations
and implement the logic inside getToken
method to return the token from the location specified in tokenLocations
?
Hey @TheCynosure, I think we can make this flexible by adding an optional If What do you think? |
9318fbc
to
f9af1e7
Compare
@nandan-bhat I liked the simplicity of overriding If unspecified, the code checks the default locations for headers, query params, and body. |
Hey @TheCynosure, I have already gone through the PR description, but I’d love to understand your specific use case better. What’s the reason for needing to customize the If the goal is simply to meet the requirement in #147, wouldn’t it be enough to define With Express: If your environment doesn't allow sending I’d love to understand your use case better since I don’t have full context on what this PR is addressing. Could you share more details on the specific problem it’s solving ? Let me know your thoughts! |
* Header, Body, or Query Parameter location to retrieve the JWT from. | ||
* Defaults to "authorization" header and "access_token" body and query parameter. | ||
*/ | ||
tokenLocation?: string; |
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’m curious, why is tokenLocation
defined as a flexible string instead of something stricter like this?
tokenLocation?: "header" | "body" | "query";
It looks like the intention isn’t just to specify where the token should be retrieved from, but also to allow customization of the query parameter name. That seems like a slightly different requirement than what was discussed in reference #147.
Hey @nandan-bhat, And yeah good callout on not solving #147. I think the original solution of overriding getToken completely solved both, but now this approach only solves my issue. |
What do you think about these approaches? Option 1: Use the "Authorization" header for everything – Okta and other providers both use it, and I’d pick the right middleware based on another header, like Option 2: Tweak the headers before calling auth({}) – Basically, modify them upfront so they work smoothly with auth({}) without extra handling later. I understand that overriding |
Description
Allows the user to specify a custom token location for retrieving the authorization/access_token JWT. Currently, the
getToken
function is fixed and the user is only able to use one of the pre-determined locations ("authorization" for headers, "access_token" for query/body) for their authorization tokens.Now the developer can specify whatever location they desire. This is asked for in #147 and I need this as well.
This should not be a breaking change.
References
#147
Testing
Unit tests were added for this functionality. Should be verifiable with
npm test
. Tested on NodeJS v18.19.1, Ubuntu 24.04.Documentation
Documentation was updated with
npm run docs
, it led to more changes than I expected, but it seems to be from a commit hash reference changing.Checklist