Skip to content
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

logoutTimer causing early logouts #404

Closed
jamesholcomb opened this Issue Jan 22, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@jamesholcomb
Copy link

jamesholcomb commented Jan 22, 2017

Steps to reproduce

From the source/socket/handler.js

// Clear any previous timeout if we have logged in again.
if (logoutTimer) {
  debug(`Clearing old timeout.`);
  clearTimeout(logoutTimer);
}

logoutTimer = setTimeout(() => {
  debug(`Token expired. Logging out.`);
  logout();
}, ms(authSettings.jwt.expiresIn));
  "auth": {
    "secret": "FEATHERS_AUTH_SECRET",
    "cookie": {
      "enabled": true, // whether the cookie should be enabled
      "name": "feathers-jwt", // the cookie name
      "httpOnly": false, // whether the cookie should not be available to client side JavaScript
      "secure": false // whether cookies should only be available over HTTPS
    },
    "jwt": {
      "expiresIn": "1y"
    },
    "strava": {
      "name": "strava",
      "successRedirect": "/auth/success",
      "clientID": "XXX",
      "clientSecret": "STRAVA_CLIENT_SECRET",
      "permissions": {
        "scope": [
          "public"
        ]
      }
    }

I haven't looked deeply into this, but the logoutTimer function seems to be prematurely logging out the user immediately after being authenticated.

Commenting it out fixes the problem.

Expected behavior

The user should not be logged out

Actual behavior

The user is logged out

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working):

    "feathers": "^2.0.2",
    "feathers-authentication-client": "^0.1.6",
    "feathers-hooks": "^1.7.1",
    "feathers-rest": "1.5.x",
    "feathers-socketio": "1.4.2",

NodeJS version:
7.2

Operating System:
Mac 10.11.6
Browser Version:
Chrome latest
React Native Version:
0.39
Module Loader:

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jan 22, 2017

Have you confirmed that the JWT's exp attribute is correct? Paste your token into JWT.io and it will decode the payload.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 20, 2017

Hmm... this shouldn't be happening but I'll have to probe this a bit more. We typically don't set our JWTs to that long a length. Usually they are at most 1 day in case they are ever compromised. Otherwise you're going to probably keep a blacklist or whitelist.

@ascloud-systems

This comment has been minimized.

Copy link

ascloud-systems commented Mar 1, 2017

I am hitting the same issue with jwt.expiresIn="90d". If I set it to "1d" it works, though.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Mar 23, 2017

This might be the same issue as #458

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 24, 2017

Weird. They do seem similar. It's impossible to test though because we can't realistically wait more than 20 days to test this. It is not recommended that you set JWT expiresIn to something that high.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Mar 25, 2017

I think we can test it by trying to see what ms returns when you put in 1y. I once wanted to set the expiration to one month, put in 1m and then was surprised why tokens always expired after a minute.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 25, 2017

ms('1y') equals 31557600000. When I did new Date(31557600000) I got 1971-01-01T06:00:00.000Z which seems about right.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 25, 2017

Looking at the code again I think I see a couple potential issues. I think the placement of the logoutTimer might be an issue. https://github.com/feathersjs/feathers-authentication/blob/master/src/socket/handler.js#L24

A few things that may be causing problems:

  1. Possibly the timer should actually be attached to the socket. I'm not sure what happens to the timer as multiple people authenticate. Given the scope of where the variable is defined, it might only be keeping time for the first person to authenticate.

  2. We should probably be explicitly clearing the timeout on logout.

  3. When we clear the timeout we should set logoutTimer to null. I believe once it is set, if (logoutTimer) { will still evaluate to true.

What do you think @daffl?

@ekryski ekryski self-assigned this Apr 11, 2017

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jul 2, 2017

I think this has all been fixed (with #534 as the last issue).

@daffl daffl closed this Jul 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.