Skip to content

Add support for SAML/etc auth #3675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gnprice merged 3 commits into from
Dec 3, 2019
Merged

Add support for SAML/etc auth #3675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb0069a
api: Add external_authentication_methods field to server_settings.
gnprice Nov 1, 2019
adca829
AuthScreen [nfc]: Split auth methods covered by new API vs. not.
gnprice Nov 4, 2019
7ccab7e
AuthScreen: Consume external auth methods, like SAML.
gnprice Nov 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/api/apiTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,4 @@ export type * from './modelTypes';
export type * from './eventTypes';
export type * from './initialDataTypes';

export type {
AuthenticationMethods,
ApiResponseServerSettings,
} from './settings/getServerSettings';
export type * from './settings/getServerSettings';
10 changes: 10 additions & 0 deletions src/api/settings/getServerSettings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,19 @@ export type AuthenticationMethods = {
...
};

export type ExternalAuthenticationMethod = {|
name: string,
display_name: string,
display_icon: string | null,
login_url: string,
signup_url: string,
|};

export type ApiResponseServerSettings = {|
...ApiResponseSuccess,
authentication_methods: AuthenticationMethods,
// external_authentication_methods added for server v2.1
external_authentication_methods?: ExternalAuthenticationMethod[],
email_auth_enabled: boolean,
push_notifications_enabled: boolean,
realm_description: string,
Expand Down
79 changes: 68 additions & 11 deletions src/start/AuthScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ import React, { PureComponent } from 'react';
import { Linking } from 'react-native';
import type { NavigationScreenProp } from 'react-navigation';

import type { AuthenticationMethods, Dispatch, ApiResponseServerSettings } from '../types';
import type {
AuthenticationMethods,
Dispatch,
ExternalAuthenticationMethod,
ApiResponseServerSettings,
} from '../types';
import { IconPrivate, IconGoogle, IconGitHub, IconWindows, IconTerminal } from '../common/Icons';
import type { IconType } from '../common/Icons';
import { connect } from '../react-redux';
Expand Down Expand Up @@ -34,7 +39,8 @@ type AuthenticationMethodDetails = {|
action: 'dev' | 'password' | {| url: string |},
|};

const authentications: AuthenticationMethodDetails[] = [
// Methods that don't show up in external_authentication_methods.
const availableDirectMethods: AuthenticationMethodDetails[] = [
{
name: 'dev',
displayName: 'dev account',
Expand All @@ -53,6 +59,19 @@ const authentications: AuthenticationMethodDetails[] = [
Icon: IconPrivate,
action: 'password',
},
{
// This one might move to external_authentication_methods in the future.
name: 'remoteuser',
displayName: 'SSO',
Icon: IconPrivate,
action: { url: 'accounts/login/sso/' },
},
];

// 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.
const availableExternalMethods: AuthenticationMethodDetails[] = [
{
name: 'google',
displayName: 'Google',
Expand All @@ -74,20 +93,22 @@ const authentications: AuthenticationMethodDetails[] = [
Icon: IconWindows,
action: { url: '/accounts/login/social/azuread-oauth2' },
},
{
name: 'remoteuser',
displayName: 'SSO',
Icon: IconPrivate,
action: { url: 'accounts/login/sso/' },
},
];

const externalMethodIcons = new Map([
['google', IconGoogle],
['github', IconGitHub],
['azuread', IconWindows],
]);
Comment on lines +98 to +102
Copy link
Copy Markdown
Contributor

@rk-for-zulip rk-for-zulip Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information already exists as a slice of the availableExternalMethods table; perhaps it should be extracted from there?

Copy link
Copy Markdown
Member Author

@gnprice gnprice Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do. As a quick experiment just now, I wrote that and it looks like this:

const externalMethodIcons = new Map(
  availableExternalMethods.map(method => [method.name, method.Icon]),
);

Thinking a bit more, though, I think I prefer the version that recites it explicitly. It's true right now that this has the same values as that slice of availableExternalMethods, but:

  • availableExternalMethods is the legacy API, and in general I prefer the implementation of a current API to stand alone without depending on the implementation of a legacy API.
    • Vice versa (legacy depends on current) would be great, but I don't see a trivial way to do that here.
  • As more external methods are added in the future that have natural icons to use, we'd like to add them to this list, but availableExternalMethods won't grow.

Of these only the first is a particularly overriding reason. So if this were more complicated I probably would want to share it, and would just do so in the other direction: arrange for availableExternalMethods (or its use site) to get the data from here, and say YAGNI to the "more in future" point.

(Well, we probably are going to need more in future; what that really means is more like "we'll cross that bridge when we come to it, and it'll be fine.")

But this is so simple, it doesn't seem worth complicating the code.


/** Exported for tests only. */
export const activeAuthentications = (
authenticationMethods: AuthenticationMethods,
externalAuthenticationMethods: ExternalAuthenticationMethod[] | void,
): AuthenticationMethodDetails[] => {
const result = [];
authentications.forEach(auth => {

availableDirectMethods.forEach(auth => {
if (!authenticationMethods[auth.name]) {
return;
}
Expand All @@ -98,6 +119,37 @@ 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;
}

// 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;
Copy link
Copy Markdown
Contributor

@rk-for-zulip rk-for-zulip Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems awkward: the server doesn't actually have to send any particular name or display_name for any particular login_url, so using name in particular to select the icon seems arbitrary.

(I have no constructive suggestions here, unfortunately.)

Copy link
Copy Markdown
Member Author

@gnprice gnprice Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually exactly the sort of thing that name is here for 🙂 . As the docs put it:

[...] name, which is a unique, stable, machine-readable name for the authentication method.

The display name could easily get tweaked in the future, and the login URL certainly has been known to change (see comment about Google auth, above in this file). The name property is here so that when a client has some logic they want to apply to a particular authentication method, there's a reasonable test to hang it off of.


result.push({
name: method.name,
displayName: method.display_name,
Icon,
action: { url: method.login_url },
});
});
}

return result;
};

Expand Down Expand Up @@ -130,8 +182,10 @@ class AuthScreen extends PureComponent<Props> {
}
});

const { serverSettings } = this.props.navigation.state.params;
const authList = activeAuthentications(
this.props.navigation.state.params.serverSettings.authentication_methods,
serverSettings.authentication_methods,
serverSettings.external_authentication_methods,
);
if (authList.length === 1) {
this.handleAuth(authList[0]);
Expand Down Expand Up @@ -187,7 +241,10 @@ class AuthScreen extends PureComponent<Props> {
name={serverSettings.realm_name}
iconUrl={getFullUrl(serverSettings.realm_icon, this.props.realm)}
/>
{activeAuthentications(serverSettings.authentication_methods).map(auth => (
{activeAuthentications(
serverSettings.authentication_methods,
serverSettings.external_authentication_methods,
).map(auth => (
<ZulipButton
key={auth.name}
Copy link
Copy Markdown
Contributor

@rk-for-zulip rk-for-zulip Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name should be unique; but we need to check it anyway, or else this will have undefined behavior.

(... although on second thought, do we have any use for key here at all?)

Copy link
Copy Markdown
Member Author

@gnprice gnprice Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm -- we probably don't! I'll think about that a bit.

If we do keep key, I agree, there should be a check. Thanks for spotting that.

Copy link
Copy Markdown
Member Author

@gnprice gnprice Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't see a point in having key here -- these are just buttons, and have no internal state that could make it relevant to preserve their identities.

On top of which, there's no way for the list of them to change in an update anyway.

Looks like the key prop was added here in 3a279bc, without explanation. I'll just take it out.

Copy link
Copy Markdown
Member Author

@gnprice gnprice Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are just buttons, and have no internal state that could make it relevant to preserve their identities.

As I type this bit into a commit message, I realize it isn't quite true -- if the user has just touched a button so e.g. the Material "ink splash" animation is in progress, that animation state belongs to the button and not to an unrelated button that might appear in the same spot.

... And although it is true that there's no way for the list to change in an update (it comes from a nav param, so it's part of the historical fact of how we navigated here), that feels like a bit of a fragile fact. So now I'm inclined to say that it's good practice to have a key: this is part of a dynamic list, the identity would matter to UI polish if there were insertions or deletions to the list, and it's best not to rely on the more external and fragile fact that the list won't actually change after the component is first rendered.

I'll just add the check that the name is unique, then.

style={styles.halfMarginTop}
Expand Down
61 changes: 55 additions & 6 deletions src/start/__tests__/AuthScreen-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,60 @@

import { activeAuthentications } from '../AuthScreen';

describe('activeAuthentications', () => {
describe('activeAuthentications: external_authentication_methods (server v2.1+ API)', () => {
test('clobbers hardcoded info for same methods', () => {
expect(
activeAuthentications({ google: true }, [
{
name: 'google',
signup_url: '/accounts/register/social/google',
display_icon: '/static/images/landing-page/logos/googl_e-icon.png',
display_name: 'Google',
login_url: '/accounts/login/social/google',
},
]),
).toMatchObject([
{
name: 'google',
displayName: 'Google',
// NB different from hardcoded URL for same method
action: { url: '/accounts/login/social/google' },
},
]);
});

test('supplements internal methods', () => {
expect(activeAuthentications({ password: true, google: true }, [])).toMatchObject([
{ name: 'password' },
]);
});

test('handles example SAML data', () => {
expect(
activeAuthentications({ saml: true }, [
{
name: 'saml:okta',
display_name: 'SAML',
display_icon: null,
login_url: '/accounts/login/social/saml/okta',
signup_url: '/accounts/register/social/saml/okta',
},
]),
).toMatchObject([
{
name: 'saml:okta',
displayName: 'SAML',
action: { url: '/accounts/login/social/saml/okta' },
},
]);
});
});

describe('activeAuthentications: old server API', () => {
test('empty auth methods object result in no available authentications', () => {
const authenticationMethods = {};

const actual = activeAuthentications(authenticationMethods);
const actual = activeAuthentications(authenticationMethods, undefined);

expect(actual).toEqual([]);
});
Expand All @@ -17,7 +66,7 @@ describe('activeAuthentications', () => {
password: true,
};

const actual = activeAuthentications(authenticationMethods);
const actual = activeAuthentications(authenticationMethods, undefined);

expect(actual).toHaveLength(2);
});
Expand All @@ -28,7 +77,7 @@ describe('activeAuthentications', () => {
password: true,
};

const actual = activeAuthentications(authenticationMethods);
const actual = activeAuthentications(authenticationMethods, undefined);

expect(actual).toHaveLength(1);
});
Expand All @@ -44,7 +93,7 @@ describe('activeAuthentications', () => {
remoteuser: true,
};

const actual = activeAuthentications(authenticationMethods);
const actual = activeAuthentications(authenticationMethods, undefined);

expect(actual).toHaveLength(6);
});
Expand All @@ -55,7 +104,7 @@ describe('activeAuthentications', () => {
unknown: true,
};

const actual = activeAuthentications(authenticationMethods);
const actual = activeAuthentications(authenticationMethods, undefined);

expect(actual).toHaveLength(1);
});
Expand Down