Create GetFullPathToken #142
Conversation
|
It looks like this was closed in favor of https://github.com/fusionjs/fusion-plugin-jwt/pull/143/files. Personally I like this approach better, but will check with the team on what we think the best API is. Possibly some combination of the two where we can pass in an override per set call? |
|
@KevinGrandon Ah, interesting. This PR could help for |
|
LGTM |
There was a problem hiding this comment.
Thanks for the PR and sorry for the delay reviewing. We'd be happy to help land this, but this needs a little bit of work. Leaving review notes here in case you want to attempt to fix these yourself. Mainly:
- Flow needs to pass. You can reproduce this locally with
yarn flow. - Linters need to pass. You can autofix this locally with
yarn lint --fix. - We should export
GetFullPathTokenfrom index.js. - It would be great if we could add a unit test for GetFullPathToken, possibly in the existing
test.node.jsfile.
| const service: SessionService = { | ||
| from: memoize((ctx: Context) => { | ||
| return new JWTSession(ctx, {secret, cookieName, expires}); | ||
| return new JWTSession(ctx, {secret, cookieName, expires, getFullPath(ctx)}); |
There was a problem hiding this comment.
Currently this is specified as part of config here, but specified as another constructor parameter in JWTSession. I'm assuming that since config is fairly session specific, that this should be another parameter here instead of in config.
There was a problem hiding this comment.
@KevinGrandon Agree, so in this case we will sepaprate geFullPath out of config object. Thanks.
In some cases, user wants to have custom namespace/claims in JWT token.