Arvados

15881-ldap @ bbb132e983f9ec5c7d50cf0ab709ec041af1f844 -- developer-run-tests: #1836

LDAP has several configs and it seemed sensible to put them in an LDAP section. But this means we have

Login:
  # ...
  GoogleClientID: "" 
  GoogleClientSecret: "" 
  GoogleAlternateEmailAddresses: true
  PAM: true
  PAMService: arvados
  LDAP:
    Enable: true
    URL: ldap://ldap:389
    # ...

Seems like we should pick one approach here: all flat, or all hierarchical like this:

Login:
  # ...
  Google:
    Enable: true
    ClientID: "" 
    ClientSecret: "" 
    AlternateEmailAddresses: true
  PAM:
    Enable: true
    Service: arvados
  LDAP:
    Enable: true
    URL: ldap://ldap:389
    # ...

Tom Clegg wrote:

LDAP has several configs and it seemed sensible to put them in an LDAP section. But this means we have

[...]

Seems like we should pick one approach here: all flat, or all hierarchical like this:

[...]

I am fine with migrating to a more hierarchical layout, and we already have a concept of deprecated configs that are automatically migrated.

However, this also affects the exported config, so anything that's expecting the old organization breaks. We don't yet have a general solution for that.

Now on its own, probably the only thing that is affected is Workbench 2 reading the PAM config. However this is the sort of thing I was trying to avoid by proposing a "LoginEndpoint" field or some other strategy for that Workbench 2 can use to determine how to do login without having to know about every login method support by Arvados.

Also, Workbench 2 needs to support LDAP, so I think we need to address this anyway.

However, this also affects the exported config, so anything that's expecting the old organization breaks.

Fortunately Login.PAM is the only config here that's ever been exported, and it has only existed as an experimental feature in dev/prerelease versions, so I think we're safe.

15881-ldap @ 0634b763dd27914cff5ca49c6cfe11233746ee31 -- developer-run-tests: #1842

Some comments & questions:

• Documentation
  • Would it be convenient to warn the reader that SSO’s going to be decommissioned in the near future? I guess new installations shouldn’t be encouraged to think that multiple auth methods will be something supported through SSO.
• Configuration
  • Do you think we could maintain config naming consistency by renaming InsecureTLS to Insecure, or just use TLS.Insecure? Maybe the latter isn’t convenient because TLS.Insecure is about Arvados’ services.
  • Arvbox config needs an update on the Login section.
• File lib/controller/localdb/login_ldap.go - Line 92: Would it be better to do this check before any LDAP server interaction?
• File services/api/config/initializers/omniauth_init.rb - Lines 12 & 18 refers to deprecated Rails.configuration.Login["ProviderAppID”] config
• On 8f435f4bac86e7ba7dbd9770d2db9bb4db6cf569, the comment is: test LDAP login using a fake LDAP server, but I’m not able to understand how this is done, as the code updates don’t seem to be about that. Is the godap dependency providing the LDAPSuite? A comment explaining how it works would be useful.
• Is the docker test being run by Jenkins? I’m not seeing any logging about it on "developer-run-tests-remainder” so I suppose it isn’t, do you think it should be run every time? A least on my side, it isn’t too slow, took like 20 seconds.

Other than that, LGTM.

• Would it be convenient to warn the reader that SSO’s going to be decommissioned in the near future? I guess new installations shouldn’t be encouraged to think that multiple auth methods will be something supported through SSO.

Yes, I've removed SSO entirely from the install guide, and added a "remove sso-provider" item to the future 2.1 upgrade notes.

• Do you think we could maintain config naming consistency by renaming InsecureTLS to Insecure, or just use TLS.Insecure? Maybe the latter isn’t convenient because TLS.Insecure is about Arvados’ services.

• Even where TLS.Insecure == true (internal communication among Arvados components is unsecured) is acceptable, revealing users' LDAP credentials to a MitM could be a real problem. So I think it should be a separate knob.
• I agree symmetry with the other Insecure flag would be nice - but I also like that InsecureTLS makes it clear that we're talking about TLS, not other aspects of authentication that could be considered insecure, like empty/easy passwords.

• Arvbox config needs an update on the Login section.

Updated.

• File lib/controller/localdb/login_ldap.go - Line 92: Would it be better to do this check before any LDAP server interaction?

Yes, moved up.

• File services/api/config/initializers/omniauth_init.rb - Lines 12 & 18 refers to deprecated Rails.configuration.Login["ProviderAppID”] config

Updated.

• On 8f435f4bac86e7ba7dbd9770d2db9bb4db6cf569, the comment is: test LDAP login using a fake LDAP server, but I’m not able to understand how this is done, as the code updates don’t seem to be about that. Is the godap dependency providing the LDAPSuite? A comment explaining how it works would be useful.

Oops, I missed adding the actual _test.go file in that commit. Fixed.

• Is the docker test being run by Jenkins? I’m not seeing any logging about it on "developer-run-tests-remainder” so I suppose it isn’t, do you think it should be run every time? A least on my side, it isn’t too slow, took like 20 seconds.

It was disabled by default (needed go test -tags docker) but yes, it seems quick enough. I've enabled it.

15881-ldap @ 28e68f813bd7c48847c39a9e07d66ff5cf61662d -- developer-run-tests: #1850

It was disabled by default (needed go test -tags docker) but yes, it seems quick enough. I've enabled it.

Oops, jenkins workers don't have docker. Now skipping the docker tests if docker info fails.

15881-ldap @ ca1eb648712232558014d648939868b2a902558a -- developer-run-tests: #1852

Some minor documentation comments:

• File doc/admin/migrating-providers.html.textile.liquid - L16: typo “intead"
• The websocket installation doc page has a duplicated "Restart the API server and controller” section (one of which mentions the SSO)
• Doc file install/install-components.html.textile.liquid can be removed as it seems to have been replaced with install/install-manual-prerequisites.html.textile.liquid (also found because of SSO being mentioned)

And with that, it LGTM. Thanks!

15881-ldap @ f26e4dec04ed9d68a9f826f72b4a9e627ddc4a5c -- developer-run-tests: #1856

I tried making the obvious changes to workbench2:

15881-ldap @ arvados-workbench2|6d2e6d292161d566f54e94f048805569ede8e3d5

I've managed to run unit tests, but not integration tests or actual browser tests.

Updates at arvados-workbench2|366e40d9
Test run: developer-tests-workbench2: #27

Simplified and somehow fixed the code deciding if it should show the login form (integration tests were failing):

diff --git a/src/views/login-panel/login-panel.tsx b/src/views/login-panel/login-panel.tsx
index ba0f584f..f60f032a 100644
--- a/src/views/login-panel/login-panel.tsx
+++ b/src/views/login-panel/login-panel.tsx
@@ -11,6 +11,7 @@ import { ArvadosTheme } from '~/common/custom-theme';
 import { RootState } from '~/store/store';
 import { LoginForm } from '~/views-components/login-form/login-form';
 import Axios from 'axios';
+import { Config } from '~/common/config';

 type CssRules = 'root' | 'container' | 'title' | 'content' | 'content__bolder' | 'button';

@@ -69,6 +70,13 @@ type LoginPanelProps = DispatchProp<any> & WithStyles<CssRules> & {
     passwordLogin: boolean,
 };

+const requirePasswordLogin = (config: Config): boolean => {
+    if (config && config.clusterConfig) {
+        return config.clusterConfig.Login.LDAP.Enable || config.clusterConfig.Login.PAM.Enable || false;
+    }
+    return false;
+};
+
 export const LoginPanel = withStyles(styles)(
     connect((state: RootState) => ({
         remoteHosts: state.auth.remoteHosts,
@@ -76,10 +84,8 @@ export const LoginPanel = withStyles(styles)(
         localCluster: state.auth.localCluster,
         loginCluster: state.auth.loginCluster,
         welcomePage: state.auth.config.clusterConfig.Workbench.WelcomePageHTML,
-        passwordLogin: state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster] &&
-            state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster].clusterConfig.Login.LDAP.Enable ||
-            state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster].clusterConfig.Login.PAM.Enable || false,
-    }))(({ classes, dispatch, remoteHosts, homeCluster, localCluster, loginCluster, welcomePage, passwordLogin }: LoginPanelProps) => {
+        passwordLogin: requirePasswordLogin(state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster]),
+        }))(({ classes, dispatch, remoteHosts, homeCluster, localCluster, loginCluster, welcomePage, passwordLogin }: LoginPanelProps) => {
         const loginBtnLabel = `Log in${(localCluster !== homeCluster && loginCluster !== homeCluster) ? " to "+localCluster+" with user from "+homeCluster : ''}`;

         return (<Grid container justify="center" alignItems="center" 

It seems that reevaluating state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster] sometimes returned undefined and made the code break.

Also, fixed the trailing slashes issue on URLs from #16392, so this makes wb2 work with arvbox again.

Simplified and somehow fixed the code deciding if it should show the login form (integration tests were failing):

Much better, thanks.

It seems that reevaluating state.auth.remoteHostsConfig[state.auth.loginCluster || state.auth.homeCluster] sometimes returned undefined and made the code break.

That is a bit mysterious, isn't it?

Also, fixed the trailing slashes issue on URLs from #16392, so this makes wb2 work with arvbox again.

Excellent, thanks. LGTM.

15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342

Expands comment on SearchFilters config.

Tom Clegg wrote:

15881-ldap @ 95d08c91f6d902054eb9ed4f79cb6bda2c3e8342

Expands comment on SearchFilters config.

LGTM.