-
-
Notifications
You must be signed in to change notification settings - Fork 666
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
Secure api with express-jwt (wip) #82
Conversation
Few suggestions:
|
@@ -8,7 +8,11 @@ export class AuthGuardLogin implements CanActivate { | |||
constructor(public auth: AuthService, private router: Router) {} | |||
|
|||
canActivate() { | |||
return this.auth.loggedIn; | |||
if (this.auth.loggedIn()) { |
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.
you can shrink this: return this.auth.loggedIn();
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.
At the time of writing while referring to angular2-jwt documentation I considered that but just left the code ready for a redirect like in the usage example. Thought that I'd let Davide see the pull request if helpful or requires changes.
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.
It's ok, mabye just add a comment stating that a redirection would fit well in those lines.
Thank you for your feedback @jmrapp1 . I do need to learn how to implement the module properly. It was a rudimentary gathering of auth functionality without restructuring in mind. One thing that I've not had time to look at is restricting the API calls to user roles on the server. I guess that we'd use the token but I'm far from a reasonable understanding of express to write atm. Please feel free to make a pull request with your ideas using any part of my work. |
|
I will look at improving this when I have time. Seems like I'd be reinventing the wheel if someone has already made the implementation based on this project... would love to see how you've done it if you'd be happy to share. As I'm learning if you can share any good reading material I'm be grateful. Thank you for your scrutiny of my attempts thus far. EDIT: Along with your feedback, I might look into https://github.com/MichielDeMey/express-jwt-permissions as it looks like it might fulfil the requirements. |
That's an interesting repo, I'll have to look more into it but it seems like it would integrate with any JWT middleware. If I have some spare time I may work on an implementation with passport and passport-jwt, but only if it's in demand and @DavideViolante is still active with this repo. A few PRs (including yours) have been open for a few months without any comment from him, so if he is letting the repo die and not merging acceptable PRs it may not be worth my time. At that point I'd rather just create my own repo and support it. |
Recently I don't have enough time to reviews those PRs. I want to be sure that the PRs I receive are worth to be merged (and I also need community help on this). |
Awesome to hear @DavideViolante :D Glad it's not dead because the repo is a great basis for people to work off of and has a lot of room for more potential. I think that this PR has a a few changes that could still be made (mentioned above), but is moving in the right direction. |
Marked as WIP there is plenty to be improved in this PR. It's probably going to need to be broken down into smaller PRs like consolidating auth module properly or using tokenNotExpired for logged in check. IF anyone is inclined, feel free to roll out those incremental PRs. |
To give some type of direction for whomever wants to move ahead with the changes, here is what I'd suggest:
|
if (!req.payload.user._id) { | ||
res.status(401).json({ | ||
'message' : 'UnauthorizedError: private' | ||
}); |
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.
These 4 lines (7-10) are repeated throughout the entire file. There is surely a better way to write this part, in a way to respect the DRY principle.
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.
i am trying to say that i have already implemented similar kind of workout
Closing PR. Revision needed. Follow up: New PR |
First draft to secure api. I hope it is useful. Includes:
Have rudimentarily tested API using postman: http://imgur.com/a/srpLU.
Also not sure about (have arbitrarily returned observable boolean in place of boolean):
30: client/app/services/auth.service.ts
TODO: Address user roles
Issues: #58