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.

Sign up for GitHub

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-7429): fix/workspaceLogout: added backup if api call fails to refresh user, … #804

Merged
merged 6 commits into from
Feb 15, 2021
Merged

(PXP-7429): fix/workspaceLogout: added backup if api call fails to refresh user, … #804

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6994422
fix/workspaceLogout: added backup if api call fails to refresh user, …
Feb 12, 2021
49a8a3d
fix/workspaceLogout: added more comments
Feb 12, 2021
79b0644
fix/workspaceLogout: moved workspace logic to workspace, removed old …
Feb 12, 2021
f5ab350
fix/workspaceLogou: remove unneeded looping of retrying user api calls
Feb 12, 2021
4844c94
fix/workspaceLogout: fix lint error
Feb 13, 2021
57c8c2c
Merge branch 'master' into fix/workspaceLogout
ocshawn Feb 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/SessionMonitor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ export class SessionMonitor {
if (response.type === 'UPDATE_POPUP') {
this.popupShown = true;
}
}).catch(() => {
// if API failed check if user is still logged in
this.notifyUserIfTheyAreNotLoggedIn();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why logging out user at here? the logic was if it can reach refreshSession() then it means the user is still logged in and actively using the portal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bug had to do with the user being logged out, but not alerted that they had been logged out, leading to other errors. this code checks if they are still logged in and if not alerts them, it does not log them out. I am still not 100% on how all of this code works if you do not think this check is needed i can remove

Copy link
Collaborator

@mfshao mfshao Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the bug was actually buried deeper in the logic flow: the notifyUserIfTheyAreNotLoggedIn function will make a call to the submission api and when it returns a 401 the portal assumes the user is logged out. But this will result in as the current behavior that for some times the popup showed up early while in other occassions some are shown up later.

This is mainly due to the fact that the access token in cookie is not updated until the pervious one has expired. After this fix uc-cdis/fence#874 is merged, both session token and access token will be updated.

For that case, I probably will test and change this logic flow back to let portal actively logout users. So this piece of code will probably be removed in my work. I'm okay will merging as is because I think it won't break things, just a FYI

The changes to the workspace code LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, thanks for looking over, im glad someone else was also looking into this

});
});
}
Expand Down
26 changes: 16 additions & 10 deletions src/Workspace/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import galaxyIcon from '../img/icons/galaxy.svg';
import ohifIcon from '../img/icons/ohif-viewer.svg';
import WorkspaceOption from './WorkspaceOption';
import WorkspaceLogin from './WorkspaceLogin';
import sessionMonitor from '../SessionMonitor';

class Workspace extends React.Component {
constructor(props) {
Expand Down Expand Up @@ -61,16 +62,6 @@ class Workspace extends React.Component {
);
}

componentDidUpdate() {
// force workspace iframe acquire focus if it does not have yet
// to fix the noVNC workspace doesn't respond to keyboard event when came up
if (document.getElementsByClassName('workspace')
&& document.getElementsByClassName('workspace')[1]
&& document.getElementsByClassName('workspace')[1] !== document.activeElement) {
document.getElementsByClassName('workspace')[1].focus();
}
}

componentWillUnmount() {
if (this.state.interval) {
clearInterval(this.state.interval);
Expand Down Expand Up @@ -110,6 +101,19 @@ class Workspace extends React.Component {
({ data }) => data.status,
).catch(() => 'Error');

oniframeLoad = (e) => {
// force workspace iframe acquire focus if it does not have yet
// to fix the noVNC workspace doesn't respond to keyboard event when came up
e.target.focus();

// add event listeners for sessionMonitor timeout
const iframeContent = e.target.contentDocument;
if (iframeContent) {
iframeContent.addEventListener('mousedown', () => sessionMonitor.updateUserActivity(), false);
iframeContent.addEventListener('keypress', () => sessionMonitor.updateUserActivity(), false);
}
}

getIcon = (notebook) => {
if (this.regIcon(notebook, 'R Studio')) {
return rStudioIcon;
Expand Down Expand Up @@ -253,6 +257,7 @@ class Workspace extends React.Component {
title='Workspace'
frameBorder='0'
src={`${workspaceUrl}proxy/`}
onLoad={this.oniframeLoad}
/>
</div>
<div className='workspace__buttongroup'>
Expand Down Expand Up @@ -333,6 +338,7 @@ class Workspace extends React.Component {
frameBorder='0'
className='workspace__iframe'
src={workspaceUrl}
onLoad={this.oniframeLoad}
/>
</div>
);
Expand Down