Project

General

Profile

Actions

Feature #15530

closed

Workbench2 trusts federation users

Added by Peter Amstutz over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
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 1 (0 open1 closed)

Task #15622: Review 15530-wb2-loginclusterResolvedPeter Amstutz10/07/2019Actions

Related issues

Related to Arvados - Idea #15529: [API] [Controller] Share user account database with a group of trusted clustersResolvedPeter Amstutz08/22/2019Actions
Related to Arvados - Bug #15709: [Workbench2] Automatically add local cluster to the remoteCluster configResolvedActions
Actions #1

Updated by Peter Amstutz over 4 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz over 4 years ago

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

Updated by Peter Amstutz over 4 years ago

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

Updated by Tom Morris over 4 years ago

  • Description updated (diff)
Actions #5

Updated by Eric Biagiotti over 4 years ago

  • Description updated (diff)
Actions #6

Updated by Eric Biagiotti over 4 years ago

  • Description updated (diff)
Actions #7

Updated by Eric Biagiotti over 4 years ago

  • Description updated (diff)
Actions #8

Updated by Tom Morris over 4 years ago

  • Story points set to 2.0
Actions #9

Updated by Tom Morris over 4 years ago

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

Updated by Tom Morris over 4 years ago

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

Updated by Peter Amstutz over 4 years ago

  • Description updated (diff)
Actions #12

Updated by Peter Amstutz over 4 years ago

  • Description updated (diff)
Actions #13

Updated by Tom Morris over 4 years ago

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

Updated by Peter Amstutz over 4 years ago

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

Updated by Peter Amstutz over 4 years ago

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

Updated by Peter Amstutz over 4 years ago

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

Actions #17

Updated by Peter Amstutz over 4 years ago

  • Status changed from New to In Progress
Actions #18

Updated by Peter Amstutz over 4 years ago

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

Updated by Peter Amstutz over 4 years ago

workbench2 repository

15530-wb2-logincluster @ 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
Actions #20

Updated by Peter Amstutz over 4 years ago

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

Updated by Eric Biagiotti over 4 years ago

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

Updated by Peter Amstutz over 4 years 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.

Actions #23

Updated by Eric Biagiotti over 4 years 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.
Actions #24

Updated by Peter Amstutz over 4 years ago

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

Updated by Peter Amstutz over 4 years 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 @ df4133dde10614e53a41b16a5c6062c3d1777059

Actions #26

Updated by Eric Biagiotti over 4 years 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.
Actions #27

Updated by Peter Amstutz over 4 years 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.

Actions #28

Updated by Eric Biagiotti over 4 years 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.

Actions #29

Updated by Peter Amstutz over 4 years 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.

Actions #30

Updated by Peter Amstutz over 4 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF