Feature #15530

Workbench2 trusts federation users

Added by Peter Amstutz 3 months ago. Updated 22 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
10/07/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0

Description

Update user interface so that if Login.LoginCluster is set, don't display multi-cluster dropdown.

Also update the account linking feature to only display remote account linking using the same criteria as for the multi-cluster dropdown.

Update Config and Auth state to reflect Login.LoginCluster and propagate to the appropriate views (login-panel, link-account-panel, fed-login).


Subtasks

Task #15622: Review 15530-wb2-loginclusterResolvedPeter Amstutz


Related issues

Related to Arvados - Story #15529: [API] [Controller] Share user account database with a group of trusted clustersResolved08/22/2019

Related to Arvados - Bug #15709: [Workbench2] Automatically add local cluster to the remoteCluster configNew

History

#1 Updated by Peter Amstutz 3 months ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz 3 months ago

  • Status changed from In Progress to New
  • Description updated (diff)

#3 Updated by Peter Amstutz 3 months ago

  • Related to Story #15529: [API] [Controller] Share user account database with a group of trusted clusters added

#4 Updated by Tom Morris 3 months ago

  • Description updated (diff)

#5 Updated by Eric Biagiotti 3 months ago

  • Description updated (diff)

#6 Updated by Eric Biagiotti 3 months ago

  • Description updated (diff)

#7 Updated by Eric Biagiotti 3 months ago

  • Description updated (diff)

#8 Updated by Tom Morris 3 months ago

  • Story points set to 2.0

#9 Updated by Tom Morris 3 months ago

  • Target version changed from To Be Groomed to 2019-08-28 Sprint

#10 Updated by Tom Morris 3 months ago

  • Target version changed from 2019-08-28 Sprint to Arvados Future Sprints

#11 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#12 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#13 Updated by Tom Morris 2 months ago

  • Target version changed from Arvados Future Sprints to 2019-09-25 Sprint

#14 Updated by Peter Amstutz 2 months ago

  • Target version changed from 2019-09-25 Sprint to Arvados Future Sprints
  • Assigned To set to Peter Amstutz

#15 Updated by Peter Amstutz 2 months ago

  • Target version changed from Arvados Future Sprints to 2019-09-25 Sprint

#16 Updated by Peter Amstutz 2 months ago

Also weird things happen if configuration changes inconsistently with what's in local storage.

#17 Updated by Peter Amstutz about 2 months ago

  • Status changed from New to In Progress

#18 Updated by Peter Amstutz about 2 months ago

  • Target version changed from 2019-09-25 Sprint to 2019-10-09 Sprint

#19 Updated by Peter Amstutz about 1 month ago

workbench2 repository

15530-wb2-logincluster @ commit:d866a8ad7d9d8f48c761fa7ea8ea96b17cbfdb2f

To test this you need a federation with LoginCluster set, eg the "fedbox" cluster set up to test arv-federation-migrate.

  • wb2 "homecluster" is set to value of logincluster
  • don't display dropdown to select alternate home cluster for login and linking when logincluster is non-empty
  • when loading "sessions" from local storage, clear "active" and "validated" flags and re-initialize
  • when searching and logincluster is set, send unsalted token
  • when logging into the head logincluster, send unsalted token to other workbenches

#20 Updated by Peter Amstutz about 1 month ago

  • Target version changed from 2019-10-09 Sprint to 2019-10-23 Sprint

#21 Updated by Eric Biagiotti about 1 month ago

  • Related to Bug #15709: [Workbench2] Automatically add local cluster to the remoteCluster config added

#22 Updated by Peter Amstutz about 1 month ago

I was going to address #15709 on this branch. I looked at it, remoteHosts already adds the local cluster entry (whether it shows up in the cluster RemoteHosts config or not). I did notice that remoteHostConfigs did not include the local config, and so I added that.

I think this is ready to be looked at.

#23 Updated by Eric Biagiotti about 1 month ago

Peter Amstutz wrote:

I was going to address #15709 on this branch. I looked at it, remoteHosts already adds the local cluster entry (whether it shows up in the cluster RemoteHosts config or not). I did notice that remoteHostConfigs did not include the local config, and so I added that.

I think this is ready to be looked at.

  • Looks like you have to update some of the tests.
  • Logging into a cluster that isn't the LoginCluster presents the user with a button that says "Login to X with user from Y". This may be confusing for users in a federation with LoginCluster set. I think its worth conditionally changing that text to just say "Login to X" if there is a LoginCluster.
  • The Site-Manager link in search results needs some explanation. Maybe something like: "To manage clusters that are searched see the Site Manager", where instead of a Link, its a Button styled like the others. A right align might work to separate it from the rest of the text on that line also.
  • Unfortunately, I think this breaks account linking. When trying to link an account on a non-LoginCluster, it thinks every user is a remote user (uuid points to LoginCluster) and expects you to link to a local account, which wont exist if users are migrated. You probably just have to change link-account-panel-actions.tsx line 205 to compare the uuid against the LoginCuster if it exists.
  • Nit: site-manager-panel-root.tsx ln 164: </Typography> needs a tab.

#24 Updated by Peter Amstutz 27 days ago

  • Target version changed from 2019-10-23 Sprint to 2019-11-06 Sprint

#25 Updated by Peter Amstutz 26 days ago

Eric Biagiotti wrote:

Peter Amstutz wrote:

I was going to address #15709 on this branch. I looked at it, remoteHosts already adds the local cluster entry (whether it shows up in the cluster RemoteHosts config or not). I did notice that remoteHostConfigs did not include the local config, and so I added that.

I think this is ready to be looked at.

  • Looks like you have to update some of the tests.

Fixed.

  • Logging into a cluster that isn't the LoginCluster presents the user with a button that says "Login to X with user from Y". This may be confusing for users in a federation with LoginCluster set. I think its worth conditionally changing that text to just say "Login to X" if there is a LoginCluster.

Now it just says "Log in" when there are not any options. The cluster id is still visible in the upper left.

  • The Site-Manager link in search results needs some explanation. Maybe something like: "To manage clusters that are searched see the Site Manager", where instead of a Link, its a Button styled like the others. A right align might work to separate it from the rest of the text on that line also.

Tweaked it a little bit. It's hard to make it a button and also explain why it is there, so it is just a link.

  • Unfortunately, I think this breaks account linking. When trying to link an account on a non-LoginCluster, it thinks every user is a remote user (uuid points to LoginCluster) and expects you to link to a local account, which wont exist if users are migrated. You probably just have to change link-account-panel-actions.tsx line 205 to compare the uuid against the LoginCuster if it exists.

Now disables account linking entirely unless you're on the home cluster.

  • Nit: site-manager-panel-root.tsx ln 164: </Typography> needs a tab.

Fixed.

15530-wb2-logincluster @ commit:df4133dde10614e53a41b16a5c6062c3d1777059

#26 Updated by Eric Biagiotti 26 days ago

  • On a remote clusters link account page, users don't know what the login cluster is, so its not worth mentioning especially since we give them a link anyway. I would change the text to "Account linking is unavailable on this cluster. You must visit zzzzz to link accounts." Or something like that.
  • On the search results page, if there is only 1 cluster, I would take out the whole heading saying what cluster is being searched and the link to site manager. At the very least, make "clusters" singular.
  • Whitespace issues in link-account-panel-root.tsx and site-manager-panel-root.tsx.

#27 Updated by Peter Amstutz 25 days ago

Eric Biagiotti wrote:

  • On a remote clusters link account page, users don't know what the login cluster is, so its not worth mentioning especially since we give them a link anyway. I would change the text to "Account linking is unavailable on this cluster. You must visit zzzzz to link accounts." Or something like that.

Now it says "Please visit cluster xxxxx to perform account linking"

  • On the search results page, if there is only 1 cluster, I would take out the whole heading saying what cluster is being searched and the link to site manager. At the very least, make "clusters" singular.

I reworked this (and ended up tinkering with the rendering some more).

  • Whitespace issues in link-account-panel-root.tsx and site-manager-panel-root.tsx.

Fixed? I think? Relying on auto-indent from the language server.

#28 Updated by Eric Biagiotti 25 days ago

Peter Amstutz wrote:

Eric Biagiotti wrote:

  • On a remote clusters link account page, users don't know what the login cluster is, so its not worth mentioning especially since we give them a link anyway. I would change the text to "Account linking is unavailable on this cluster. You must visit zzzzz to link accounts." Or something like that.

Now it says "Please visit cluster xxxxx to perform account linking"

  • On the search results page, if there is only 1 cluster, I would take out the whole heading saying what cluster is being searched and the link to site manager. At the very least, make "clusters" singular.

I reworked this (and ended up tinkering with the rendering some more).

  • Whitespace issues in link-account-panel-root.tsx and site-manager-panel-root.tsx.

Fixed? I think? Relying on auto-indent from the language server.

This LGTM. I won't hold up the merge if you think link-account-panel-root.tsx is OK the way it is (I don't). Going forward, if the language server is mangling code, please don't commit it. It obscured your actual changes and made review more difficult.

#29 Updated by Peter Amstutz 22 days ago

Eric Biagiotti wrote:

This LGTM. I won't hold up the merge if you think link-account-panel-root.tsx is OK the way it is (I don't).

This comment is vague. Are you referring to the code formatting or something functional?

Going forward, if the language server is mangling code, please don't commit it. It obscured your actual changes and made review more difficult.

Well the review comment was non-specific "whitespace issues". So maybe I should have just left it alone rather than bulk reformat it. Unfortunately the language server I'm using has different ideas about how things should be indented than the original developers, perhaps we need to harmonize on a code formatter or particular settings.

#30 Updated by Peter Amstutz 22 days ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF