Improve token refresh flow #1434
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pietern
approved these changes
May 16, 2024
| output := &bytes.Buffer{} | ||
| c.SetOut(output) | ||
| return c, output | ||
| } |
There was a problem hiding this comment.
FYI there are helpers under internal that do the same and run the command in the background.
There was a problem hiding this comment.
Though you don't need interactive back-and-forth here, so this is also fine.
Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>
pietern
added a commit
that referenced
this pull request
May 22, 2024
CLI: * Add line about Docker installation to README.md ([#1363](#1363)). * Improve token refresh flow ([#1434](#1434)). Bundles: * Upgrade Terraform provider to v1.42.0 ([#1418](#1418)). * Upgrade Terraform provider to v1.43.0 ([#1429](#1429)). * Don't merge-in remote resources during deployments ([#1432](#1432)). * Remove dependency on `ConfigFilePath` from path translation mutator ([#1437](#1437)). * Add `merge.Override` transform ([#1428](#1428)). * Fixed panic when loading incorrectly defined jobs ([#1402](#1402)). * Add more tests for `merge.Override` ([#1439](#1439)). * Fixed seg fault when specifying environment key for tasks ([#1443](#1443)). * Fix conversion of zero valued scalar pointers to a dynamic value ([#1433](#1433)). Internal: * Don't hide commands of services that are already hidden ([#1438](#1438)). API Changes: * Renamed `lakehouse-monitors` command group to `quality-monitors`. * Added `apps` command group. * Renamed `csp-enablement` command group to `compliance-security-profile`. * Renamed `esm-enablement` command group to `enhanced-security-monitoring`. * Added `databricks vector-search-indexes scan-index` command. OpenAPI commit 7eb5ad9a2ed3e3f1055968a2d1014ac92c06fe92 (2024-05-21) Dependency updates: * Bump golang.org/x/text from 0.14.0 to 0.15.0 ([#1419](#1419)). * Bump golang.org/x/oauth2 from 0.19.0 to 0.20.0 ([#1421](#1421)). * Bump golang.org/x/term from 0.19.0 to 0.20.0 ([#1422](#1422)). * Bump github.com/databricks/databricks-sdk-go from 0.39.0 to 0.40.1 ([#1431](#1431)). * Bump github.com/fatih/color from 1.16.0 to 1.17.0 ([#1441](#1441)). * Bump github.com/hashicorp/terraform-json from 0.21.0 to 0.22.1 ([#1440](#1440)). * Bump github.com/hashicorp/terraform-exec from 0.20.0 to 0.21.0 ([#1442](#1442)). * Update Go SDK to v0.41.0 ([#1445](#1445)).
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 22, 2024
CLI: * Add line about Docker installation to README.md ([#1363](#1363)). * Improve token refresh flow ([#1434](#1434)). Bundles: * Upgrade Terraform provider to v1.42.0 ([#1418](#1418)). * Upgrade Terraform provider to v1.43.0 ([#1429](#1429)). * Don't merge-in remote resources during deployments ([#1432](#1432)). * Remove dependency on `ConfigFilePath` from path translation mutator ([#1437](#1437)). * Add `merge.Override` transform ([#1428](#1428)). * Fixed panic when loading incorrectly defined jobs ([#1402](#1402)). * Add more tests for `merge.Override` ([#1439](#1439)). * Fixed seg fault when specifying environment key for tasks ([#1443](#1443)). * Fix conversion of zero valued scalar pointers to a dynamic value ([#1433](#1433)). Internal: * Don't hide commands of services that are already hidden ([#1438](#1438)). API Changes: * Renamed `lakehouse-monitors` command group to `quality-monitors`. * Added `apps` command group. * Renamed `csp-enablement` command group to `compliance-security-profile`. * Renamed `esm-enablement` command group to `enhanced-security-monitoring`. * Added `databricks vector-search-indexes scan-index` command. OpenAPI commit 7eb5ad9a2ed3e3f1055968a2d1014ac92c06fe92 (2024-05-21) Dependency updates: * Bump golang.org/x/text from 0.14.0 to 0.15.0 ([#1419](#1419)). * Bump golang.org/x/oauth2 from 0.19.0 to 0.20.0 ([#1421](#1421)). * Bump golang.org/x/term from 0.19.0 to 0.20.0 ([#1422](#1422)). * Bump github.com/databricks/databricks-sdk-go from 0.39.0 to 0.40.1 ([#1431](#1431)). * Bump github.com/fatih/color from 1.16.0 to 1.17.0 ([#1441](#1441)). * Bump github.com/hashicorp/terraform-json from 0.21.0 to 0.22.1 ([#1440](#1440)). * Bump github.com/hashicorp/terraform-exec from 0.20.0 to 0.21.0 ([#1442](#1442)). * Update Go SDK to v0.41.0 ([#1445](#1445)).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Currently, there are a number of issues with the non-happy-path flows for token refresh in the CLI.
If the token refresh fails, the raw error message is presented to the user, as seen below. This message is very difficult for users to interpret and doesn't give any clear direction on how to resolve this issue.
When logging in again, I've noticed that the timeout for logging in is very short, only 45 seconds. If a user is using a password manager and needs to login to that first, or needs to do MFA, 45 seconds may not be enough time. to an account-level profile, it is quite frustrating for users to need to re-enter account ID information when that information is already stored in the user's
.databrickscfgfile.This PR tackles these two issues. First, the presentation of error messages from
databricks auth tokenis improved substantially by converting theerrorinto a human-readable message. When the refresh token is invalid, it will present a command for the user to run to reauthenticate. If the token fetching failed for some other reason, that reason will be presented in a nice way, providing front-line debugging steps and ultimately redirecting users to file a ticket at this repo if they can't resolve the issue themselves. After this PR, the new error message is:To improve the login flow, this PR modifies
databricks auth loginto auto-complete the account ID from the profile when present. Additionally, it increases the login timeout from 45 seconds to 1 hour to give the user sufficient time to login as needed.To test this change, I needed to refactor some components of the CLI around profile management, the token cache, and the API client used to fetch OAuth tokens. These are now settable in the context, and a demonstration of how they can be set and used is found in
auth_test.go.Separately, this also demonstrates a sort-of integration test of the CLI by executing the Cobra command for
databricks auth tokenfrom tests, which may be useful for testing other end-to-end functionality in the CLI. In particular, I believe this is necessary in order to set flag values (like the--profileflag in this case) for use in testing.Tests
Unit tests cover the unhappy and happy paths using the mocked API client, token cache, and profiler.
Manually tested