Move and refactor LoginRestServlet helper methods

[anoadragon453]

This is split out from #7438, which had gotten rather large.

LoginRestServlet has a couple helper methods, login_submission_legacy_convert and login_id_thirdparty_from_phone. They're primarily used for converting legacy user login submissions to "identifier" dicts (see spec). Identifying information such as usernames or 3PID information used to be top-level in the login body. They're now supposed to be put inside an identifier parameter instead.

#7438's purpose is to allow using the new identifier parameter during User-Interactive Authentication, which is currently handled in AuthHandler. That's why I've moved these helper methods there. I also moved the refactoring of these method from #7438 as they're relevant.

One debatable point is that @clokep mentioned that conversion of the login submission dictionary could be done at the REST layer, instead of in AuthHandler. This could work. There's 6 endpoints calling validate_user_via_ui_auth that I can see. We could do the conversion in each of those, but that sounds like it might duplication code too much to me. Open to feedback on this!

Best reviewed commit-by-commit.

[clokep]

One debatable point is that @clokep mentioned that conversion of the login submission dictionary could be done at the REST layer, instead of in AuthHandler. This could work. There's 6 endpoints calling validate_user_via_ui_auth that I can see. We could do the conversion in each of those, but that sounds like it might duplication code too much to me. Open to feedback on this!

As far as I can tell this does do it in the REST layer? The REST layer (LoginRestServlet specifically) calls the conversion methods.

I think this comment will make more sense if they're used in more places?

[clokep]

It looks like this check is duplicated.

[clokep]

Thanks for splitting this out, by the way! 👍 Much clearer to see just what's changing in these methods.

[anoadragon453]

As far as I can tell this does do it in the REST layer? The REST layer (LoginRestServlet specifically) calls the conversion methods.

I think this comment will make more sense if they're used in more places?

It might've been too early for this comment, but I was thinking about what to do when we hook these helper methods up to UIA stuff. There's a number of REST classes that call validate_user_via_ui_auth, which in the original PR then called these helper methods. But should the REST classes import the helper methods directly, and if so should the helper methods be placed somewhere other than AuthHandler?

That last bit is that question I was getting at, in a roundabout way 😅

[clokep]

But should the REST classes import the helper methods directly

I don't see why not.

and if so should the helper methods be placed somewhere other than AuthHandler?

I'm not sure if there's a better place for them? If there's like an auth utils module that would work too. I find it a bit hard to reason about the move without seeing the other places they'll be used. 😄

I wonder if this PR should focus on refactoring, then we follow it up with another PR which uses them in the 6 places for consistency, then we follow-up with other PRs.

[anoadragon453]

If there's like an auth utils module that would work too.

Yeah, scanning through AuthHandler I suppose that's really what it is already in a way.

I wonder if this PR should focus on refactoring, then we follow it up with another PR which uses them in the 6 places for consistency, then we follow-up with other PRs.

Yep, that's the plan. I just wanted to save needing to move the helper functions to another file again in a separate PR, but I think there's a pretty good case for them at least remaining in this file for now 👍