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

Add support of 'keepSessionInfo' bool param for strategies #1141

Open
2 tasks done
kukidon-dev opened this issue May 6, 2024 · 2 comments
Open
2 tasks done

Add support of 'keepSessionInfo' bool param for strategies #1141

kukidon-dev opened this issue May 6, 2024 · 2 comments

Comments

@kukidon-dev
Copy link
Contributor

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Need to allow users to prevent clearing session data during passport login. I use @fastify/passport with @fastify/cookies and @fastify/session (redis store). After each passport login, the session is regenerated which is reasonable (prevent token/CSRF fixation attack). But in some my cases, I want to just change session Id, I do not want to clear whole session data since I have required info before calling login function.
Currently, it is handled here:

if (request.session.regenerate) {

It would be much better to handle it as in original express passport: https://github.com/jaredhanson/passport/blob/0575de90dc0e76c1b8ca9cc676af89bd301aec60/lib/sessionmanager.js#L38

The idea is to give user a choice to choose clear session on regenerate call or not in each run of strategy. It is achieved by passing keepSessionInfo parameter in options to the strategy.
The description of why this was implemented in express passport - https://medium.com/passportjs/fixing-session-fixation-b2b68619c51d.

Motivation

Currently, to achieve behaviour of keeping session on login function call, we have to keep the list of property names that we want to keep in session which is not flexible (with clearSessionIgnoreFields params). Also, there is no way to keep session only for particular strategies. You can only always prevent clearing data from the session or always clear data.
Also, if you present new custom field in the session, you have to remember to always add it to the list of clearSessionIgnoreFields.

Example

Now, we can only do something like that:

import { Authenticator } from '@fastify/passport';

const authenticator = new Authenticator({ clearSessionIgnoreFields: ['field1', 'field2'] });

authenticator.use('strategyName', strategy);
server.register(authenticator.initialize());
server.register(authenticator.secureSession());

....

server.get(
  '/login',
  {
    preValidation: [
      authenticator.authenticate('strategyName', {redirect_uri: 'https://example.com/login/callback'})
    ],
  },
  () => {}
);

It would be great to have something like:

import fastifyPassport from '@fastify/passport';

fastifyPassport.use('strategyName', strategy);
server.register(fastifyPassport.initialize());
server.register(fastifyPassport.secureSession());

....

server.get(
  '/login',
  {
    preValidation: [
      fastifyPassport.authenticate('strategyName', {redirect_uri: 'https://example.com/login/callback', keepSessionInfo: true})
    ],
  },
  () => {}
);

server.get(
  '/alter-login',
  {
    preValidation: [
      fastifyPassport.authenticate('strategyName', {redirect_uri: 'https://example.com/alter-login/callback', keepSessionInfo: false})
    ],
  },
  () => {}
);

@mcollina
Copy link
Member

mcollina commented May 7, 2024

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@kukidon-dev
Copy link
Contributor Author

@mcollina Opened PR to address this issue. Please, review. Thanks!

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

No branches or pull requests

2 participants