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

API Improvment #9

Open
simonepri opened this issue Aug 6, 2018 · 5 comments
Open

API Improvment #9

simonepri opened this issue Aug 6, 2018 · 5 comments

Comments

@simonepri
Copy link
Owner

simonepri commented Aug 6, 2018

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

@mcollina
Copy link

mcollina commented Aug 6, 2018

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

@gavinhenderson
Copy link

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?

@simonepri
Copy link
Owner Author

simonepri commented Aug 6, 2018

"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

@gavinhenderson
Copy link

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);

@simonepri
Copy link
Owner Author

simonepri commented Aug 7, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants