Centralize WTS refresh token initialization #1095

pieterlukasse · 2022-09-19T15:16:18Z

Jira Ticket: VADC-154

Improvements

WTS is a useful service for other backend components that need to access gen3 services using a token bound to a specific user account.

Dependency updates

WTS becomes a dependency for data-portal in general, instead of just for the /workspace subsection of the portal.
- The change IS backwards compatible, since this is controlled by a new optional "feature flag" called workspaceTokenServiceRefreshTokenAtLogin. If this is set to true it will refresh the WTS token directly at portal login (recommended mode). If not set, this refresh happens only when the user enters the workspace section of the portal (default/old/previous mode).

Deployment changes

this change assumes WTS is already a core/central service. The dependency is hereby moved to the homepage (instead of the ./workspace page)

pieterlukasse · 2022-09-19T15:18:00Z

@m0nhawk @kmhernan who else needs to be added as reviewer here?

m0nhawk · 2022-09-19T21:11:46Z

Also Mingfei is back. I've added him. 👍

Avantol13 · 2022-09-26T18:47:12Z

I'm not sure I agree with the concept here. WTS should not be a hard dependency of portal. We have data commons that need portal but don't need WTS

Avantol13 · 2022-09-26T18:47:51Z

The assumption that WTS is configured and used everywhere is not a safe assumption

pieterlukasse · 2022-09-26T19:30:36Z

@Avantol13 thanks for the review! You bring up a good point, and I thought about this as well when making this PR. One reason I added it here is because I see other workspace related items in src/index.jsx already. Do these then also not belong there, or is there a part of "workspace" that functions and is useful without WTS?

Avantol13 · 2022-10-03T19:37:16Z

@Avantol13 thanks for the review! You bring up a good point, and I thought about this as well when making this PR. One reason I added it here is because I see other workspace related items in src/index.jsx already. Do these then also not belong there, or is there a part of "workspace" that functions and is useful without WTS?

Honestly I don't think workspace things should necessarily be there either, but adding more coupling (for example to WTS) is just making things worse in my opinion. I don't have a ton of portal experience so ultimately I need to defer to @mfshao

imo portal should support commons that don't have workspaces at all, commons that only have workspaces for themselves (e.g. no need to WTS), and it should support ecosystems where we want workspaces and WTS.

We have running, production examples of all of those things, and it seems like this PR presumes that we want everything to be an ecosystem with workspaces and WTS, and that just isn't the case.

pieterlukasse · 2022-10-03T20:04:34Z

thanks @Avantol13, that makes sense. I agree that in this case probably the other references to workspaces should also be removed. @mfshao what do you think?

mfshao · 2022-10-04T15:00:10Z

I agree, WTS token initialization shouldn't happen as a part of portal app init process. It should be triggered only if a WTS related feature is enabled: workspace, GWASApp or AggAuthMapping

I think we have the flags in config file to determine whther the later 2 feature has been enabled, for workspace, that is part of an old logic. We should probably also use a feature flag to explicitly to turn it on.

pieterlukasse · 2022-10-11T17:46:29Z

@mfshao thanks for your comment. I added this commit 1456267 which will trigger the initialization/refresh based on a very specific feature flag (I called it workspaceTokenServiceRefreshTokenAtLogin). This is still based on @Avantol13 's comment that some installations might have workspace without needing WTS (although this code seems to contradict @Avantol13's comment - is this a bug?). Please let me know what you think. Maybe workspaces and WTS are always together?

docs/portal_config.md

mfshao

I don't think we support "workspace without WTS" a long while ago...

But this PR has some linting issues that needs to be fixed

mfshao · 2022-10-11T20:08:11Z

src/Workspace/index.jsx

-          }
-        },
-      );
+    initWorkspaceRefreshToken(this.connected);


so, if this new feature is enabled, when user navigates from a page to workspace, another wts call will be dispatched? seems like a bit redundant

Good point... But I think that if we want to do this with a feature flag that is false by default, then we will need to keep some of this redundancy for backwards compatibility purposes. Also, an extra refresh is not so bad...it is better than no refresh (which happens if users use the portal but don't go into the workspace for a long period - a scenario we have in our project).

Please note that this diff is technically not really a code/behaviour change: I basically moved the code on the left into a generic function that can be used in different places.

Another option is to make a breaking change and make a feature flag for workspaces as a whole, and make this flag true by default. This would simplify the code, but would require some special attention from teams that have a deployment that does not use workspace/ WTS. Please let me know what you think.

why not check if workspaceTokenServiceRefreshTokenAtLogin is enabled, and only make the call if it's not?

That could work, but IMO it is an unnecessary optimisation. But if you prefer this, I can easily add it. Let me know.

it makes sense to me... depending on the flag, the call is made either globally, or when the workspace page loads, and not both. But we can defer to @mfshao who brought this up in the first place

That is the point I bring this up: we should be able to add a check so this didn't get called repeatly

Ok, I'll add a condition.

Fixed here: 882043f

mfshao · 2022-10-11T20:08:46Z

src/index.jsx

@@ -67,6 +67,12 @@ import ErrorWorkspacePlaceholder from './Workspace/ErrorWorkspacePlaceholder';
 import { ReduxStudyViewer, ReduxSingleStudyViewer } from './StudyViewer/reduxer';
 import NotFound from './components/NotFound';
 import ErrorPage403 from './components/ErrorPage403';
+import { initWorkspaceRefreshToken } from './Workspace/WorkspaceRefreshToken';
+
+if (isEnabled('workspaceTokenServiceRefreshTokenAtLogin')) {


other index pages (which are being used by other portal bundles) also needs this change?

not sure. Is this one not used by all?

no, some Data Commons have their own landing page, so they have their own index page (covid19Index.jsx, nctIndex.jsx, etc)

so do we need to add this change there as well @paulineribeyre?

The logic needs to happen for every Commons, not just the ones that use the default landing page

Ok, I'll add that

Or not... This change has been reverted in favor of the one in src/Login/ProtectedContent.jsx 😅

paulineribeyre · 2022-10-18T17:55:11Z

src/Login/ProtectedContent.jsx

@@ -70,6 +72,10 @@ class ProtectedContent extends React.Component {
                const latestState = { ...newState };
                latestState.dataLoaded = true;
                this.setState(latestState);
+                if (newState.authenticated && isEnabled('workspaceTokenServiceRefreshTokenAtLogin')) {
+                    // initialize WTS:
+                    initWorkspaceRefreshToken();


because this is in ProtectedContent.componentDidMount, is the call to WTS going to be triggered for each ProtectedContent component in the index page?

@paulineribeyre would that be a problem? I think it is a really light call.

Yes i think that's a problem, from a quick search i see 26 ProtectedContents in src/index.jsx, we should not make so many API calls every time someone loads the page. Can we make a single call to WTS when the portal is loaded?

Thanks for checking @paulineribeyre !
It is an interesting finding, since the same could be said regarding the call to checkLoginStatus just a few lines up:

.then( () => this.checkLoginStatus(store, this.state)

Maybe that is why checkLoginStatus has the code below to avoid repeated calls? Should we do something similar here? Or should we try to find another location for our init code?

checkLoginStatus = (store, initialState) => { const newState = { ...initialState }; const nowMs = Date.now(); newState.authenticated = true; newState.redirectTo = null; newState.user = store.getState().user; if (nowMs - lastAuthMs < 60000) { // assume we're still logged in after 1 minute ... return Promise.resolve(newState); } ...

@mfshao what would you suggest for this?

you can do something similar like this

or change in your wts init code so that if a user is not logged in, don't force them to do an OIDC dance with WTS

i am curious why you chose to add it to ProtectedContent instead of index.jsx like in your initial code?

@paulineribeyre I was asked by @mfshao to do this...I now realise this was in slack and is not here in the PR comments.

@mfshao do you mind replying to this here?

In a nutshell: the code here is "guaranteed to be called after login", and having it in index.jsx will break any parts that are public because it would force a redirect to a login page

@paulineribeyre @mfshao I added a proposal here in this last commit: f3e37c7. Please let me know what you think. It is based on the current solution for checkLoginStatus in ProtectedContent.jsx

paulineribeyre

lgtm, deferring to @mfshao

mfshao

I think we should test this features more. At least we should configure the portal to have some public/protected pages and try to login with user who already has WTS connected and who doesn't have WTS connected. To see if there will be any unexpected behaviors

mfshao · 2022-10-21T17:38:27Z

src/Workspace/WorkspaceRefreshToken.js

+        .then(
+          ({ status }) => {
+            if (status !== 200) {
+              window.location.href = `${wtsPath}/authorization_url?redirect=${window.location.pathname}`;


Notice that the redirect here only contains window.location.pathname, so if the page that the user is at contains some query params in its URL, they will be lost

This was not a problem before because WTS login redirect only happens in certian pages that don't need query params. But if you are moving this redirect so it can happen virtually from any portal pages, you need to accomodate accordingly, because this will have a product-wise impact

Also there are some location and history based redirect/state restore happens in other pages of portal. We should make sure this change doens't breaks them

hi @mfshao, as discussed, I've update the redirect code to use the same logic as is used in the src/Login/Login.jsx page. See latest commit added to this PR: fc6c36a

Please let me know if this is the change you were looking for.

mfshao · 2022-10-21T17:40:03Z

Also the tests need to get fixed before it can be merged

pieterlukasse requested review from m0nhawk and kmhernan September 19, 2022 15:17

pieterlukasse requested review from paulineribeyre and craigrbarnes September 19, 2022 15:18

m0nhawk requested a review from mfshao September 19, 2022 21:11

pieterlukasse force-pushed the centralize_wts_refresh_token_initialization branch from cf6dab7 to 1456267 Compare October 11, 2022 17:41

Avantol13 reviewed Oct 11, 2022

View reviewed changes

docs/portal_config.md Show resolved Hide resolved

pieterlukasse mentioned this pull request Oct 11, 2022

use new feature flag to enable faster refresh of WTS token uc-cdis/gitops-qa#2227

Merged

mfshao reviewed Oct 11, 2022

View reviewed changes

pieterlukasse force-pushed the centralize_wts_refresh_token_initialization branch 2 times, most recently from 42436aa to a0ce7ed Compare October 18, 2022 17:12

paulineribeyre reviewed Oct 18, 2022

View reviewed changes

pieterlukasse force-pushed the centralize_wts_refresh_token_initialization branch from 82b7b20 to f3e37c7 Compare October 20, 2022 19:19

paulineribeyre reviewed Oct 20, 2022

View reviewed changes

mfshao reviewed Oct 21, 2022

View reviewed changes

pieterlukasse changed the title ~~Centralize wts refresh token initialization~~ Centralize WTS refresh token initialization Nov 2, 2022

pieterlukasse force-pushed the centralize_wts_refresh_token_initialization branch 4 times, most recently from b91eccb to ad4ec40 Compare November 3, 2022 15:16

This was referenced Feb 6, 2023

Gen3 Monthly Release 2023.02 AnVIL internalstaging internalstaging.theanvil.io - 2023.02 1675716791 uc-cdis/cdis-manifest#5700

Closed

AnVIL 2023.02 production 1675796785 uc-cdis/cdis-manifest#5707

Merged

PlanXCyborg mentioned this pull request Feb 19, 2023

Reduce qa.planx-pla.net footprint uc-cdis/gitops-qa#2373

Merged

PlanXCyborg mentioned this pull request Mar 31, 2023

update services uc-cdis/cdis-manifest#5892

Merged

github-actions bot mentioned this pull request Nov 2, 2023

feat(va-perf): update to 2023.10 uc-cdis/cdis-manifest#6744

Merged

Centralize WTS refresh token initialization #1095

Centralize WTS refresh token initialization #1095

Conversation

pieterlukasse commented Sep 19, 2022 • edited Loading

Improvements

Dependency updates

Deployment changes

pieterlukasse commented Sep 19, 2022

m0nhawk commented Sep 19, 2022

Avantol13 commented Sep 26, 2022

Avantol13 commented Sep 26, 2022

pieterlukasse commented Sep 26, 2022

Avantol13 commented Oct 3, 2022

pieterlukasse commented Oct 3, 2022 • edited Loading

mfshao commented Oct 4, 2022

pieterlukasse commented Oct 11, 2022 • edited Loading

mfshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieterlukasse Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieterlukasse Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulineribeyre Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

pieterlukasse Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieterlukasse Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

pieterlukasse Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

paulineribeyre left a comment

Choose a reason for hiding this comment

mfshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieterlukasse Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

mfshao commented Oct 21, 2022

pieterlukasse commented Sep 19, 2022 •

edited

Loading

pieterlukasse commented Oct 3, 2022 •

edited

Loading

pieterlukasse commented Oct 11, 2022 •

edited

Loading

pieterlukasse Oct 12, 2022 •

edited

Loading

pieterlukasse Oct 19, 2022 •

edited

Loading

paulineribeyre Oct 19, 2022 •

edited

Loading

pieterlukasse Oct 20, 2022 •

edited

Loading

pieterlukasse Oct 20, 2022 •

edited

Loading

pieterlukasse Oct 20, 2022 •

edited

Loading

pieterlukasse Nov 2, 2022 •

edited

Loading