Clean up auth code in preparation for adding SAML#3674
Clean up auth code in preparation for adding SAML#3674gnprice merged 12 commits intozulip:masterfrom
Conversation
This takes only two more lines to just write directly than to invoke, and saves having to go refer to another file to see what this means.
This is super closely tied to the AuthScreen component: witness especially the `handler` values, which are interpreted as names of methods on the component (yuck!). So, move it into the same module -- the better to be read and understood together, and to evolve together.
rk-for-zulip
left a comment
rk-for-zulip
left a comment
There was a problem hiding this comment.
A couple of items for your consideration. (It's mergeable as-is, though.)
| type AuthenticationMethodDetails = {| | ||
| method: string, | ||
| name: string, | ||
| displayName: string, | ||
| Icon: IconType, | ||
| handler: string, | ||
| |}; |
There was a problem hiding this comment.
Given the commit message, it sounds like 'id' or 'methodName' might be a better key than 'name'.
(Either way, these two items could use docstrings, or at least docstringishly descriptive comments.)
There was a problem hiding this comment.
Yeah, I agree name is ambiguous on identifier-like name vs. display name. I think it helps that the other name in the pair is unambiguous, though.
To me id would suggest a numeric ID -- like our user IDs, realm IDs, stream IDs, etc. Realms and streams have identifier-like names, even, but then also "IDs" which are numbers. I think methodName doesn't add any information beyond name; to the extent an auth method's "name" could mean its display name, "method name" could just as well. So I don't think either of those would help.
Some jsdoc is a good idea. I'll add some.
There was a problem hiding this comment.
To me
idwould suggest a numeric ID -- like our user IDs, realm IDs, stream IDs, etc.
Ah, that's understandable. And noted. (The meaning I was thinking of was from the DOM: document.getElementById('blorple'), etc.)
There was a problem hiding this comment.
(The meaning I was thinking of was from the DOM:
document.getElementById('blorple'), etc.)
Ah, makes sense! Yeah, within Zulip I think we pretty consistently say "ID" only for a numeric ID.
The one exception I can think of is within the server codebase -- we've managed to keep it out of the API -- where Realm.string_id is an identifier-style string, better known as the "subdomain".
| describe('extractApiKey', () => { | ||
| test('correctly extracts an API key that has been XORed with a OTP', () => { | ||
| const key = 'testing'; | ||
| const otp = 'A8348A93A83493'; | ||
| const encoded = xorHexStrings(asciiToHex(key), otp); | ||
| expect(extractApiKey(encoded, otp)).toBe(key); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test was removed, but doesn't seem to have been replaced in a new location.
I swear I edited this comment after I saw the new test structure.
No action needed or requested here, except maybe a comment in the commit message about the upcoming new tests.
[Edit 2: ... and then I refreshed, and your reply below popped up. ¯\_(ツ)_/¯ ]
There was a problem hiding this comment.
Thanks. That was intentional, but I should mention it in the commit message. (The test just recites the implementation, and I didn't see value in keeping it. Once authFromCallbackUrl gets some tests a couple of commits later, that effectively includes a new test of this code that actually specifies the input.)
The name "method" here is confusing at best: the whole object describes an auth method, and this property is just a sort of identifier-style name for it. Also add a bit of jsdoc on this type and these properties.
We're about to replace this with something structured and reasonable. Start by making a place for that to go, deduplicating the messiness it'll replace.
This lets us stop pulling strings out of a table and invoking the
functions that have their names.
It also lets us treat the four OAuth methods more generically --
the thing to do for each of them differs only by URL, so make the
URLs data instead of embedding them in separate function definitions.
For the type of `action` we could do something more elaborate like
{| type: 'dev' |} | ... | {| type: 'oauth', url: string |}
and that'd be the right thing if this were an API spread widely.
Because it's entirely internal to this module, though, that
doesn't seem necessary.
The thing the various cases of this codepath really have in common isn't OAuth: even for the auth methods that happen to be OAuth underneath, we don't touch any OAuth-related details about them. (And indeed `remoteuser` auth is not OAuth or even much like it; and SAML auth, which we've newly added on the server and will add here soon, isn't OAuth either although it looks similar if you squint.) Rather, what this codepath is really about is that the auth flow is one we don't have a specific implementation of in the app. To deal with that, we hand the user a browser where they can go through it on the web; there's a twist to have the server send them back to the app when done. So, give this code a more appropriately generic name. Also write a comment explaining what it's about.
The `openBrowser` function in particular is far more specific in what it actually means than its bare name suggests.
Its job is wholly specific to this protocol. Also drop a meaningless unit test that just recites the implementation. A proper unit test for this would actually provide the input and expected output... and in an upcoming commit, we'll do exactly that as part of testing the code that this is a tiny helper function for.
For one thing, this makes it possible to write reasonable tests! Also brings the detailed protocol knowledge this encodes into the same module where we have most of the other details of this protocol.
Also add a comment about the one potentially-puzzling wrinkle in its semantics. See f2ee751 where this condition was introduced.
This type `AuthenticationMethods` could in reality have fewer properties (from an older server) or more of them (from a future server with new features... at least in principle, as in practice it's now likely that we'd do something else in that case.) The tests already tested those scenarios; fix the type to match.
|
Thanks @ray-kraesig for the review! Merged, with revisions from your comments. |
2 participants
These are all pure refactors, or tests.
They came as I was refactoring this code to make it suitable to incorporate the new data-driven list of auth methods (#3670) which we'll use for SAML... and then as I studied it harder and unraveled what some parts of it were really doing, and sought to make the names and organization better match that.