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

changed userRecommendations api to public #252

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

helloukey
Copy link
Collaborator

Change-Id: I6965296e6f9281a8516ecbe7449d71ca9c406b65

Copy link

vercel bot commented Jan 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
openmentorship ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 2, 2024 7:29pm

@helloukey
Copy link
Collaborator Author

helloukey commented Jan 1, 2024

This pull request is part of a stack:

  1. changed userRecommendations api to public (#252)
  2. changed searchMentors api to public (#253)
  3. removed /explore route from nextAuth list (#254)
  4. updated userConfirmation state and modal for join mentorship program (#255)
  5. returned error for userType mentor (#256)
  6. fixed layout shift for explore page mentors listing (#257)

Change-Id: I6965296e6f9281a8516ecbe7449d71ca9c406b65
@helloukey helloukey force-pushed the mergify_cli/GEN-190/feature/public_mentor_listing/I6965296e6f9281a8516ecbe7449d71ca9c406b65 branch from ec845ae to d41a190 Compare January 2, 2024 19:27
Copy link

Copy link

);
router.get('/userRecommendations', (req, res, next) => {
passport.authenticate('jwt', { session: false }, (err, user) => {
if (err || !user) return next();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have return next() if no user and just calling next() when there is a user, shouldn't it be the same.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we don't need to check for a user and assign it to req.user. That it already done when we do passport.authenticate(). Thats why in lot of the controller methods, we are already using req.user.

Basically, i think you can keep the code as it was before and just remove the util.checkRole.... line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikhil-g777,
passport.authenticate() is doing the JWT check. So, if it cannot find the user, it throws the default error 'unauthorized' and due to this default behavior, We cannot do further functionalities.
So, basically what we are doing is:

  1. Check if a user is available, if not available then normally move to the next middleware.
  2. If the user is available and is a userType mentee then add the user to the req object and move to the next middleware.
  3. If the user is available and is a userType mentor then return the error 'unauthorized'.

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

Successfully merging this pull request may close these issues.

2 participants