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

Refactoring config/passport.js #1279

Open
YasharF opened this issue Oct 11, 2023 · 8 comments
Open

Refactoring config/passport.js #1279

YasharF opened this issue Oct 11, 2023 · 8 comments

Comments

@YasharF
Copy link
Collaborator

YasharF commented Oct 11, 2023

NOTE: Prior to any PRs the potential design and implementation needs to be discussed in this issue as this is not a minor code change. Please make sure you have a good understanding of oAuth 2.0 including refresh tokens, etc. prior to any work on this issue.

Background: At a higher level, the strategies in passport.js seem to be following similar patterns.
Problem: Maintenance of the strategies when there is a change to the underlying libraries is cumbersome. We can also end up with bugs because a fix may have been applied to most of the strategies but one or two may have been missed. To provide an example, the passport,js mongoose 7 upgrade resulted in a ~900 line diff, but at the core of it, it was the same change getting repeated in each strategy: 55defd3#diff-fa60852f5a2e88327803171e7f8972c7799de05a17566b46e958f27c4c7b965e

Potential solution:
We refactor the code so there is a core generic routine for handling oAuth, which uses configs for each oAuth provider. This potential solution can also allow us or users to easily add (or remove) oAuth providers without worrying about potential token handling bugs in specific implementations.

NOTE: Prior to any PRs the potential design and implementation needs to be discussed in this issue as this is not a minor code change. Please make sure you have a good understanding of oAuth 2.0 including refresh tokens, etc. prior to any work on this issue.

@bhavuk2002
Copy link

I relatively need to open source and contributions, but if you can guide me how to make the required changes, i can work and deliver excellent results.

@Chirag77302
Copy link

Hey @YasharF
I have prior experience of working on OAuth projects. I am up for resolving this issue. You can assign this to me & we can have the further discussions around this.

@YasharF
Copy link
Collaborator Author

YasharF commented Oct 13, 2023

No assignment at this point. Please discuss as you may see fit.

@YasharF YasharF changed the title Refactoring passport.js Refactoring config/passport.js Oct 17, 2023
Repository owner deleted a comment from Ahmedtakrit Nov 9, 2023
@Mano3009
Copy link

We could improve Passport.js OAuth setup by organizing code into smaller parts, making it easier to manage multiple login options. Create a common function to handle different services and set up a way to easily add new logins. This will simplify the process and make it smoother to expand login choices.

@Mano3009
Copy link

Can someone check on the pr #1293 , i focused on importing the already defined "handleservice" function and utilizing it within Passport authentication strategies.

@ATHARVA262005
Copy link

hey @YasharF

You can assign this to me

The common approach is to handle each strategy separately, but there's an efficient way to centralize the logic to avoid repetition. You can create a common function to handle authentication across different providers. This function will handle linking accounts, checking if the user already exists, and managing tokens for various providers.

Here's a simplified overview of how this works:

Setup the strategies: Use passport.use for each provider (Google, Facebook, etc.), passing in the necessary credentials from .env and callback URLs.

Common handling logic: Create a function that processes the authentication callback. This function checks if the user is logged in, links accounts if needed, or creates a new user if it’s a new login.

Serialize and deserialize user: Store the user in the session using serializeUser and retrieve them using deserializeUser.

config/passport.js

const passport = require('passport');
const OAuth2Strategy = require('passport-oauth2');
const User = require('../models/User');

// A common handler for OAuth strategies
function setupStrategy(provider, Strategy, config, callback) {
  passport.use(new Strategy({
    clientID: config.clientID,
    clientSecret: config.clientSecret,
    callbackURL: config.callbackURL,
    passReqToCallback: true
  }, callback));
}

// Common OAuth callback logic for all providers
async function oauthCallback(req, accessToken, refreshToken, profile, done) {
  try {
    if (req.user) {
      // If the user is already logged in, link the account
      const existingAccount = await User.findOne({ [`${profile.provider}.id`]: profile.id });
      if (existingAccount) {
        return done(new Error('There is already an account linked with this provider.'));
      }
      req.user[profile.provider] = { id: profile.id, accessToken, refreshToken };
      await req.user.save();
      return done(null, req.user);
    } else {
      // User is not logged in, so it's a new sign-in
      let user = await User.findOne({ [`${profile.provider}.id`]: profile.id });
      if (!user) {
        // Create a new user if no match is found
        user = new User();
        user[profile.provider] = { id: profile.id, accessToken, refreshToken };
        user.name = profile.displayName;
        await user.save();
      }
      return done(null, user);
    }
  } catch (err) {
    return done(err);
  }
}

// Central configuration for multiple OAuth providers
const oauthProviders = {
  google: {
    Strategy: require('passport-google-oauth20').Strategy,
    config: {
      clientID: process.env.GOOGLE_CLIENT_ID,
      clientSecret: process.env.GOOGLE_CLIENT_SECRET,
      callbackURL: '/auth/google/callback'
    }
  },
  facebook: {
    Strategy: require('passport-facebook').Strategy,
    config: {
      clientID: process.env.FACEBOOK_CLIENT_ID,
      clientSecret: process.env.FACEBOOK_CLIENT_SECRET,
      callbackURL: '/auth/facebook/callback'
    }
  },
  // Add more providers here
};

// Setup each strategy
Object.keys(oauthProviders).forEach(provider => {
  const { Strategy, config } = oauthProviders[provider];
  setupStrategy(provider, Strategy, config, oauthCallback);
});

passport.serializeUser((user, done) => {
  done(null, user.id);
});

passport.deserializeUser(async (id, done) => {
  const user = await User.findById(id);
  done(null, user);
});

module.exports = passport; 

routes/auth.js

const express = require('express');
const passport = require('passport');
const router = express.Router();

// Google OAuth authentication routes
router.get('/auth/google', passport.authenticate('google', { scope: ['profile', 'email'] }));

router.get('/auth/google/callback',
  passport.authenticate('google', { failureRedirect: '/login' }),
  (req, res) => {
    res.redirect('/');
  }
);

// Facebook OAuth authentication routes
router.get('/auth/facebook', passport.authenticate('facebook', { scope: ['email'] }));

router.get('/auth/facebook/callback',
  passport.authenticate('facebook', { failureRedirect: '/login' }),
  (req, res) => {
    res.redirect('/');
  }
);

// You can add more providers similarly
// ...

module.exports = router;

User Model Update

const mongoose = require('mongoose');

const userSchema = new mongoose.Schema({
  name: String,
  google: {
    id: String,
    accessToken: String,
    refreshToken: String
  },
  facebook: {
    id: String,
    accessToken: String,
    refreshToken: String
  },
  // Add more provider fields here (e.g., Twitter, GitHub)
});

module.exports = mongoose.model('User', userSchema);

Environment Variables

GOOGLE_CLIENT_ID=your-google-client-id
GOOGLE_CLIENT_SECRET=your-google-client-secret
FACEBOOK_CLIENT_ID=your-facebook-client-id
FACEBOOK_CLIENT_SECRET=your-facebook-client-secret

@krrish-sehgal
Copy link

I would like to work on this , Please assign me this issue under hacktoberfest 2024.
Thanks.

@mathangpeddi
Copy link

I have raised a PR for this - #1310

Can you please check this and give me some inputs?

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

8 participants
@YasharF @mathangpeddi @bhavuk2002 @Chirag77302 @ATHARVA262005 @Mano3009 @krrish-sehgal and others