switch from static module to factory

[mastilver]

BREAKING CHANGE: The modude exports a factory instead a set of static method. It needs do be instanciate before use.

closes #25

[ericelliott]

@tjconcept Would you like to review this?

[madbence]

:-1: the API should stay more compatible: configure should return a new instance, and module.exports = configure(defaults)

[mastilver]

I got ride of configure current way:

var customInstance = credential.configure({ ... });

this PR way:

var customInstance = credential({ ... });

[tjconcept]

I'm generally not a fan of redefining all functions on each "factoring", but that might be the way to do things if you want to avoid prototypes.

Let me give an example; the hash methods actual definition is: hash( context, password, callback )

We want a simple way to save the user from passing around the context. I can think of two ways: prototypes and scoping (binding). Prototypes are simply a language feature that acknowledges this problem as a very frequent one and aims to solve it in a uniform way (context becomes this). This has the added benefit of you being able to validate something like (new Something()).myMethod === (new Something()).myMethod) - you know that only the context differs (also very handy for promisification). When using a factory, for all you know, the factory could be handing out entirely different functions with different behaviour on each run. In the ideal world this shouldn't be problem though as the API is documented and followed.

Anyway, would it be an idea to define each function as completely unrelated in the topmost scope like so:

hash(hashMethod, keyLength, work, password, callback)

and then in the factory return wrapper functions like:

function(){
  return hash(opts.hashMethod, opts.keyLength, opts.work, password, callback);
}

It has the nice trait of separating API boundary (input validation, optional arguments/defaults) from the logical parts.

I've done something similarly in my fork to support promises: https://github.com/srcagency/credentials/blob/master/src/index.js

[mastilver]

those functions: pbkdf2, createSalt, iterations, parseHash, verify should be out of the factory I will apply your wrapper idea for: expired and toHash

[mastilver]

btw, that's why I started working on this feature, I wanted credential to use Promise

[tjconcept]

btw, that's why I started working on this feature, I wanted to use credential using Promise

Well, I don't mean to advertise, but you will probably have a more smooth experience just using my fork: https://www.npmjs.com/package/credentials - I will (as far as possible) keep these two projects in parity anyway.

[mastilver]

Cool I initially wanted to use pify, I will have a look at your implementation

[tjconcept]

I still think this PR is the right way to go for this codebase though - with the changes we've already agreed on.

[mastilver]

:+1: I will make the change tonight

[tjconcept]

@madbence I don't agree that configure should return new instances. That would eliminate it's purpose and make it redundant to the factory function (the one exported).

The sole purpose of configure is to be able to change the settings of a already-handed-out instance.

It's probably a very rare use case though (someone increasing the work in a long-running process/service?) - so if any change, I would remove it.

[tjconcept]

@mastilver I didn't consider this at first, but I think we would like to keep the configure method for now to cause least possible change (although it's still a major).

I actually think it's a decent thing to have in the API. The biggest flaw is that it's not immediately obvious whether it resets to defaults or not (it does): configure({ bad: true }) will reset the work option.

[madbence]

@tjconcept i'd remove the factory function completely, and configure would work like defaults in the request module (return a preconfigured instance)

[mastilver]

Just to be clear, configure should:

• reset to the default value (not the one the user passes to the factory)
• return the current absence

[tjconcept]

@madbence I guess "replace the factory function with configure" is the same as "remove the configure function" since the factory function in this PR works like you describe?

I'm just saying; let's keep the configure function on the returned object (whether you call the factory function another configure, I don't care) to make the impact of this PR smaller.

[madbence]

import credential from 'credential';

credential.hash(...); // work with default work params
credential.configure(...).hash(...); // work with custom work params (configure does not modify the options in the current instance, but returns a new instance)

Ok, i've just read the referenced issue (#25), it now makes sense (instance safety), so ignore my comments :smile_cat:. btw i'm not sure why we need instance safety (what are the security risks in the current implementation?), can you elaborate?

[mastilver]

@madbence I kind of like the idea I would still use the factory but internally

@tjconcept What do you think:

• current api with a configure function that return a new instance (@madbence idea)
• factory api (this PR)

[tjconcept]

I really don't like global-anything. Imagine two unrelated modules using this internally and one is setting the work factor to 0.001 - now the other could have a security bug. It is probably rare in practice, but it gives some very hard-to-debug scenarios, and I could see myself running into this issue in testing (were I spawn multiple instances of my services). @ericelliott care to chip in on why instance safety is sought after?

I'm not a fan of having configure return a new instance. I find it unintuitive as you can always just use the exported factory method for that (given var credential = require('credential) why type pw.configure(opts) vs. credential(opts)? You'd expect the first to have some relation with pw).

[mastilver]

I think instance safety would prevent this (unlikely) case When instance1 and instance2 are the credential module

• instance1 hash password1
• instance2 change work
• instance1 verify password1 --> the password is wrong

[tjconcept]

Exactly, that's why we want instance safety and not a credential.hash function that uses some global options :-)

[tjconcept]

If we were to have a credential.hash function, I would suggest it always used the defaults, but I really don't see the point. Is it to avoid the two extra characters (())?

[madbence]

Options should be immutable (eg. not accessible from the outside), to me the ideal implementation of credential would look like this:

function configure(opts) {
  return {
    hash: ...
    verify: ...
    configure: configure
  };
}

module.exports = configure(defaults);

// usage:

var credential = require('credential');
credential.hash(...);
credential.configure(...).hash(...);

or maybe even simpler API:

function create(opts) {
  function configure(opts) {
    return create(opts);
  }
  configure.hash = ...
  configure.verify = ...
  return configure;
}

module.exports = create(defaults);

// usage:

var credential = require('credential');
credential.hash(...);
credential(...).hash(...);

since opts are not accesible from the outside, they are safe :wink:. or am i missing something?

[mastilver]

@madbence this is how I was seeing it (the first one), so yeah lgtm ;)

[tjconcept]

I actually think we're pretty aligned then.

I agree with the last one, but not the first. If the first example should make sense, it should "inherit" the options from current before applying new ones, e.g.: configure({work:2}).configure({hashMethod:'some'}) would give you an instance with work of 2 and hash method of "some".

But then again; that further work should be in another PR as it is really not necessary to bake into this at first.

[mastilver]

@tjconcept cool, thank you for the input

[mastilver]

@tjconcept One last think, how should options be:

• directly on the instance (as the current state of this PR)
• into an options property
• non-accessible / private (The one I would prefer)

[tjconcept]

Directly on the instance.

I agree with the preference, but we need to keep the changes focused. Feel free to create an issue for "privatizing" the options.

[ericelliott]

Wow.

This is a lot of conversation for something very simple.

credential should export a factory -- meaning a function that returns an object. That function should have no methods attached to it, so:

import credential from 'credential';

credential.hash() // error -- hash is not a function.

import credential from 'credential';

pw = credential(options);
pw2 = credential(differentOptions);
pw.hash() // is defined
pw2.hash() // not affected by the options for `pw`.

[tjconcept]

I think that's were we arrived as well :)

[ericelliott]

Couple more clarifications:

• Options: Non-accessible / private
• .configure(): No longer needed.

Because we're exporting a factory that takes options, we no longer need a .configure() method. This is a major (breaking) change. Feel free to break things to make them better. We'll do a major version bump when we publish these changes.

[mastilver]

@ericelliott awesome, I starting it right now

[ericelliott]

:+1:

[mastilver]

There it is! It took some time, because of a timing issue with constantTimeCompare

[ericelliott]

Can you get the checks passing?

[mastilver]

done!

[ericelliott]

Hurray!

[tjconcept]

Nice job on the source @mastilver Aren't we missing some docs work? Does this affect the CLI?

[mastilver]

@tjconcept yep I didn't do the docs :-1: CLI???? There is none..., is there?

[ericelliott]

Please update the readme. When that's done, we'll publish the new version. =)

[mastilver]

@tjconcept just found out about the cli, I will check it out :blush: @ericelliott doing it