upash icon indicating copy to clipboard operation
upash copied to clipboard

API Improvment

Open simonepri opened this issue 5 years ago • 5 comments

Description

Expose a constructor to allow the users to have multiple instance of upash instead of having it as a singleton.

Examples

const UPASH = require('upash');

// Create an instance of upash providing the algorithms of your choice.
const upash = new UPASH({
  argon2: require('@phc/argon2'),
  pbkdf2: require('@phc/pbkdf2')
}, {default: 'argon2'});

// You can explicitly tell upash which algorithm to use.
const hashstr_pbkdf2 = await upash.use('pbkdf2').hash('password');
// => "$pbkdf2-sha512$i=10000$O484sW7giRw+nt5WVnp15w$jEUMVZ9adB+63ko/8Dr9oB1jWdndpVVQ65xRlT+tA1GTKcJ7BWlTjdaiILzZAhIPEtgTImKvbgnu8TS/ZrjKgA"

// If you don't do so it will automatically use the default one.
const hashstr_argon2 = await upash.hash('password');
// => "$argon2i$v=19$m=4096,t=3,p=1$mTFYKhlcxmjS/v6Y8aEd5g$IKGY+vj0MdezVEKHQ9bvjpROoR5HPun5/AUCjQrHSIs"

// When you verify upash will automatically choose the algorithm to use based
// on the identifier contained in the hash string.
const match_pbkdf2 = await upash.verify(hashstr_pbkdf2, 'password');
// => true

// This will allow you to easily migrate from an algorithm to another.
const match_argon2 = await upash.verify(hashstr_argon2, 'password');
// => true

Notes

Probably it makes sense to remove install and uninstall methods and add a getDefault method. This would lead to a breaking change.

cc @mcollina

simonepri avatar Aug 06 '18 14:08 simonepri

Good work! I would actually go ahead and remove install and uninstall.

mcollina avatar Aug 06 '18 14:08 mcollina

I'm happy help :) Will open a PR in the next few days. I have a few questions first though.

Is this finialised?

const upash = new UPASH({
  argon2: require('@phc/argon2'),
  pbkdf2: require('@phc/pbkdf2')
}, {default: 'argon2'});

Is there ever going to be any other options passed in the second object? If not I would favour this approach:

const upash = new UPASH({
  argon2: require('@phc/argon2'),
  pbkdf2: require('@phc/pbkdf2')
}, 'argon2');

Also what do we want the behaviour to be if no default is specified?

gavinhenderson avatar Aug 06 '18 18:08 gavinhenderson

"I'm happy help :)"

Thank you @gavinhenderson !!!

"Also what do we want the behaviour to be if no default is specified?"

It should definitely throw an error.

"Is this finalised?"

Not sure, I'm open to suggestions.

I was also thinking at something like that:

const upash = new UPASH({
  argon2: {algoritmh: require('@phc/argon2'), default: true}
  pbkdf2: {algoritmh: require('@phc/pbkdf2'), options: {someParamToOverride: 'somevalue'}}
});

But it seems over complicated. I want to make the API as easy as possible

simonepri avatar Aug 06 '18 19:08 simonepri

But it seems over complicated. I want to make the API as easy as possible

I completely agree, people (including me) are turned off to a package at the first sign of a weird API so we want to avoid that as much as possible.

Not sure I like the idea of specifying the default in the algorithm object as this could to lead to people entering multiple defaults which we could handle but I think its unnecessary. I would favour the one you originally suggested:

const upash = new UPASH({
  argon2: require('@phc/argon2'),
  pbkdf2: require('@phc/pbkdf2')
}, {default: 'argon2'});

This allowing for easy expansion of the options object which I like. Maybe would could make it more clear in the usage guide (when we get to that) and do something like:

const options = { default: 'argon2' }

const upash = new UPASH({
  argon2: require('@phc/argon2'),
  pbkdf2: require('@phc/pbkdf2')
}, options);

gavinhenderson avatar Aug 07 '18 07:08 gavinhenderson

@gavinhenderson Yeah I totally agree, I was just throwing ideas.

simonepri avatar Aug 07 '18 10:08 simonepri