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

[legacy-framework] Allow the ctx to be accessed within passport strategies #2058

Merged
merged 5 commits into from Mar 8, 2021

Conversation

james2406
Copy link

@james2406 james2406 commented Mar 4, 2021

Closes: blitz-js/legacy-framework#201

What are the changes and their implications?

This change allows access to the session inside the verify callback of the passport strategy.

Checklist

  • Changes covered by tests (tests added if needed)
  • PR submitted to blitzjs.com for any user facing changes

@blitzjs-bot blitzjs-bot bot added this to In Review in Dashboard Mar 4, 2021
@james2406
Copy link
Author

I tested this with the example auth project locally and it works as expected.

The only problem is, each strategy displays a typescript error whenever the passportAuth config is a callback. This is really strange because the config object types are the same for both the config object and config callback.

If you have any ideas why this could be happening, I'm happy to make the changes so this does not error!

Thanks

@flybayer
Copy link
Member

flybayer commented Mar 4, 2021

wow that is weird 🤔 I tried a few things and couldn't get it to work either.

Also, you can go ahead and open a PR to the docs repo to document this

@flybayer
Copy link
Member

flybayer commented Mar 4, 2021

For anyone else looking at this, do this:

  1. yarn
  2. yarn dev
  3. Open examples/auth/app/api/auth/[...auth].ts and see the type error

@james2406
Copy link
Author

wow that is weird 🤔 I tried a few things and couldn't get it to work either.

Yeah. I'll give it another go tomorrow, but something tells me this could be a bug with typescript. It just doesn't make sense.

Also, you can go ahead and open a PR to the docs repo to document this

Done! blitz-js/blitzjs.com#399

@james2406
Copy link
Author

james2406 commented Mar 4, 2021

Interesting. After a bit more investigation, I found the problem is the environment variables (whos types are guaranteed by the assert function) are reverting back to the type string | undefined once the callback is introduced.

I guess this makes sense as these variables could technically change.

The option would be to typecast:

consumerKey: process.env.TWITTER_CONSUMER_KEY as string,

Or add something like an environment.d.ts file to the project which would contain the following:

declare global {
  namespace NodeJS {
    interface ProcessEnv {
      TWITTER_CONSUMER_KEY: string;
      TWITTER_CONSUMER_SECRET: string;
    }
  }
}

export {};

I'll go for the environment.d.ts file approach for now, but can change it if you prefer the other option!

@flybayer
Copy link
Member

flybayer commented Mar 5, 2021

Sweet! Let's go with the as string approach because it's more copy-pastable and will prevent people from running into this (hopefully).

And let's add that type cast to the docs to, or at least mention it somehow there

@james2406
Copy link
Author

the as string approach because it's more copy-pastable

Yeah, it's a good point. I also thought the environment.d.ts file approach could confuse people if they are copy and pasting.

And let's add that type cast to the docs to, or at least mention it somehow there

Will do.

@flybayer flybayer changed the title Allow the session to be accessed within the passport strategy Allow the ctx to be accessed within passport strategies Mar 8, 2021
@flybayer flybayer merged commit ee7acd4 into blitz-js:canary Mar 8, 2021
Dashboard automation moved this from In Review to Done Mar 8, 2021
@james2406 james2406 deleted the passport-adapter-ctx branch March 13, 2021 11:19
@dillondotzip dillondotzip changed the title Allow the ctx to be accessed within passport strategies [legacy-framework] Allow the ctx to be accessed within passport strategies Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Ability to access the session data within a passport strategy
2 participants