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 buf registry whoami command #3416

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

Conversation

doriable
Copy link
Member

@doriable doriable commented Oct 22, 2024

This adds a buf registry whoami command, which checks if you
are logged in to the Buf Schema Registry at a given domain. The domain
defaults buf.build if none is provided.

Fixes #3414

Copy link
Contributor

github-actions bot commented Oct 22, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedOct 24, 2024, 4:47 PM

CHANGELOG.md Outdated
@@ -2,7 +2,8 @@

## [Unreleased]

- No changes yet.
- Add `buf beta registry whoami` command, which checks if you are logged in to the Buf Schema
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we have this start as a beta command and then promote?

Copy link
Member

Choose a reason for hiding this comment

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

No

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this out of beta and directly to buf registry whoami.

Comment on lines 81 to 83
if connectErr := new(connect.Error); errors.As(err, &connectErr) && connectErr.Code() == connect.CodeUnauthenticated {
return fmt.Errorf("Not currently logged in for %s", remote)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Checking for the unauthenticated error code here and returning this error, we get the following output:

$ buf beta registry whoami
Failure: Not currently logged in for buf.build

If we do not check, and simply return what the error wrapper gives us, we get:

$ buf beta registry whoami
Failure: you are not authenticated for buf.build. Set the BUF_TOKEN environment variable or run "buf registry login", using a Buf API token as the password. For details, visit https://buf.build/docs/bsr/authentication

Both clearly indicate to the user they are unauthenticated, so they both seem fine to me, it just depends on what we want the UX to be.

if connectErr := new(connect.Error); errors.As(err, &connectErr) && connectErr.Code() == connect.CodeUnauthenticated {
return fmt.Errorf("Not currently logged in for %s", remote)
}
return err
Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, we aren't really differentiating between unauthenticated vs. other error states, everything exits 1 unless authenticated (which exits 0).

The error message will indicate the nature of the error, for example, in the case of an invalid remote:

$ buf beta registry whoami fake.address
Failure: the server hosted at that remote is unavailable. Are you sure "fake.address" is a valid remote address?

But we do not do any special handling for the errors.

@doriable doriable marked this pull request as ready for review October 22, 2024 22:03
type flags struct{}

func newFlags() *flags {
return &flags{}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about a --format=json for accessing the username?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this! I can add this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added --format=json (and --format=text will print the "logged in as" statement as before).

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

Probably good to go

@@ -248,6 +248,14 @@ func NewOrganizationEntity(organization *ownerv1.Organization, remote string) En
}
}

func NewUserEntity(user *registryv1alpha1.User) Entity {
Copy link
Member

Choose a reason for hiding this comment

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

No godoc

formatFlagName = "format"
)

func NewCommand(
Copy link
Member

Choose a reason for hiding this comment

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

No godoc

}
user := currentUserResponse.Msg.User
if user == nil {
return errors.New("No user found for provided token")
Copy link
Member

Choose a reason for hiding this comment

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

What am I supposed to do with this as a user? What provided token? Where did the provided token come from? This error message doesn't help me much.

Copy link
Member Author

Choose a reason for hiding this comment

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

This mirrors the error that is returned from buf registry login when we check the token:

if user == nil {
return errors.New("no user found for provided token")
}

We can prompt the user to get a new token using buf registry login. If that makes sense, then I can update the error message on both sides.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't provide a token though. Like as a user, this command does not accept a token, so it doesn't make sense to tell the user that their provided token didn't result in a user.

I get that this is the error message for the other one, but this just means both need to be updated, and should in no way reference tokens, as the user does not interact with tokens (typically) via either this command of buf registry login.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is fair -- I left the buf registry login error as-is, since it's part of a validation flow where the token is provided through the command. For this one, I provided instructions to the user to refresh their credentials and to check the env var if set.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't make sense to me - the error message is just as confusing for buf registry login.

@bufdev
Copy link
Member

bufdev commented Oct 23, 2024

Adjust the title and description

@doriable doriable changed the title Add buf beta registry whoami command Add buf registry whoami command Oct 23, 2024
user := currentUserResponse.Msg.User
if user == nil {
return fmt.Errorf(
`No valid user found for login credentials. Run %q to refresh your credentials. If you have %s environment variable set, ensure that the token is valid.`,
Copy link
Member

@bufdev bufdev Oct 24, 2024

Choose a reason for hiding this comment

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

  • "If you have BUF_TOKEN environment variable set" isn't proper english, I believe you mean "If you have the BUF_TOKEN environment variable set".
  • "Run 'buf registry login' to refresh your credentials" isn't correct. If someone runs "buf registry whoami acme.buf.dev", they should be instructed to run "buf registry login acme.buf.dev".
  • Change "No valid user found for login credentials" to "No user is logged in to %s", with the hostname being put in there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Errors are now reported as:

$ buf registry whoami
Failure: No user is logged in to buf.build. Run "buf registry login" to refresh your credentials. If you have the BUF_TOKEN environment variable set, ensure that the token is valid.
exit status 1
$ buf registry whoami bufbuild.internal
Failure: No user is logged in to bufbuild.internal. Run "buf registry login bufbuild.internal" to refresh your credentials. If you have the BUF_TOKEN environment variable set, ensure that the token is valid.
exit status 1

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.

Allow a user to determine if they are logged into the BSR.
3 participants