Implements limited-input device auth flow, to replace deprecated OOB auth flow. #6107
Conversation
tensorboard/uploader/auth.py
Outdated
| return CustomInstalledAppFlow.from_client_config( | ||
| client_config, scopes=OPENID_CONNECT_SCOPES | ||
| ) | ||
| class _TbUploaderAuthFlow(auth_flows.Flow): |
There was a problem hiding this comment.
Does it make sense for this to extend auth_flows.Flow? It seems that this class is now a higher level non-Flow object that is composed of two different Flows?
It it doesn't need to extend auth_flows.Flow, can we eliminate the layer entirely and move its init() and run() logic into authenticate_user() and make the other functions to be file-level? I feel like the class creates a confusing layer of indirection.
Alternatively, if you don't want to remove the layer of indirection, can we rename it to reflect that it is not actxually an auth_flows.Flow? Maybe _TbUploaderAuthenticator?
There was a problem hiding this comment.
I wanted to be a "clear" update in the sense that it maintains the same general interface with the rest of the code. The class also helps encapsulate several methods that are relevant only for the limited-input device flow, but I agree that it seemed like a weird abstraction to some extent (Ideally it would have been two separate classes, one for the limited-input device flow, and one custom flow that used one or the other in the underlying implementation).
I decided to make this class just about the LimitedInputDeviceFlow, and move the high-level logic of which one to use, to the authenticate_user method.
I don't have a strong opinion on whether we should actually extend that class or not (we don't really need it), but it made sense to me in the sense of exposing the same interface as the other flow. Let me know if you have any preferences.
There was a problem hiding this comment.
Ended up removing the base class. I think it was misleading and erroneous, because the base methods don't really make sense (or work, probably) for this flow.
tensorboard/uploader/auth.py
Outdated
| to `localhost` with an authorization code that would then be received by the | ||
| local web server started here. | ||
|
|
||
| Notably, when the uploaoder is run from a colab notebook, this flow cannot |
tensorboard/uploader/auth.py
Outdated
| local web server started here. | ||
|
|
||
| Notably, when the uploaoder is run from a colab notebook, this flow cannot | ||
| be used, as colab notebooks are run in an environment where a browser is not |
There was a problem hiding this comment.
Is this true? Colab is run in a browser, so the environment has access to a browser. I've never really thought about it before but I suspect the real problem is you can't start a local web server in colab.
On the other hand, I think the statement "a browser is not available" is true for the remote terminal environments.
There was a problem hiding this comment.
I'm not sure, but I rephrased in a less explicit way to avoid making false statements.
tensorboard/uploader/auth.py
Outdated
| to `localhost` with an authorization code that would then be received by the | ||
| local web server started here. | ||
|
|
||
| Notably, when the uploaoder is run from a colab notebook, this flow cannot |
There was a problem hiding this comment.
Thanks for calling out colab specifically. I think you should also call out remote terminals since that is a roughly equal important use case.
tensorboard/uploader/auth.py
Outdated
|
|
||
| If any of the following is true, a different auth flow will be used: | ||
| - the flag `--auth_force_console` is set to true, or | ||
| - a browser is not available (e.g. when running in a colab notebook), or |
There was a problem hiding this comment.
Similar to other comment: I don't think "browser is not available" is actually the problem in the colab scenario.
tensorboard/uploader/auth.py
Outdated
| http://developers.google.com/identity/protocols/oauth2/limited-input-device) | ||
| will be used, in which the user is presented with a URL and a short code | ||
| that they'd need to use to authenticate and authorize access in a separate | ||
| browser or device, after which the uploader will poll for access until the |
There was a problem hiding this comment.
Is the "after which" in relation to "user is presented with a URL" or the user's attempt to "authenticate and authorize access in a separate browser or device". It's not clear in this sentence. The sentence is sort of run-on anyway, so maybe just start a new one and drop "after", saying: "The uploader will poll until the access is granted or rejected . . . "
tensorboard/uploader/auth.py
Outdated
| if resp.status_code in {400, 401}: | ||
| raise ValueError("There must be an error in the request.") | ||
|
|
||
| if "error" in r and r["error"] == "authorization_pending": |
There was a problem hiding this comment.
Can we move this as the 2nd case since it is really not an "error" case. It's just the expected response when user has not yet completed the authorization flow. (Would you also mind adding a comment?)
tensorboard/uploader/auth.py
Outdated
| if "access_token" in r: | ||
| return r | ||
| if "error" in r and r["error"] == "access_denied": | ||
| raise PermissionError("Auth was denied.") |
There was a problem hiding this comment.
Nit: "Access" or "Authorization" instead of "Auth"? Maybe also include "by user" at the end of the sentence?
tensorboard/uploader/auth.py
Outdated
| if "error" in r and r["error"] == "authorization_pending": | ||
| time.sleep(polling_interval) | ||
| elif "error" in r and r["error"] == "slow_down": | ||
| time.sleep(int(polling_interval * 1.5)) |
There was a problem hiding this comment.
Could you leave a comment explaining the reasoning for this reaction to the error code? My initial reaction was that this error should actually stop the program - it would appear to indicate some programming error that we are not honoring polling_interval. But I can imagine some justification for doing it this way instead - I'm just not sure which justification you are using :).
There was a problem hiding this comment.
Added a comment. I don't really expect this to be present in reality, but since it's a retryable error, I opted to handle it somehow.
I was imagining some strange edge-case in which the subsequent request is sent "too quickly" after the interval, and for some reason the server says we're polling too fast. Perhaps this is unrealistic. It shouldn't happen.
tensorboard/uploader/auth.py
Outdated
| time.sleep(int(polling_interval * 1.5)) | ||
| else: | ||
| raise RuntimeError( | ||
| "An unexpected error occurred while authenticating." |
There was a problem hiding this comment.
On line 281 you use "authorization" instead of "authentication". Can we be consistent?
tensorboard/uploader/auth.py
Outdated
| self, auth_response | ||
| ) -> google.oauth2.credentials.Credentials: | ||
|
|
||
| expiration_timestamp = datetime.datetime.utcfromtimestamp( |
There was a problem hiding this comment.
expiration_timestamp should actually be named expiration_datetime (the timestamp is the input to utcfromtimestamp()).
I'm kind of shocked that Credential.expiry is a datetime instead of a timestamp - what could they possibly need from the datetime?
There was a problem hiding this comment.
Yeah. I thought it was a bit weird as well, but this is how they build the credentials here:
github.com/googleapis/google-auth-library-python-oauthlib/blob/main/google_auth_oauthlib/helpers.py#L140
tensorboard/uploader/auth_test.py
Outdated
| ) | ||
| ) | ||
|
|
||
| def test_authenticate_user__no_force_console_override__has_display__uses_installed_app_flow( |
There was a problem hiding this comment.
Trying to get these names to be more concise, I'd recommend just naming this:
test_uses_installed_app_flow(self):
- The test is already called AuthenticateUserTest, so perhaps "authenticate_user" is redundant.
- I also consider the "installed app flow" the happy path so it seems unnecessary to include all the scenarios where it might not be used in the name. It might just be preferable to include a comment in the test about the conditions in which its used.
There was a problem hiding this comment.
Done. Totally agree with the redundant authenticate_user.
For the rest... I think it's not always clear what the "happy path" is or why, and it could even change over time, so I like detailing the conditions for each test instead of assuming or describing one as the happy path. But I guess I agree it can be considerably less readable when trying to include the entire state in the test name, so I made them shorter.
Related to the verbosity of the state, I was trying to avoid using when and and, to make them shorter, but with your suggestions I removed some of the state part, and I think I found a reasonable brief name that allows better readability.
Thanks for the review and all the helpful comments!
tensorboard/uploader/auth_test.py
Outdated
| self.assertTrue(fake_auth_flow.run_local_server_was_called) | ||
| self.mocked_device_auth_flow.assert_not_called() | ||
|
|
||
| def test_authenticate_user__no_force_console_override__no_display__uses_device_flow( |
There was a problem hiding this comment.
Perhaps just name "test_uses_device_flow_when_no_display(self):"
tensorboard/uploader/auth_test.py
Outdated
| self.mocked_device_auth_flow.assert_called_once() | ||
| self.mocked_device_auth_flow.return_value.run.assert_called_once() | ||
|
|
||
| def test_authenticate_user__no_force_console_override__has_display__webbrowser_error__uses_device_flow( |
There was a problem hiding this comment.
Perhaps just name "test_uses_device_flow_on_webbrowser_error(self):"
There was a problem hiding this comment.
Updated to something different, but it's a bit shorter and easier to read.
tensorboard/uploader/auth_test.py
Outdated
| self.mocked_device_auth_flow.assert_called_once() | ||
| self.mocked_device_auth_flow.return_value.run.assert_called_once() | ||
|
|
||
| def test_authenticate_user__force_console_override__uses_device_flow(self): |
There was a problem hiding this comment.
Perhaps just name "test_uses_device_flow_when_force_console(self):"
tensorboard/uploader/auth_test.py
Outdated
|
|
||
| self.flow.run() | ||
|
|
||
| device_params = { |
There was a problem hiding this comment.
Maybe rename expected_device_params and expected_polling_params?
…auth flow. (tensorflow#6107) * Motivation for features / changes The OOB auth flow has been deprecated. We concluded that the limited-input device flow is appropriate for our use case where the uploader runs in an environment where a browser is not available. * Technical description of changes Implements a new auth flow which calls an RPC to fetch a device_code, verification_url and user_code, and asks user to visit the verification_url in another device and enter the user_code; then starts polling for the access token after the user authorizes the access from another device. * Screenshots of UI changes N/A * Detailed steps to verify changes work correctly (as executed by you) - Wrote test script that uses this class, and tested the auth flow and was able to print the credentials. - Wrote tests. * Alternate designs / implementations considered Basically, implementing something similar to this flow or the OOB flow ourselves. (cherry picked from commit 8da06b5)
…auth flow. (tensorflow#6107) * Motivation for features / changes The OOB auth flow has been deprecated. We concluded that the limited-input device flow is appropriate for our use case where the uploader runs in an environment where a browser is not available. * Technical description of changes Implements a new auth flow which calls an RPC to fetch a device_code, verification_url and user_code, and asks user to visit the verification_url in another device and enter the user_code; then starts polling for the access token after the user authorizes the access from another device. * Screenshots of UI changes N/A * Detailed steps to verify changes work correctly (as executed by you) - Wrote test script that uses this class, and tested the auth flow and was able to print the credentials. - Wrote tests. * Alternate designs / implementations considered Basically, implementing something similar to this flow or the OOB flow ourselves. (cherry picked from commit 8da06b5)
…auth flow. (#6107) * Motivation for features / changes The OOB auth flow has been deprecated. We concluded that the limited-input device flow is appropriate for our use case where the uploader runs in an environment where a browser is not available. * Technical description of changes Implements a new auth flow which calls an RPC to fetch a device_code, verification_url and user_code, and asks user to visit the verification_url in another device and enter the user_code; then starts polling for the access token after the user authorizes the access from another device. * Screenshots of UI changes N/A * Detailed steps to verify changes work correctly (as executed by you) - Wrote test script that uses this class, and tested the auth flow and was able to print the credentials. - Wrote tests. * Alternate designs / implementations considered Basically, implementing something similar to this flow or the OOB flow ourselves. (cherry picked from commit 8da06b5)
…auth flow. (tensorflow#6107) * Motivation for features / changes The OOB auth flow has been deprecated. We concluded that the limited-input device flow is appropriate for our use case where the uploader runs in an environment where a browser is not available. * Technical description of changes Implements a new auth flow which calls an RPC to fetch a device_code, verification_url and user_code, and asks user to visit the verification_url in another device and enter the user_code; then starts polling for the access token after the user authorizes the access from another device. * Screenshots of UI changes N/A * Detailed steps to verify changes work correctly (as executed by you) - Wrote test script that uses this class, and tested the auth flow and was able to print the credentials. - Wrote tests. * Alternate designs / implementations considered Basically, implementing something similar to this flow or the OOB flow ourselves.
…auth flow. (tensorflow#6107) * Motivation for features / changes The OOB auth flow has been deprecated. We concluded that the limited-input device flow is appropriate for our use case where the uploader runs in an environment where a browser is not available. * Technical description of changes Implements a new auth flow which calls an RPC to fetch a device_code, verification_url and user_code, and asks user to visit the verification_url in another device and enter the user_code; then starts polling for the access token after the user authorizes the access from another device. * Screenshots of UI changes N/A * Detailed steps to verify changes work correctly (as executed by you) - Wrote test script that uses this class, and tested the auth flow and was able to print the credentials. - Wrote tests. * Alternate designs / implementations considered Basically, implementing something similar to this flow or the OOB flow ourselves.
Motivation for features / changes
The OOB auth flow has been deprecated. We concluded that the limited-input device flow is appropriate for our use case where the uploader runs in an environment where a browser is not available.
Technical description of changes
Implements a new auth flow which calls an RPC to fetch a device_code, verification_url and user_code, and asks user to visit the verification_url in another device and enter the user_code; then starts polling for the access token after the user authorizes the access from another device.
Screenshots of UI changes
N/A
Detailed steps to verify changes work correctly (as executed by you)
Basically, implementing something similar to this flow or the OOB flow ourselves.