feathersjs-ecosystem / feathers-authentication-management

Adds sign up verification, forgotten password reset, and other capabilities to local feathers-authentication
https://feathers-a-m.netlify.app/
MIT License
247 stars 98 forks source link

Password hash on reset differs from account creation #92

Closed richard-edwards closed 6 years ago

richard-edwards commented 6 years ago

Steps to reproduce

  1. Create a new user, pick random password, and note hashed password.
  2. Verify email for that user and login (everything good here)
  3. Initiate reset password with token
  4. Change password using token and new password (try password123) and note hashed password
  5. Login to account with new password (password123) and user cannot be logged in
  6. Create a new user, with pasword (password123) and note hashed password
  7. Note that hashed password from step 6 doesn't equal the hashed password from the reset in step 4
  8. Copy hashed password from step 6 into the user record from step 1-4 and try login. User can now login.

Expected behavior

After a password reset, user should be able to log in with the new password and the hash should be the same as a good known password hash for the same password

Actual behavior

After a password reset, user cannot log into account with valid password

System configuration

Windows 10, Node 8.9.4

"feathers-authentication-hooks": "^0.1.2",
"feathers-authentication-management": "^2.0.0",
"@feathersjs/authentication": "^2.1.1",
"@feathersjs/authentication-jwt": "^2.0.0",
"@feathersjs/authentication-local": "^1.1.0",
eddyystop commented 6 years ago

Do you have a hashPassword hook on patch after? If so, do you condition it so it doesn't rehash the password written by the repo, which password is already hashed.

eddyystop commented 6 years ago

Consider this:


  before: {
    all: [],
    find: [ authenticate('jwt') ],
    get: [ authenticate('jwt') ],
    create: [
      hashPassword(),
      verifyHooks.addVerification()
    ],
    update: [
      commonHooks.disallow('external')
    ],
    patch: [
      commonHooks.iff(
        commonHooks.isProvider('external'),
          commonHooks.preventChanges(
            'email',
            'isVerified',
            'verifyToken',
            'verifyShortToken',
            'verifyExpires',
            'verifyChanges',
            'resetToken',
            'resetShortToken',
            'resetExpires'
          ),
          hashPassword(),
          authenticate('jwt')
        )
    ],
    remove: [ authenticate('jwt') ]
  },```
richard-edwards commented 6 years ago

Ok, I think that was probably it. I was hashing password every time on patch. This is my user.hooks.js file, I'd really appreciate a quick review to see if this all makes sense. I find it hard to get my head around what I really need in here.

const { authenticate } = require("@feathersjs/authentication").hooks;
const commonHooks = require("feathers-hooks-common");
const { restrictToOwner } = require("feathers-authentication-hooks");
const {
  addVerification,
  removeVerification
} = require("feathers-authentication-management").hooks;
const {
  hashPassword,
  protect
} = require("@feathersjs/authentication-local").hooks;

const restrictToAuthenticatedOwners = [
  authenticate("jwt"),
  restrictToOwner({
    idField: "_id",
    ownerField: "_id"
  })
];

const preventAuthManagementChanges = commonHooks.iff(
  commonHooks.isProvider("external"),
  commonHooks.preventChanges(
    "email",
    "isVerified",
    "verifyToken",
    "verifyShortToken",
    "verifyExpires",
    "verifyChanges",
    "resetToken",
    "resetShortToken",
    "resetExpires"
  ),
  hashPassword(),
  authenticate("jwt")
);

const sendEmailVerification = require("../../hooks/send-verification-email");

module.exports = {
  before: {
    all: [],
    find: [authenticate("jwt")],
    get: [...restrictToAuthenticatedOwners],
    create: [
      hashPassword(),
      addVerification() // adds .isVerified, .verifyExpires, .verifyToken, .verifyChanges
    ],
    update: [preventAuthManagementChanges, ...restrictToAuthenticatedOwners],
    patch: [preventAuthManagementChanges, ...restrictToAuthenticatedOwners],
    remove: [...restrictToAuthenticatedOwners]
  },

  after: {
    all: [
      commonHooks.when(
        hook => hook.params.provider, // protect for everything except internal (undefined)
        protect("password")
      )
    ],
    find: [],
    get: [],
    create: [
      protect("password"),
      sendEmailVerification(),
      removeVerification() // removes verification/reset fields other than .isVerified
    ],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};
eddyystop commented 6 years ago

This may be useful https://hackernoon.com/setting-up-email-verification-in-feathersjs-ce764907e4f2