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

Comments

Projects
None yet
3 participants
@simonepri
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

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Aug 6, 2018

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

mcollina commented Aug 6, 2018

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

@gavinhenderson

This comment has been minimized.

Show comment
Hide comment
@gavinhenderson

gavinhenderson Aug 6, 2018

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 commented Aug 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@simonepri

simonepri Aug 6, 2018

Owner

"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

Owner

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

This comment has been minimized.

Show comment
Hide comment
@gavinhenderson

gavinhenderson Aug 7, 2018

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 commented Aug 7, 2018

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

This comment has been minimized.

Show comment
Hide comment
@simonepri

simonepri Aug 7, 2018

Owner

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

Owner

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