trailsjs / sails-auth

Passport-based User Authentication system for sails.js applications. Designed to work well with the sails-permissions module.
https://www.npmjs.org/package/sails-auth
MIT License
266 stars 141 forks source link

sails.getBaseURL() is deprecated. #140

Closed tom-sherman closed 6 years ago

tom-sherman commented 8 years ago

As getBaseURL() is being deprecated in Sails v1.0, there should be implemented a more elegant way to define this.

The offending bit in passport.js:

var baseUrl = sails.getBaseurl();

switch (protocol) {
  case 'oauth':
  case 'oauth2':
    options.callbackURL = url.resolve(baseUrl, callback);
    break;

  case 'openid':
    options.returnURL = url.resolve(baseUrl, callback);
    options.realm     = baseUrl;
    options.profile   = true;
    break;
}

Reference:

There is no reliable, cross-platform way to automatically detect the external URL of a running Sails app (or any other Node app). In mission-critical situations, you are advised to pre-determine the URL and save it in a custom environment-dependent configuration value (e.g. sails.config.appUrl) that you can reference elsewhere in the app.

tjwebb commented 8 years ago

There is no reliable, cross-platform way to automatically detect the external URL of a running Sails app

So, what should we do?

tom-sherman commented 8 years ago

Could look for a few configuration values before using this deprecated method. Maybe then add an error message to advise to set one up?

scott-wyatt commented 8 years ago

I agree, I think that this should be set through an environment variable in future releases.

Garland220 commented 8 years ago

This will fail if 'sails.config.appUrl' is undefined.

I would suggest adding a check for undefined as well:

      var baseUrl = '';
      if (sails.config.appUrl && sails.config.appUrl !== null) {
        baseUrl = sails.config.appUrl;
      }
      else {
        sails.log.warn('Please add "appUrl" configuration value.');
        baseUrl = sails.getBaseurl();
      }

155

tom-sherman commented 8 years ago

@Garland220 That's strange, I specifically remember testing for this case and it produced the expected result (a warning).

TheSinding commented 7 years ago

Is there a fix for this issue?

tom-sherman commented 7 years ago

@TheSinding Which issue are you referring to? Both my solution and the hotfix have been merged into master

0aps commented 7 years ago

@tom-sherman he probably says that because your solution is not yet publish into npm.

TheSinding commented 7 years ago

Sorry I didnt answer until now @tom-sherman. Yeah @0aps is properly correct, when I tried with Sail 1.0 and the auth lib, I kept getting an sails.baseUrl() error..

abalad commented 6 years ago

Same here

amoritan commented 6 years ago

(+1) It would be great if the npm package was updated with the latest release available on GitHub.

lauragift21 commented 6 years ago

Is there a fix for this error?

michaelfahmy commented 6 years ago

Any fix?!!! It's too long since this issue has been opened. Please update the package!

tjwebb commented 6 years ago

see: