Conversation
mravinale
requested review from
AlphaGit,
juampick,
kilisoria,
mravinale,
noeliasfranco and
pablomigliassoms
| ``` | ||
|
|
||
| **This sub-generator was designed to be executed only once per application.** | ||
|
|
| this.destinationPath("package.json") | ||
| ); | ||
|
|
||
| } |
There was a problem hiding this comment.
all these files have changes compared to the originals?
There was a problem hiding this comment.
All have changes, but not all are new files...
Taking the assumption that it will run immediately we download the seed, I just copied all the files to their target folder as a quick solution. We can of course improve this.
There was a problem hiding this comment.
ok, as we chat before, in the mean time we can implement the modifications for the files doing update
|
|
||
| //If there's no user, then we get an empty array | ||
| if (foundUser.length === 0) { | ||
| res.status(404).json({}); |
There was a problem hiding this comment.
I think that non having elements, is not an error for returning 404
There was a problem hiding this comment.
Ok, what error code do you think we can return? My intention is to return a value different than 200 so we can realize there are no results.
There was a problem hiding this comment.
maybe 204(no content) would be more accurate
There was a problem hiding this comment.
In my opinion, User (or any other entity, keep on mind there is a template generator) not found functionality shouldn't be part of Login/SignUp.
On the other hand, how are you going to handle token expiration?
There was a problem hiding this comment.
in the other hand this would be a simple strategy (as a demo) looking for the user into the database, create a token with expiration and returning the token and maybe name and surname, or any other public information that we may need if the user is found in the database,
There was a problem hiding this comment.
Well, it seems to be the approach of this PR is only Login/Signup. However, to complete the whole workflow maybe you have to take on mind "User Registration" and "Forgot Password". Going back to token expiration issue, what will be the behaviour when it is expired? Redirect to login? Show an alert to notify that? Renew the token (silent mode)?
There was a problem hiding this comment.
good question, what do you think @dtoranzoms if the client in this case may show an alert in the same page, but in general how to handle the 403 error should be responsibility of each client from the BE point of view
There was a problem hiding this comment.
I really do not like either 401 nor 403 for this case. Those codes are for unauthorized (401) or for Forbidden (403). I think this case is not either case. The user just do not exist.
@mravinale regarding your question, yes. I think we should show an alert when the token expires and redirect to login when the user accepts it.
There was a problem hiding this comment.
As any authentication mechanism the last thing you need to do is to notify the user doesn't exist because it won't be secured at all.
|
@dtoranzoms If you have a chance please take a look to this repos: I'd always want to add security to localStorage in some way. If you think it is interesting and you have enough time please go ahead. Thanks!!!! |
5 participants
No description provided.