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

PXP-4219 Fix/session #806

Merged
merged 11 commits into from
Feb 22, 2021
Merged

PXP-4219 Fix/session #806

merged 11 commits into from
Feb 22, 2021

Conversation

mfshao
Copy link
Collaborator

@mfshao mfshao commented Feb 18, 2021

Jira Ticket: PXP-4219

Improved token refresh and session monitor logic, see comments in SessionMonitor/index.js

TL;DR: Portal will now tracking user's activity interval and actively logging out inactive users, rather than passively rely on waiting for Fence's token expiration.

To support this, Fence must be able to update access token in cookie before the current one expires as well, see uc-cdis/fence#874

Also removed a good chunk of old codes in portal (mostly relevent to the old submissionApiOauth, which is before Fence)

Deployed in: https://qa-mickey.planx-pla.net/

Improvements

  • Updated session monitor and logout inactive user logic, fixing various bugs (401 requests in middle of session, wrongly displayed logout popups, etc...)

Dependency updates

  • Should be used with Fence 4.26.1

@@ -168,5 +168,4 @@ module.exports = {
typescript: {}, // this loads <rootdir>/tsconfig.json to eslint
},
},
ignorePatterns: ['src/params.js'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have to put this into .eslintignore since my vscode eslint doesn't like this somehow and it breaks my local eslint 🤷‍♂️

@mfshao mfshao marked this pull request as ready for review February 18, 2021 21:07
const newState = Object.assign({}, initialState);

if (!newState.authenticated) {
return Promise.resolve(newState);
}
if (nowMs - lastTokenRefreshMs < 41000) {
return Promise.resolve(newState);
}
return store.dispatch(fetchProjects())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if-blocks in this area appear to be unnecessary -- the conditions all result in return Promise.resolve(newState);

Copy link
Contributor

@ZakirG ZakirG Feb 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically this snippet:

        const projects = store.getState().submission.projects;
        if (projects) {
          // user already has a valid token
          return Promise.resolve(newState);
        } else if (info.status !== 403 || info.status !== 401) {
          // do not authenticate unless we have a 403 or 401
          // should only check 401 after we fix fence to return correct
          // error code for all cases
          // there may be no tables at startup time,
          // or some other weirdness ...
          // The oauth dance below is only relevant for legacy commons - pre jwt
          return Promise.resolve(newState);
        }
        return Promise.resolve(newState);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the original logic of marking user as not authed and redirecting to /login is still valid, so I updated with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants