Skip to content

Enable refresh-token server side support - #2026

Closed
jackywxd wants to merge 6 commits into
feathersjs:crowfrom
jackywxd:crow
Closed

jackywxd wants to merge 6 commits into
feathersjs:crowfrom
jackywxd:crow

Conversation

@jackywxd

Copy link
Copy Markdown

Summary

Enable refresh-token server side support by extending the existing Authentication Service. As refresh-token is a stateful resource, it also depends on refresh-token entity service, similar as local strategy depends on user entity service.

Related issue: #1337

Includes unit test cases

@daffl

daffl commented Aug 18, 2020

Copy link
Copy Markdown
Member

Thank you very much for this pull request @jackywxd! Sorry it took so long to get back to it. I think this is a great start. My questions would be

  • If it can be its own module, it is always easier to publish it separately instead of pulling it into the core
  • Aren't there any changes that need to be made in the core modules (like the authentication-client)?

@jackywxd

Copy link
Copy Markdown
Author

@daffl David, thanks for your time! You are right, it can be on its own module and it is a much cleaner and easier solution.

The current code change is only applied for server-side, not related to authentication-client. I have't got a chance to look at the client-side code yet. Ideally, the client-side integration should be included.

Anyway, I will close this request for now, will see if I can make changes to both server and client and publish as separated modules.

Thanks again!

@jackywxd jackywxd closed this Aug 24, 2020
@daffl

daffl commented Aug 24, 2020

Copy link
Copy Markdown
Member

Great, thank you. Please let me know if you need anything. Once it is ready we should definitely also do a writeup.

@itoonx

itoonx commented Aug 28, 2020

Copy link
Copy Markdown

I'm looking for it 👍

@bujji1

bujji1 commented Dec 6, 2021

Copy link
Copy Markdown

@jackywxd! I found only the TypeScript version of the "refresh token " fork , do we have the Java script version also? Thanks for your help on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants