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

Update docs #138

Closed
claustres opened this issue Mar 3, 2020 · 8 comments
Closed

Update docs #138

claustres opened this issue Mar 3, 2020 · 8 comments
Labels

Comments

@claustres
Copy link
Collaborator

Docs should probably be copied to https://github.com/feathers-plus/docs

@OnnoGabriel
Copy link
Contributor

We should also update/rewrite the docs:

  • Include the step-by-step guide by Imre from Hackernoon. Or at least show a minimal working example.
  • Details on usage with Feathers v3 and v4.
  • Usage with TypeScript.

I am willing to put some effort in this task.

However, there are resonable plans to rewrite this package (TS support, REST-like API), and I get more and more the feeling that we maybe should restart/fork this package to get rid of old ballast.

There is a high demand for a simple e-mail-verification/password-reset extension for feathers. I am not sure if this is the right place for this discussion, but somehow we should solve the question how much effort we put into this package. Or if there are better solutions. @daffl mentioned on Slack that he would be willing to add auth management functionality to authentication-local (#general, Thread by Beau Shaw on Aug 12th, 13:37).

@claustres, @TheSinding and others, would do you think?

@claustres
Copy link
Collaborator Author

claustres commented Aug 24, 2020

On my side I am open to any suggestion to ease sustainability and keep these featuree alive. According to the effort I could put in this goal, trying to take the current code over was the best solution. However, if some maintainers can start a bigger effort to initiate a new fresh module (as a part of authentication-local or independently) it would probably be a great thing, I would be pleased to help if possible.

IMHO we should start documenting upfront using flow diagram(s) like I tried to do here. Indeed, it seems to me that a lot of people are confused when using this module simply because they are confused about the different processes themselves (signup, reset, etc.), and also if they can occur once you are authenticated or not. They are also confused because of implicit security-related issues addressed by the module like hashing tokens in some cases and not in others cases. This kind of design choices should be explained through a rationale in the docs. It seems to me that some difficulties also arise from the fact that this module is only a small piece in a more complete workflow, notably because it has to rely on third-party services to manage user communication channel like SMS or email sending which are not provided "out-of-the-box" but are to be plugged using a dedicated API (aka notifier). Last but not least, the current module has a lot of configuration options about the way tokens are generated, etc. All of this is probably too much and lacks flexibility, it should be more easily "composed" by the user using "standard" hooks or using a "plugin" system and not be part of the module.

So if @daffl is ok to add some of the features to authentication-local it can probably address some of the issues. However I would like to highlight that although most users perceive features like password reset or token generation as "easy to do" because most web apps provide it out-of-the-box today, doing it right is pretty hard regarding security concerns and required flexibility. That's probably why for instance the authentication part of feathers has been rewritten between v3 and v4.

@TheSinding
Copy link
Collaborator

That is some good thoughts @OnnoGabriel.

I agree with @claustres, that we need to make it clearer on how to use the module, through the documentation.
And if I understand your comment correctly @claustres, then I agree that we should also make the module more concise and it should be better at providing some flexibility for the developer.

I'm wondering if this module were merged into the authentication-local, would there be any need for this repo to exist then ?

@OnnoGabriel
Copy link
Contributor

I am pretty sure that @daffl would implement only a minimal auth management functionality, for sure not everything this package provides now. And he will probably need some help.

So, this module would exists as long as there is no better alternative :-) I am pretty sure, that we cannot change much in the current module, because it would break current implementations. For example, the REST-wrapper is a great idea, but it would also make this module more complex as it is already now, just to keep the original interface.

Rewriting the docs could help users to implement the current f-a-m module, I agree here with @claustres. But next to the docs we could also write some specs to identify a general auth-management scheme and the required minimal functionality – kind of a pre-work for either for a new, more simple module or for an extension of authentication-local.

This is similar to what @eddyystop started with the a-l-m module. However, that was also an example how complex this task is. Therefore, I hesitate to implement too much and recommend to start with a general, flexible scheme and a very basic functionality.

@OnnoGabriel
Copy link
Contributor

...and, yes, security is an important issue here. This is no easy task and requires a lot of experience, I agree.

@claustres
Copy link
Collaborator Author

The minimum viable module can be hard to find, for instance if we extract the notifier and the token generation from the existing module some operations will reduce to simply patch some properties computed externally on a user, simply put nothing that would require a dedicated module. The most interesting way to go seems to me to mimic feathers authentication which defines an abstract authentication strategy interface that can be easily extended but also provides a default implementation for eg token generation. This seems to me harder for notification management, for this we should rely on either events to be catch or hooks. I also see some nice additional features like user invitation management and maybe user access revocation.

@TheSinding
Copy link
Collaborator

TheSinding commented Aug 24, 2020

@claustres - That's not a bad idea. This would also make it more aligned with the feathers project, making it easier to integrate if that's the route we want to follow.
I like the additional features you're think of - If the more simplified module is integrated into the authentication-local module, I think the way of handling the notification management and etc with events would provide a great "out-of-the-box" experience, but wouldn't be as flexible, though you could control that through an options object.

IMO I think the basic local management should be baked into the authentication-local module, and this project (or another) could be some sort of icing on the cake if you will, with additional features.

@fratzinger
Copy link
Collaborator

Closing this because now we have https://feathers-a-m.netlify.app/ with feathers-authentication-management@4 🎉

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

No branches or pull requests

4 participants