feathersjs-ecosystem / feathers-authentication-management

Adds sign up verification, forgotten password reset, and other capabilities to local feathers-authentication
https://feathers-a-m.netlify.app/
MIT License
247 stars 98 forks source link

v4 support #124

Closed sxwebdev closed 4 years ago

sxwebdev commented 5 years ago

Hello. When will version 4 be supported? The password is currently incorrectly restored.

TheSinding commented 5 years ago

I've got it working with feathers-authentication-local v4. See more here.

Are you interested in me creating a pull request @eddyystop ? The tests are successful, but its not backwards compatible.

octopusOnJellyfish commented 5 years ago

I've got it working with feathers-authentication-local v4. See more here.

Are you interested in me creating a pull request @eddyystop ? The tests are successful, but its not backwards compatible.

Hi there, thanks for compatible version with v4.

I forked your repo and made a npm package at authentication-local-management-v4

OnePunchMan007 commented 5 years ago

Can you make a pull request octopus or Sinding ?

TheSinding commented 5 years ago

Done :)

longseespace commented 5 years ago

Not sure what's going on here but I'm using feathers-authentication-management (v2.0.1) and @feathersjs/authentication + @feathersjs/authentication-local (both v4.3.0) and looks like it's working fine.

Am I missing anything?

OnePunchMan007 commented 5 years ago

well i don't get it : it seems eddyystop rewrite feathers-authentication-management in to authentication-local-management. so i thinks it's better to continue with authentication-local-management?

ivankostic85 commented 5 years ago

@OnePunchMan007 I'm about to start implementing verify email and password reset functionality. I'm using feathers v4. I want to choose the right package, but I can't install authentication-local-management via npm?

OnePunchMan007 commented 5 years ago

From my research it's : eddyystop rewrites feathers-authentication-management in to authentication-local-management but he stops working on it this january 2019 (CF : slack feathers). And longseespace was right feathers-authentication-management (v2.0.1) is working with feathers.js v4.0. I implement it like this and it's working. what octopus and thesinding have done it's to update dependency of feathers-authentication-management (v2.0.1) and they commit it but eddyytop is no longer working on it so i don't know if it will be accepted.

TheSinding commented 5 years ago

@OnePunchMan007 Yeah, that sounds about right

Should I package my version ?

eikaramba commented 5 years ago

yes that would be great. unfortunately eddyystop basically vanished from earth until 6 months around. hopefully he is fine, but there is no sign of him online anywhere

eikaramba commented 5 years ago

btw: something is wrong with the repo from @octopusOnJellyfish https://www.npmjs.com/package/authentication-local-management-v4 i get "verifiedExpires.getTime is not a function" upon calling resendVerifySignup action.

eikaramba commented 5 years ago

Sry for spamming but i just like to summarize my observations (not saying they are true, i admit i had weird problems while testing):

TheSinding commented 5 years ago

@eikaramba I'm very busy at work lately, but I can take a look at it in a couple of weeks. I can also publish my package (ha) if you want

Have you tried reaching him on the #feathersjs Slack ?

OnePunchMan007 commented 4 years ago

Sry for spamming but i just like to summarize my observations (not saying they are true, i admit i had weird problems while testing):

* repo from @octopusOnJellyfish (based on @TheSinding ): resetting password **is working**, resending verification link **not working** (`verifiedExpires.getTime is not a function`)

* repo from @TheSinding : **not published**, build it locally - **working**

* feathers-authentication-management(3.0.0): resetting password **is not working**, resending verification link **is working**

* feathers-authentication-management(2.0.1): **working**

* authentication-local-management (refactored from eddyystop): unfortunately not yet finished it seems

The repo from @octopusOnJellyfish (based on @TheSinding ) is working for me

sechel commented 4 years ago

I would find it the better solution to resume working on this official version here than putting together alternative repos and npm releases. If @eddyystop is not able to continue maintaining this we should find another maintainer who can accept pull requests and release the package to npm. What do you think? Pinging @daffl

TheSinding commented 4 years ago

@sechel I agree, but as I understood it, the alternative repos were a temporary idea

sechel commented 4 years ago

@sechel I agree, but as I understood it, the alternative repos were a temporary idea

It's a good thing for the moment 👍 but in the long run it's harmful for the feathers ecosystem to have many competing packages on npm doing the same thing. It's confusing and makes it hard for people getting started.
From what I understand now, something must have happened to @eddyystop in mid 2019 that made him stop working on this project and all of his others. I hope he is ok but it doesn't look good.

TheSinding commented 4 years ago

It's a good thing for the moment +1 but in the long run it's harmful for the feathers ecosystem to have many competing packages on npm doing the same thing. It's confusing and makes it hard for people getting started. Agreed!

From what I understand now, something must have happened to @eddyystop in mid 2019 that made him stop working on this project and all of his others. I hope he is ok but it doesn't look good.

Let's hope not!

claustres commented 4 years ago

In case nobody has noticed it there was an on-going upgrade guide to move from https://github.com/feathers-plus/feathers-authentication-management to https://github.com/feathers-plus/authentication-local-management. We had some discussions with John Szwaronek (@eddyystop) about new features he would like to add in the module like inviting users, plugin architecture, etc. I hope this can help.

OnnoGabriel commented 4 years ago

@claustres, thanks for the update. However, the "new" project https://github.com/feathers-plus/authentication-local-management is quite old with the last commit 13 months ago (i.e. before Feathers v4). The same for the upgrade guide.

What would really help: is there any (official or at least actively maintained) package similar to feathers-authentication-management or authentication-local-management? A package that extends the basic auth functionality in Feathers with endpoints for e-mail verification, password resend and password reset? Including e-mail notifications? This is still a common work flow for many apps.

It is great that this package still runs with Feathers 4. But for how long without active maintenance? So, I agree with @sechel. The current situation is a bit confusing.

daffl commented 4 years ago

The answer is pretty simple: This package is currently not maintained and looking for a maintainer. It is also up to whichever maintainer to decide which one of the two would be the better one to continue. If someone accepts to take it on, it will be moved to the feathersjs-ecosystem organization. There's clearly a need and it's open source so be wild.

download

OnePunchMan007 commented 4 years ago

i'm a beginner but i love feathers.js and i have read a lot about this package. i would be glad to help (documentation or what ever) the new maintainer

claustres commented 4 years ago

As discussed in slack and due to the fact @eddyystop cannot work anymore on this module a couple of people will try to keep it alive. I've just started looking at current issues/PRs on this one, any help is welcome.

I will not have much time to work on the new module in the near future https://github.com/feathers-plus/authentication-local-management so that the best option is to focus here because, at least, I already know how it works in broad lines. Any volunteer to start looking at the other module ?

OnnoGabriel commented 4 years ago

What is the status of this issue? The current version of this package does not work with Feathers 4. And the pull request by @TheSinding is still pending because of a conflict, as far as I can see.

There is also a fork feathers-authentication-management-v4 by @octopusOnJellyfish based on the work of @TheSinding. I used that version already successfully with Feathers 4. Should that version be integrated back to this original package?

claustres commented 4 years ago

As far as I remember the PR is waiting about some feedback from @TheSinding because some people reported that the module (at least the published version) works as is with Feathers v4.

This PR is probably related to the master version only, but not sure about that as I did not investigate the code in detail.

TheSinding commented 4 years ago

My bad, i totally forgot this.

I can take a look at it, during the next week. (I'm gonna write it down this time 😄)

TheSinding commented 4 years ago

This should be fixed now.

OnnoGabriel commented 4 years ago

Thanks a lot @TheSinding and @claustres. I just tried the new master branch of this package in our Feathers v4 app. Seems to be fine now!

It should be published as npm package, too.

claustres commented 4 years ago

@OnnoGabriel I am currently discussing with the Feathers guys to get access to the NPM if possible. Another option is to move the module to our own organisation https://github.com/kalisio if we take it over and start with a new namespace like @kalisio/feathers-authentication-management. It might be more easy to manage access right instead of always asking someone else. I think we will be set soon, let me know of any feedback on this.

OnnoGabriel commented 4 years ago

@claustres Both options would be fine with me, as long as it facilitates a fast and easy NPM packaging. If you move the project to @kalisio, will there we redirects from the old GitHub location?

claustres commented 4 years ago

It seems to be the case https://help.github.com/en/github/administering-a-repository/transferring-a-repository.

About the NPM packaging there are the required npm scripts already although we will need to test it because the structure has changed (no babel output anymore).

TheSinding commented 4 years ago

I'd argue the best would be to stay in the feathers repos, since the founder of the repo was closely tied to feathers.

claustres commented 4 years ago

According to what Daffl said to me the founder what not really "closely tied" to feathers core team, that's the reason why right management was separated between different people. He proposed to either migrate to our own organisation or possibly to move the module to https://github.com/feathersjs-ecosystem, which is more "closely tied" to feathers core team.

TheSinding commented 4 years ago

Hrm. What would the benefits be of moving it to https://github.com/feathersjs-ecosystem? Better discovery ?

claustres commented 4 years ago

I don't know if it will provide benefits except being probably more "closely tied" to feathers core team. Moving to our kalisio org is basically to have an easier access to the "management" part if we take it over (GitHub, NPM, Travis, etc.) but no more at this point. It also depends on who is able to help maintaining the repo at this stage and what is the preferred way to collaborate ?

TheSinding commented 4 years ago

I wouldn't mind helping, if you need the help. It true, it would be easier to manage it on your own repo.

I can see pros and cons of both ways.

claustres commented 4 years ago

Closing now as the discussion is not really related to migration to v4.

claustres commented 4 years ago

@OnnoGabriel @TheSinding @mattyg For your information before trying to publish to NPM I updated some old dependencies like mocha or istanbul. Then I tried to lint the code and found some strange things. Notably this line which I commented out now but that should probably be a throw. However if I add this throw then a lot of tests fail, meaning that some tests are not probably written correctly. If anyone could give it a try it would help, thanks.

TheSinding commented 4 years ago

@claustres I'll give it a look on Sunday

OnnoGabriel commented 4 years ago

@TheSinding Please let me know if you need help or if you have no time.

claustres commented 4 years ago

FYI @TheSinding I've just invited you as a maintainer on the repo it it helps

TheSinding commented 4 years ago

@OnnoGabriel - I will do, had plans to do it sunday afternoon, but I had family stuff. I will see if I can get it done by today or tomorrow.

@claustres - I have accepted your invite :)

claustres commented 4 years ago

Good news guys, I think I am done now with the tests and fixed latest issue after identifying the problem with @TheSinding, before releasing it will be great however if someone could check if it works well on his side.

TheSinding commented 4 years ago

@claustres I can do that, but right now I'm in the middle of moving and increased workload from my job, but I'll give it a look around the 5th ?

OnnoGabriel commented 4 years ago

@claustres, great news indeed! I can test it in our app, although we are not using all of the functionality of this package (just e-mail verification and password request/reset). On which branch?

claustres commented 4 years ago

@OnnoGabriel master, an integration test with an app would be great @TheSinding you do what you can, no problem ;-) If not possible soon I will try to release anyway

TheSinding commented 4 years ago

@claustres - I just ran the tests on my machine, from the master, and I can confirm that it works.

OnnoGabriel commented 4 years ago

@claustres, all tests on my mache run through, too.

Next, I installed the master branch from this repo in our app, created a new user, verified its email address and performed a password request/reset. All fine!

claustres commented 4 years ago

Great, will try to release then.

claustres commented 4 years ago

Just published a new major release (v3.0.0) on npm, let me know if it works fine on your side.