Remove child proxying for web authentication workflow #2710
Conversation
juanscr
changed the title
|
"Haven't done manual tests yet." |
|
Is everyone in agreement that we can merge this change without first adding any additional UX for users in Safari and/or Firefox who might experience pop-up blocking? |
How does this affect iOS where, from my understanding, the browsers are still Safari underneath? |
Good question! On the dev plan, we opted to start with refactoring this issue mainly so apps can benefit in security sooner, without having to wait on other work being done. We can backtrack that decision ofc, but I still strongly feel having auth as secure as possible is something to stride for. |
By iOS do you mean the native clients or people using Safari to login to Teams and stuff like that? Because native clients would still behave as usual, and they don't have any issue with pop-up blocking. |
Yes, I wanted to get some reviews for this first to test the waters a bit to see if there is a glaring issue in this refactoring. I will do some more thorough manual testing on Monday, as I want to run the tests in different browsers, my phone and in Mac and will update PR with findings. For now, as short-sighted as automated testing can be, at least all the unit tests I coded and the older E2E tests pass which is a good signal that it may haven't break anything. |
|
@AE-MS just updated the PR description with my results of some manual testing. |
I know we had discussed switching child proxying to be opt-in rather than the current state (force-in, I guess) so that if any apps were depending on it, they can still use it for a period of time while the product itself can be secure by default. I guess I wasn't sure whether the same opt-in stance also applied to the auth window change. |
Is it worth getting someone on a Mac to try this to see what happens? I think we know the expected result (pop-up blocked) which is fine but then I guess it goes back to my other comment about whether the opt-in stance also applies to the auth window changes. |
Yes, let me ping someone in engineering if they can run the test by themselves. I wouldn't think that it would get blocked, the test client I am using (of what I noticed) pretty much opens the window "immediately" as there isn't many things executing at once. Therefore, the Safari strict requirement of opening in the time frame of 100 milliseconds would be fulfilled, therefore opening the window. In essence, I tried replicating busy event loop by the chromium cpu throttling but that of course is just an approximation of a real scenario. |
Got it! At least for now, I would think it isn't worth the risk to have opt-in for auth. Is not like that it will break authentication in web entirely, at least for what my research and tests show. There are just some cases in which the event loop and massive requests will end up blocking the pop-up which before it wouldn't have happened. On the contrary, removing child proxying will unequivocally break some apps and we don't want to remove support entirely. |
juanscr
force-pushed
the
juancard/fixAuthFlow
branch
from
c78a8af to
dd7f870
Compare
|
Just left one suggestion in the unit tests for you to consider; might make the code cleaner. |
size-limit report 📦
|
Description
This pull requests secures the
authenticatefunction by removing all code that used child proxying. As such, now both web and non-web hosts will use the same authentication flow, relaying to the parent the full responsibility for executing the authentication capability.Main changes in the PR:
Validation
Validation performed:
Validated through automated testing with unit and E2E tests.
For manual testing, I first tested in Edge/Chromium in a test client by disabling network caching, using a simulated 3G network and changed CPU throttling to the available values (0x, 4x, 6x and 20x slowdown). For all cases, the authentication window opened successfully. On Firefox, there isn't currently a way to throttle CPU, but it does allow for network throttling which I tested good 3G, slow 3G, good 2G and slow 2G. On all cases, the authentication window didn't get blocked. Initial tests on Safari shows the window opening properly.
Unit Tests added:
Yes
End-to-end tests added:
No
Additional Requirements
Change file added:
Yes