acquia / cli

Acquia CLI
GNU General Public License v2.0
42 stars 47 forks source link

CLI-1212: [auth:logout] remove secrets #1643

Closed danepowell closed 9 months ago

danepowell commented 9 months ago

@anavarre can you take this for a spin and provide feedback on the new UX and messaging?

I'm trying to give customers the flexibility to either "unset" the active key (so that they can re-use it by running auth:login again without having to re-input credentials) or to "delete" it entirely.

I'm not sure if "unset" and "delete" are entirely clear in this context. I'm open to feedback.

Delete credentials (the default option):

https://github.com/acquia/cli/assets/1984514/2ddb0fc9-de41-4f79-bc97-68bb775451cb

Unset credentials (interactive):

https://github.com/acquia/cli/assets/1984514/d658c899-02c5-4460-b9e4-6e7faa0dd413

Unset credentials (via --no-delete option)

https://github.com/acquia/cli/assets/1984514/030d3648-3bd2-450e-96b2-775d74d55f65

codecov[bot] commented 9 months ago

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (6917e4a) 91.81% compared to head (258d5a5) 91.52%.

Files Patch % Lines
src/Command/Auth/AuthLoginCommand.php 69.23% 4 Missing :warning:
src/Command/Auth/AuthLogoutCommand.php 80.00% 3 Missing :warning:
src/Command/CommandBase.php 90.90% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1643 +/- ## ============================================ - Coverage 91.81% 91.52% -0.29% - Complexity 1778 1782 +4 ============================================ Files 122 122 Lines 6390 6410 +20 ============================================ Hits 5867 5867 - Misses 523 543 +20 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danepowell commented 9 months ago

Maybe "unselect" is less ambiguous than "unset".

anavarre commented 9 months ago

Thanks for the recordings. That's really helpful to understand what you're trying to achieve here.

It feels to me like we should:

danepowell commented 9 months ago

The subscription isn't relevant here, since the key is attached to the user account, not a subscription.

I'll work to clean up the language.

Multiple prompts aren't necessary since the user clearly intended to do something by running the command in the first place, and unsetting the credentials is easy to reverse by running auth:login again (I can mention that in the command output). If someone really freaks out and wants to abort the command, they can ctrl+c on the prompt and no settings will be changed.

danepowell commented 9 months ago

Give the name of the subscription in bold

Oh I see what you mean though, the API key does have an associated label that we should use.

danepowell commented 9 months ago

I think I've got this the way I like.

I found another significant bug in these commands, which is that they rely on isMachineAuthenticated when they really should only be considering keys in the datastore (since isMachineAuthenticated can also return true based on environment variables).

And that revealed that our tests were just mocking isMachineAuthenticated rather than properly mocking the Cloud datastore and credentials. I don't know how to fix that offhand, so unfortunately I have to remove those tests for now.