User.logout() return 500 error if auth token not found

[emgeee]

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 a 500 error could make sense in this situation but it might make more sense for the method to simply return success.

[superkhau]

@bajtos @raymondfeng @ritch Opinions?

[raymondfeng]

We have a pending PR at #1496. @ritch Can you finish it?

[emgeee]

I don't think PR #1496 quite fixes the issue I'm running into. If User.logout() requires a user to first be logged in (as in, a valid AccessToken exists in the database), is it possible to log a user out without having to manually call window.localStorage.removeItem('$LoopBack$accessTokenId')?

[barocsi]

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.

[bajtos]

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
               });

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){});

[barocsi]

Great, it works, thanks @bajtos

[kevintechie]

A missing or invalid access token should return a 401 per HTTP 1.1 RFC. See my response to related issue: #1496

[doublemarked]

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:

    {
      "principalType": "ROLE",
      "principalId": "$everyone",
      "permission": "ALLOW",
      "property": "logout"
    },

Change to $authenticated?

[dkarlovi]

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.

[seanmangar]

@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.

'use strict';

const path = require('path');
const appRoot = require('app-root-path');

function slugify(name) {
  name = name.replace(/^[A-Z]+/, s => s.toLowerCase());
  return name.replace(/[A-Z]/g, s => '-' + s.toLowerCase());
}

module.exports = (Model, options) => {
  let configFile = options.config;
  if (!configFile) {
    // Works for 99% of cases. For others, set explicit path via options.
    configFile = path.join('./common/models/', slugify(Model.modelName) + '.json');
  }
  const config = appRoot.require(configFile);

  if (!config || !config.acls) {
    console.error('ClearBaseAcls: Failed to load model config from', configFile);
    return;
  }

  Model.settings.acls.length = 0;
  config.acls.forEach(r => Model.settings.acls.push(r));
};

[dkarlovi]

@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.

[loay]

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