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

Clear projects data when logging out #400

Closed
1 task done
fyliu opened this issue May 30, 2020 · 2 comments · Fixed by #402
Closed
1 task done

Clear projects data when logging out #400

fyliu opened this issue May 30, 2020 · 2 comments · Fixed by #402
Assignees
Labels
bug Release Note: Shows as Error Correction level: missing priority: missing role: missing

Comments

@fyliu
Copy link
Member

fyliu commented May 30, 2020

Overview

User should no longer be able to access projects when logged out. Currently, the user can still access projects belonging to the logged out user.

Action Items

  • clear projects data when logging out
@fyliu fyliu added the bug Release Note: Shows as Error Correction label May 30, 2020
@lemazza lemazza self-assigned this May 31, 2020
@lemazza
Copy link
Contributor

lemazza commented Jun 1, 2020

After investigating a little bit, the reason a user can still access a project after logging out, is that a jwt cookie remains in the browser after the logout action.

with the cookie still present, the server will still authenticate requests.

from jwt-session.js lines 32, 33

async function validateUser(req, res, next) {
  const jwtString = req.headers.authorization || req.cookies.jwt;

the cookie is set by the server during login and can't be deleted from the client because it is httpOnly:

from jwt-session.js lines 22-27

async function login(req, res) {
  const token = await sign({ email: req.user.email, id: req.user.id });
  res.cookie("jwt", token, { httpOnly: true });
  const user = req.user;
  res.json({ isSuccess: true, token: token, user });
}

I haven't touched the back end on this project yet, so I want to be careful about rejiggering authorization. I think the best option here is removing httpOnly when doing res.cookie, but I'm not sure what best practices dictate for security. The language of our Registration and Login read-me suggest it could be removed by the client:

from register.md lines 66-70

Logout
To "logout", the client simply deletes the JWT token from the cookies, and deletes the user object from its state. Subsequent requests to the server will not have the authentication cookie until the user logs in again.
(This is not the most secure design. If a bad actor intercepts the JWT token, it will still be valid on the server before its cookie expiration time.)

Related:

res.headers.authorization isn't showing up on the server. When setTokenInHeaders is called, it attempts to retrieve the jwt token from localStorage, but a jwt token is never set in localStorage, so the Authorization header is never set. (I didn't write the TODO, but it seems to be correct).

This means as things are currently set, the jwt cookie is necessary for authorization. One possible solution to issue 400 is keeping the token in local storage and eliminating the res.cookie and req.cookies.jwt check entirely.

from App.js lines 59-71

  //TODO: This doesn't seem like it's getting used anymore. Don't see token in local storage. Check on authorization flow to see if token is still needed.
  const setTokenInHeaders = () => {
    axios.interceptors.request.use(
      config => {
        let token = localStorage.getItem("token");
        if (token) {
          config.headers["Authorization"] = `Bearer ${token}`;
        }
        return config;
      },
      error => Promise.reject(error)
    );
  };

Let me know the preferred path forward. As far as I can tell removing httpOnly from the jwt cookie so it can be deleted by the client seems the most simple. The other option being removing the use of cookies and handling jwt through local storage.

@lemazza
Copy link
Contributor

lemazza commented Jun 1, 2020

Found a simpler way to just clear the form inputs, but still believe the jwt cookie should be cleared on logout which can only be done by removing httpOnly or creating another call to the server to remove the cookie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Release Note: Shows as Error Correction level: missing priority: missing role: missing
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants