-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: rest-express-auth example #2869
base: latest
Are you sure you want to change the base?
Conversation
Hi @eternalmoon1234 👋🏽 Do you have any updates on this PR? Would you like any help pushing this across the finish line? 🙂 |
Hey @eternalmoon1234 👋🏽 Do you intend to pick up this PR, and do you need any help? |
Hey @ruheni, ✋ Looking forward to finishing this! |
Hey @ruheni 👋 I finally finished up this PR. I've implemented the changes you proposed (as well as some others) and have followed the same standard of the rest-express-auth example. Please review & propose any changes when you can! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. I've left some comments. The example seems to be missing a test for it. You can add one in the .github/tests
folder.
One other thing I didn't include in the comments is that I think some endpoints might not require authentication, such as /feed
and /post/:id
(both GET endpoints). I would recommend protecting endpoints with write operations in general. Let me know if you would like any help determining what endpoints to protect
Hey @ruheni 👋 I've incorporated all of your requested changes. As for the protected routes, I've modified the ones that I thought required protection based on your guidance, although please feel free to suggest any alterations if you'd propose something different. Please let me know if there's anything else you'd like me to change 🙂 |
Do I need to update the |
Hey 👋🏾 |
Sounds good, I'll go ahead and write the new test in. |
Hey @ruheni 👋 The tests should pass now. Mind running the checks & verifying that everything's all good? If so, I think this is ready to merge. |
The logs for the failed test are no longer available. Can anyone specify what's the problem. |
This is the PR where I am working on the rest-express-auth example, details #2838. This is currently a draft.