Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logout sub command #1737

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Conversation

RicNord
Copy link

@RicNord RicNord commented Sep 1, 2024

Changes

This PR includes a new sub cmd: auth logout [PROFILE]. That will "logout" a specified profile, similar to az logout.

Logout process looks like this:

  1. Determine profile
  2. Load config file (Default: ~/.databrickscfg) and tokenCache (Default: ~/.databricks/token-cache.json)
  3. Remove OAuth token it if exist
    4. Overwrite profile section in config file, but without any sensitive values, PAT token etc.

Tests

  • Unit test has been written for added functionality.
  • Local execution of built bin successful

@RicNord
Copy link
Author

RicNord commented Sep 4, 2024

Hi,

Please dont hesitate to give feedback or any ideas of how to improve the PR, in case you have that! I would be happy to implement suggested changes, or elaborate further on the PR :)

@pietern
Copy link
Contributor

pietern commented Sep 23, 2024

@RicNord Thank you for this contribution!

We require a CLA to accept contributions. If you're open to signing one, could you send an email to [email protected], and I'll kickstart the process (this is a manual process still).

The new command looks great. As for the scope, I think we shouldn't rewrite the databrickscfg file. I'd rather keep parity between login/logout and focus exclusively on OAuth. If login supported non-OAuth profiles then it would make sense.

cmd/auth/logout.go Show resolved Hide resolved
libs/auth/cache/cache.go Outdated Show resolved Hide resolved
libs/auth/cache/file.go Outdated Show resolved Hide resolved
if err != nil {
return fmt.Errorf("marshal: %w", err)
}
return os.WriteFile(c.fileLocation, raw, ownerReadWrite)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an unexported function to write the configuration to disk?

It can be shared between Store and Delete.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in commit d037ec3

Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping @pietern. I took a look at Azure CLI's implementation as a reference (https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/_profile.py#L308), where they do actually remove the metadata about those subscriptions that the user is logging out of. It's also convenient in that it means that users don't have to manually modify the .databrickscfg file directly, which isn't very user-friendly. Even if login only supports the U2M OAuth flow, it might be nice that logout cleans up everything about the profile. I think the only real difference is that a user could login via a profile name, rather than needing to type the host (and account ID, if necessary).

cmd/auth/logout.go Outdated Show resolved Hide resolved
@RicNord
Copy link
Author

RicNord commented Sep 24, 2024

@RicNord Thank you for this contribution!

We require a CLA to accept contributions. If you're open to signing one, could you send an email to [email protected], and I'll kickstart the process (this is a manual process still).

The new command looks great. As for the scope, I think we shouldn't rewrite the databrickscfg file. I'd rather keep parity between login/logout and focus exclusively on OAuth. If login supported non-OAuth profiles then it would make sense.

Signed the CLA :)

Please let me know how to proceed with the scope of the PR!

@RicNord
Copy link
Author

RicNord commented Oct 1, 2024

Hi @pietern and @mgyucht ,

I have signed the CLA :) Does it look good on your side?

Please let me know how you want to proceed with the scope of the PR, so I can make potential adjustments!

@RicNord
Copy link
Author

RicNord commented Oct 6, 2024

Hi @pietern and @mgyucht, commit 865964e reduces the scope of the logout command to only remove the OAuth token from token-cache. Please let me know if it looks good to you, and if there is something else I need to do!

@RicNord RicNord requested a review from pietern October 6, 2024 21:53
@RicNord RicNord requested a review from mgyucht October 6, 2024 21:53
Copy link

github-actions bot commented Nov 6, 2024

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 1737
  • Commit SHA: ca08796f77e577ef26801bef0d1e651f9728fae3

Checks will be approved automatically on success.

@RicNord
Copy link
Author

RicNord commented Nov 6, 2024

Hi @pietern @mgyucht do you have any updates or feedback regarding this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants