Add support for SAML/etc auth

[gnprice]

Most of the commits in this branch are just #3674 ; the last few are the part specific to this PR.

This is a draft because I haven't yet had a chance to test it end to end. Barring surprises there, though, I think the code is basically ready.

Fixes #3670 .

[gnprice]

@timabbott @mateuszmandera I'd like to test this against a server that has SAML auth. How would you recommend doing that?

[mateuszmandera]

@gnprice Perhaps Tim will have a better solution to this, but I got something running that can be used now:

1. You should have an activation email (to your @zulipchat address) to our test Okta instance - once you activate your account there, you should be able to:
2. I configured SAML on my test production server at www.gentoox.net. You can, through SAML authenticatoin that will use the Okta account from above, sign up and then log in there and do the testing.

[timabbott]

That sounds like a fine way to do the end-to-end SAML test.

[gnprice]

I've now come back to this, and successfully tested it end to end! (With @mateuszmandera 's very helpful test server mentioned above.)

So I've cleaned up the branch (commit messages, another test case, tweak some naming and a comment) and it's now ready for review.

NB that only the last three commits are really part of this branch itself; the ones before that are #3674 . (Merged #3674 .)

[rk-for-zulip]

The first commit message could use a reference to the Zulip server commit (and/or PR) in which the external_authentication_methods field landed.

Other than that, LGTM.

[rk-for-zulip]

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

[gnprice]

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.

[rk-for-zulip]

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.)

[gnprice]

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.

[rk-for-zulip]

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?)

[gnprice]

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.

[gnprice]

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.

[gnprice]

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.

[gnprice]

Thanks for the review!

Just pushed an update which adds the unique-name check, and adds to the first commit message the information you asked for there. Based on your LGTM, I'll go ahead and merge.