api types: Make external_authentication_methods required; present sin…

…ce 2.1 And simplify downstream.
chrisbobbe · Jan 20, 2024 · 9f09e42 · 9f09e42
1 parent d27668e
commit 9f09e42
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 119 deletions.
diff --git a/src/api/settings/getServerSettings.js b/src/api/settings/getServerSettings.js
@@ -29,9 +29,7 @@ export type ExternalAuthenticationMethod = {|
 type ApiResponseServerSettings = {|
   ...$Exact<ApiResponseSuccess>,
   authentication_methods: AuthenticationMethods,
-
-  // TODO(server-2.1): Mark this as required; simplify downstream.
-  external_authentication_methods?: $ReadOnlyArray<ExternalAuthenticationMethod>,
+  external_authentication_methods: $ReadOnlyArray<ExternalAuthenticationMethod>,
 
   // TODO(server-3.0): New in FL 1. When absent, equivalent to 0.
   zulip_feature_level?: number,

diff --git a/src/start/AuthScreen.js b/src/start/AuthScreen.js
@@ -83,34 +83,6 @@ const availableDirectMethods: $ReadOnlyArray<AuthenticationMethodDetails> = [
   },
 ];
 
-// Methods that are covered in external_authentication_methods by servers
-// which have that key (Zulip Server v2.1+).  We refer to this array for
-// servers that don't.
-// TODO(server-2.1): Simplify this away.
-const availableExternalMethods: $ReadOnlyArray<AuthenticationMethodDetails> = [
-  {
-    name: 'google',
-    displayName: 'Google',
-    Icon: IconGoogle,
-    // Server versions through 2.0 accept only this URL for Google auth.
-    // Since server commit 2.0.0-2478-ga43b231f9 , both this URL and the new
-    // accounts/login/social/google are accepted; see zulip/zulip#13081 .
-    action: { url: 'accounts/login/google/' },
-  },
-  {
-    name: 'github',
-    displayName: 'GitHub',
-    Icon: IconGitHub,
-    action: { url: 'accounts/login/social/github' },
-  },
-  {
-    name: 'azuread',
-    displayName: 'Azure AD',
-    Icon: IconWindows,
-    action: { url: '/accounts/login/social/azuread-oauth2' },
-  },
-];
-
 const externalMethodIcons = new Map([
   ['google', IconGoogle],
   ['github', IconGitHub],
@@ -121,10 +93,15 @@ const externalMethodIcons = new Map([
 /** Exported for tests only. */
 export const activeAuthentications = (
   authenticationMethods: AuthenticationMethods,
-  externalAuthenticationMethods: $ReadOnlyArray<ExternalAuthenticationMethod> | void,
+  externalAuthenticationMethods: $ReadOnlyArray<ExternalAuthenticationMethod>,
 ): $ReadOnlyArray<AuthenticationMethodDetails> => {
   const result = [];
 
+  // A server might intend some of these, such as 'dev' or 'password', but
+  // omit them in external_authentication_methods. The only sign that
+  // they're intended is their presence in authentication_methods… even
+  // though that's marked as deprecated in 2.1. Discussion:
+  //   https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/audit.20for.20change.20entries.20vs.2E.20central.20changelog/near/1404115
   availableDirectMethods.forEach(auth => {
     if (!authenticationMethods[auth.name]) {
       return;
@@ -137,35 +114,25 @@ export const activeAuthentications = (
     result.push(auth);
   });
 
-  if (!externalAuthenticationMethods) {
-    // Server doesn't speak new API; get these methods from the old one.
-    availableExternalMethods.forEach(auth => {
-      if (authenticationMethods[auth.name]) {
-        result.push(auth);
-      }
-    });
-  } else {
-    // We have info from new API; ignore old one for these methods.
-    externalAuthenticationMethods.forEach(method => {
-      if (result.some(({ name }) => name === method.name)) {
-        // Ignore duplicate.
-        return;
-      }
+  externalAuthenticationMethods.forEach(method => {
+    if (result.some(({ name }) => name === method.name)) {
+      // Ignore duplicate.
+      return;
+    }
 
-      // The server provides icons as image URLs; but we have our own built
-      // in, which we don't have to load and can color to match the button.
-      // TODO perhaps switch to server's, for the sake of SAML where ours is
-      //   generic and the server may have a more specific one.
-      const Icon = externalMethodIcons.get(method.name) ?? IconPrivate;
-
-      result.push({
-        name: method.name,
-        displayName: method.display_name,
-        Icon,
-        action: { url: method.login_url },
-      });
+    // The server provides icons as image URLs; but we have our own built
+    // in, which we don't have to load and can color to match the button.
+    // TODO perhaps switch to server's, for the sake of SAML where ours is
+    //   generic and the server may have a more specific one.
+    const Icon = externalMethodIcons.get(method.name) ?? IconPrivate;
+
+    result.push({
+      name: method.name,
+      displayName: method.display_name,
+      Icon,
+      action: { url: method.login_url },
     });
-  }
+  });
 
   return result;
 };

diff --git a/src/start/__tests__/AuthScreen-test.js b/src/start/__tests__/AuthScreen-test.js
@@ -50,63 +50,3 @@ describe('activeAuthentications: external_authentication_methods (server v2.1+ A
     ]);
   });
 });
-
-// TODO(server-2.1): Delete this (and the logic it tests.)
-describe('activeAuthentications: old server API', () => {
-  test('empty auth methods object result in no available authentications', () => {
-    const authenticationMethods = {};
-
-    const actual = activeAuthentications(authenticationMethods, undefined);
-
-    expect(actual).toEqual([]);
-  });
-
-  test('two auth methods enabled result in two-item list', () => {
-    const authenticationMethods = {
-      dev: true,
-      password: true,
-    };
-
-    const actual = activeAuthentications(authenticationMethods, undefined);
-
-    expect(actual).toHaveLength(2);
-  });
-
-  test('ldap and password are handled the same so do not duplicate them', () => {
-    const authenticationMethods = {
-      ldap: true,
-      password: true,
-    };
-
-    const actual = activeAuthentications(authenticationMethods, undefined);
-
-    expect(actual).toHaveLength(1);
-  });
-
-  test('recognizes all auth methods and returns them as a list with details', () => {
-    const authenticationMethods = {
-      dev: true,
-      github: true,
-      azuread: true,
-      google: true,
-      ldap: true,
-      password: true,
-      remoteuser: true,
-    };
-
-    const actual = activeAuthentications(authenticationMethods, undefined);
-
-    expect(actual).toHaveLength(6);
-  });
-
-  test('only recognized auth methods are returned while the unknown are ignored', () => {
-    const authenticationMethods = {
-      password: true,
-      unknown: true,
-    };
-
-    const actual = activeAuthentications(authenticationMethods, undefined);
-
-    expect(actual).toHaveLength(1);
-  });
-});