Skip to content

Password hash on reset differs from account creation #92

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

Closed
richard-edwards opened this issue Feb 19, 2018 · 4 comments
Closed

Password hash on reset differs from account creation #92

richard-edwards opened this issue Feb 19, 2018 · 4 comments

Comments

@richard-edwards
Copy link

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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