Discussing enhancements and changes for V3 rewrite

[eddyystop]

V1 used callbacks. V2 used promises and introduced support for mobile. V3 will use async/await, plus enhancemenst we can discuss here.

My initial goals are:

• Convert to async/await to simplify the code.
• Make encripting tokens optional, as they are such a pain during testing.
• Allow params to be customized on calls to the user-entity service.
• Provide a template for each type of potential notification.
• Integrate into @feathers-plus/cli.

Other candidates:

• 2FA.
• Rate limiting.

Yes comment on these and add your suggestions.

[eddyystop]

reserved

[eddyystop]

reserved

[claustres]

We have built a modular framework on top of Feathers for our own applications and used feathers-authentication-management module as well. We mainly perform integration testing and added some "extension" to it in our own module kNotify, which manages all kind of notifications (emails, push), so we'd like to share some ideas here.

First, IMHO I do not think encryption should be optional, just like storing password in plain text should not be an option, Feathers should enforce production grade policies by default. What makes it successful is that you get all the boring things like security out-of-the-box. In tests one can always add a "fake" notifier and catch unencrypted tokens like this. If you'd like to go further you could retrieve information like tokens directly from a test user gmail inbox, you can have a look to https://github.com/kalisio/kNotify/blob/master/test/account.test.js, this is more complex but possible.

We worked on email templating. This can be a good enhancement for this module but i18n should be taken into account. We found email-templates to be a great module, we simply attached the locale of the user upon account creation to him in order to manage i18n.

We also worked on invitation requests. This might also be a good enhancement for this module, although there is different ways to tackle it. Most online services allow a user to invite another to join, the invited user can be created when the invitation is sent or only on first login. We used the first approach, creating the user and generating a password we send him by email. In this case you have to manage cleanup of inactive accounts because the invited users could never connect, and we have to take care to GDPR. The second option can be simpler to handle but it does not allow the invited user to be used in the app as if he was already registered (think about a project management app and you'd like to add the invited member in a team, task or whatever). However, it could be really simple to implement like any other operation in this module by adding an "invitation token". Because it has to be stored somewhere I guess the user has to be created anyway.

[eddyystop]

Would you be able to provide the email formats/texts you use for the different types of emails the repo can send?

What do you think of supporting 2FA?

[claustres]

Our production templates can be found here. What can be tricky is that you might want to insert a lot of things in the email template and subject like the app name, a response email, etc. As a consequence we should provide developers with a way to inject this context in the module.

With short tokens there is already the basics for 2FA but something more polished would be really useful out-of-the-box: a way to automatically send sms/email if the last connection was too old or on a new device, a way to block authentication until the token has been validated, etc. However you start touching really specific things, in our own module we use AWS SNS and Apache cordova to do so.

[beeplin]

@eddyystop I strongly agree with @claustres that invitation requests could be a major hit point in the v3. While personally I doubt the popularity of 2FA coz I have tried to use it in my projects but my co-workers and users got frustrated ;_)

encrypting passsword should be the only option in dev/prod mode, but we can provide a switch option to allow disabling encrypting when NODE_ENV is 'test'.

[claustres]

About rate limiting I think this is usually done at app level because it really depends on your needs and can be managed simply and generically in Feathers (e.g. https://blog.feathersjs.com/feathersjs-in-production-password-policy-and-rate-limiting-32c9874dc563). Providing something specific to this module does not really make sense IMHO, this could be a more generic feathers-limiter module with relevent application mixins and hooks. Moreover depending on the environment you might want to disable it or not (e.g. to perform benchmarking), so this should not be "forced" out-of-the-box.

[eddyystop]

@claustres , @beeplin , et al.: I'd like your opinions on a database issue. The represent repo expects a record format that is MongoDB compatible. SQL DBs don't support booleans nor arrays. Devs were expected to reformat the records on mutation by the repo.

I expect they did this using hooks. I don't think this is elegant, and I feel it increases the cognitive load for using the repo.

I would like to make the new version friendlier to SQL DBs, especially since I want @feathers-plus/cli to optionally add this repo to generated apps.

Questions:

1. Do we want the record layout to be backward compatible, or can we expect developers to migrate their users records?
2. Do we want the same users record format in both MongoDB-style and SQL DBs? This would make things easier for this repo but neither set of developers may be happy. I'm considering feathers-hooks-common standardizing with https://github.com/feathers-plus/feathers-hooks-common/tree/master/lib . However MongoDB devs won't be happy with stringified arrays, and SQL devs may not be happy with the date format.
3. Do we want a new option on initialization which indicates which style is used? The repo would convert internally.
4. We could within the repo use a MongoDB record format, and have an option with allows the user to convert between this internal MongoDB format and the persisted format.

[eddyystop]

I would like to highlight a limitation with the present hook design which may affect us. It involves hooks making internal calls on another service.

First the before and after hooks run when such a call is made and they may modify the record returned. This is a real problem for hooks like cache, which I would like to make work with this repo's rewrite.

Second, setting context.result is almost futile because the remaining hooks are still run. Few hooks, certainly not the ones in feathers-hooks-common, no-op if they see context.result is set.

Third, returning SKIP is problematic in before hooks because the after hooks are still run.

In summary, there is no elegant way I know of to read or mutate a raw record. This is fatal for the development of many more advanced hooks, and it may impact us.

[eddyystop]

@claustres , @beeplin : I would like the Feathers authentication packages to check flags set by this repo, e.g. isVerified. I think its a source of problems expecting the dev to add checking hooks themselves.

There are also the addVerification and removeVerification hooks. I don't see a way of eliminating their use without making matters worse.

What do you think?

[claustres]

About the DB issues I am really opinionated. Just like Feathers does, this repo should use JSON as internal format, it should have no link with any adapter. Even the Feathers MongoDB adapter does not automatically convert ObjectID or date string because the data model is really at the core of any app and should be flexible. We could provide some helpers (eg hooks) to convert if we'd like but no more. By the way on the current version I can't see what is really MongoDB specific.

I don't really get your remark about hooks. According to current Feathers design hooks are part of services, as such you can actually see an external service call as hooks + internal service call. I can't see why this would break cache, a cache has no knowledge of the internals and maps an input to an output, viewing hooks + service call as a black box. If someone add a hook that modify the data model of the result on a given service then he breaks the interface of the service. Just like changing the signature of a function this should not be done unless you know what you are doing but then be ready to accept some issues ;-) IMHO setting context.result of using skip should only be relevant for performances but you should never skip internal hooks of a service intentionally because they might be mandatory for the good functioning of the app.

About checking flags do you want to move verification from this module to the authentication module or to enforce this flags everywhere ?

[eddyystop]

Most of the "it doesn't work" questions I get concern the more complex common hooks and are because of interactions between hooks. Users don't understand the ramifications, and the only suggestion I could really document is using (assuming the hook sets a param prop)

before: [ softDelete(), iff( ctx => !ctx.params.something, ... theRemainingHooks)]

The infrastructure should help out when the pain becomes too much.

[eddyystop]

I was thinking of authentication checking the 'isVerified` flag. I agree its conflating the 2 modules together.

[beeplin]

There are no array types used in current db fields, only boolean. If it is still the case in the new version, we may just provide a helper hook to help sql users to convert boolean to integer. The cli can automatically add the hook for sql db, withouy touching any logic within this repo.

The cli can also help people add hooks to the authentication service to check isVerified, add vertification, etc. That is what a smart cli tool means, I think.

[beeplin]

As for the problem of service-hook infrastructure... I suffered from that too :(((

I don't see a way that can simply avoid all the problems, (even the new koa-like hook proposal would still have this issue,) as long as we allow people to freely wrap all kinds of hooks around the core service calls.

To alleviate the pain, we could wrap multiple services with different hooks. Just as @claustres said above, service call and the hooks should be seen as a whole, so a service with hook A&B should be seen as a different service from the service with only hook A. So we can make them two services and give different names to them.

[eddyystop]

1. beeplin, in general the cli cannot be certain of placing hooks in the right place in the hook array because of user added hooks, because of conditional hooks. Placing a hook at the front of the hook array is easier, though you could have several hooks which should be first.

When the user customizes the hook order, the cli has little idea of what to do.

2. claustres' suggestion about hooks+service groupings bears thinking about.

3. At a more basic level, one of the hook infrastructure problems I run into is that you cannot set context.result and return SKIP in the same hook. Its one or the other.

Its an ugly proposal, but I'm thinking perhaps we should have a hook like to

module.exports = function checkIfSkipHooks() {
  return context => context.params.__SKIP_BEFORE ? SKIP : context;
};

and place it after hooks which could set context.result and this flag, e.g. cache. A more specialized skipHooksIfResultSet would be similar.

If we also use __SKIP_AFTER, then a before hook can skip all remaining hooks.

[claustres]

I think this discussion is also related to https://github.com/feathersjs/feathers/issues/816#issuecomment-363920534. This highlights how it is difficult for a CLI to produce relevant results except in really simple/demo apps. Maybe it is just me but the problem which is always raising is that you use a tutorial, CLI, generator, whatever to get a simple working app and you think "wooo it's great" ! Then you try to build a "real-world" app from here and you end thinking that it would have probably be better or faster to start without the CLI because everything that has been generated has to be thrown away. Not to say that beginners and so on ask why the hell the generated code breaks when adding real-stuffs inside because they don't really understand it does not cover all use cases...

One should probably first create an app from scratch before using the CLI not to look too much stupid ;-)

[eddyystop]

Your comment caused me to flashback to my first small Angular app and shiver. :(

I agree with much of what you say. That's why the Auk Basic Guide I wrote (now dropped) started with hand coding Feathers. I thought people ought to really understand what was being generated.

Its tough to solve a problem completely. For example, the populate hook grew and grew, an option added for almost every snippet possible in a find call. There was no end in sight. So one of the advantages I see in fastJoin is that users can code whatever find call they want.

I've slowly become happier with imperative interfaces (like fastJoin) rather than declarative ones (populate). Handle the boilerplate for the user and let them give code for the details. Its not perfect but it can handle more cases.

[beeplin]

The primary purpose of a cli tool should be helping people initialize their projects (with/without auth, with/without auth-local-management, etc). Technically I don't think a cli can do all the modifications for our users in the middle of their development. If the user is adding something new to an existing project with the cli tool, the best thing the cli can do might be adding the new codes to the most-possible-place and then prompting users to check and change them if needed.

So for the auth-local-management plugin, the cli could just add the hook on the top of the authentication service, and then tell people to check and relocate it.

[beeplin]

I mean for an existing project, the function of a cli is to type (some repeated boilerplates) for people, not think for people.

When I use the old feathers-cli to add a hook to a service, I just choose the service, the hook type (before/after/error), etc. and then go to the service hook file to move the injected line to the place I wanted. I doubt if a cli tool can be smarter than that.

In case people don't understand the importance of the hook position/order, we can inject a todo comment togeter with the code, something like // TODO: The following line is auto generated by cli tool. Please relocate it to the proper place

[eddyystop]

f+/cli has generate hook and generate test. They were being tested and I'm documenting them now.

generate hook does nor ask for before/after or all/find/... . It just adds a require/import to services/name/name.hooks.js. I did feel right about adding a hook in what is likely the wrong place in the hook chain.

Do you all think that is wrong?

[eddyystop]

@claustres, I was thinking about your comment that a service+hooks should be considered an entity and that multiple services, each with different hooks, could be created for the same table.

The service+hooks idea is clean but creating multiple services feels perhaps over reach. The same concept could be implemented now with a new hook.

const subServices = {
  name1: {
    before: {
      all: [ ... ],
      find: [ ...],
      ...
    },
    after: {
      all: [ ... ],
      find: [ ... ],
      ...
    }
  },
  name2: {
    before: { ... },
    after: { ... },
  },
};

module.exports = {
  before: caseOf(context=> context.params.$subservice, subServices),
  after: caseOf(context=> context.params.$subservice, subServices),
};

// service.find({ query: { ... }, $subservice: 'name1' });

I also paid attention to the issue you refer to, the one suggesting a hook tree be used rather than a hook array. I thought the branching nodes could be implemented with a case syntax, the same code as above, as it be used recursively.

I haven't done anything about it as I've had no one to discuss this with.

[eddyystop]

Rewrite started.

[eddyystop]

Status report:

1. feathers-plus/feathers-authentication management (f-a-m) has been rewritten using async/await as feathers-plus/authentication-local-management (a-l-m). No npm package has been published.
2. The interfaces can be called using await or using Promises.
3. Converting the tests took over 75% of the time. They now use async/await and Feathers instances. They no longer use stubs for Feathers.
4. I expect what's there now should work as a drop-in replacement. If you want to help, drop it in into a copy of an existing app and test.
5. DO NOT use a-l-m except for short-term testing. I feel no obligation to maintain compatibility as we move forward.

Next steps:

1. Address outstanding issues from f-a-m.
2. Add a-l-m to an app generated by cli+.
3. Expand a-l-m with templates, emailer, maybe an SMS push.
4. We talk about other enhancements.

[eddyystop]

Moved to https://github.com/feathers-plus/authentication-local-management/issues/1