-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
User.logout() return 500 error if auth token not found #1669
Comments
@bajtos @raymondfeng @ritch Opinions? |
I don't think PR #1496 quite fixes the issue I'm running into. If |
This nature has another bad result, as co-issue comes with Angular JS SDK User.logout(): it simply does not return anything, not even fires the callback. Meaning there is no way to detect the result of the logout nor the nature of the logout in this special case when accessToken had been deleted from the database. Reference #1081 return User.logout(function (err, result) {
// Never receives any result
}); As a workaround I would try to add a http rejction interceptor for 500s in angular but that would simply bad. |
Assuming the code snippet above is using the Angular client, then it is expected that the callback is not called on error. Angular's ngResource uses two callbacks - a success & an error callback, not a single callback with error as the first arg. return User.logout(function onSuccess(result){}, function onError(response){}); |
Great, it works, thanks @bajtos |
A missing or invalid access token should return a 401 per HTTP 1.1 RFC. See my response to related issue: #1496 |
I see the discussion from #1489 has been making its way through various issues and PRs. I want to again voice the suggestion I placed there - would the correct fix perhaps just to adjust the ACL? We all agree that logout should not be called without an access token. However, the ACL currently allows $everyone:
Change to $authenticated? |
Any way forward on this? Can I work around this from config somehow, via ACL? Edit: Overriding in local model ACL doesn't work as expected, other than overriding the remote method, cannot proceed further without a upstream fix. |
@dkarlovi You can use a mixin to clear the base ACL. The code looks something like this. I got it somewhere around the issues on github but can't remember where.
|
@seanmangar thanks, will try it out. But, shouldn't Loopback merge ACL rules by default and allow for overriding them from local model? If it's not done because of security (overriding ACL might lead to unsecured apps), I guess it could emit a warning when in development environment to ensure the dev is not overriding accidentally. |
This issue should have been fixed as per this pr: strongloop/loopback-sdk-angular#178 If the same problem is still persisting, please leave a comment so we can look further. Thanks |
I think this is related to #1489 but it seems to me that if an auth token is not found, Strongloop should assume the request is unauthorized and return http code 401. Currently, it returns a 500 error which implies the server is broken.
EDIT: upon further inspection of the code, it looks like the error is happening explicitly when
User.logout()
is called and the access token does not exist in the database. I can see how a500
error could make sense in this situation but it might make more sense for the method to simply return success.The text was updated successfully, but these errors were encountered: