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

Update example code to call Transaction.End #17

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

msteinert
Copy link
Owner

No description provided.

Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

Looks good, just a nit.

example_test.go Outdated
switch s {
case pam.PromptEchoOff:
fmt.Print(msg)
fmt.Printf("%s", msg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fmt.Printf("%s", msg)
fmt.Printf("%s: ", msg)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I know this looks bizarre, but on my machine (running Arch), I get the following values for the message string:

  1. Echo on: login: (lowercase, colon, no trailing space)
  2. Echo off: Password: (uppercase, colon, traliing space)

Do you get a different result? I guess maybe I should just leave it as it was previously since this probably depends on the PAM module running on your specific system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, definitely... It's up to the module to define those..

It's quite weird that it does assumptions on the UI is running though, since a module could message can be print anywhere (including a non-terminal UI), so it's not supposed to send that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding the space is fine in any case though... Since in the password case we won't see anything printing anyways.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I suspect most people ignore the message strings and print whatever they want instead. I'll revert those lines.

example_test.go Outdated
pw, err := term.ReadPassword(int(os.Stdin.Fd()))
if err != nil {
return "", err
}
fmt.Println()
return string(pw), nil
case pam.PromptEchoOn:
fmt.Print(msg)
fmt.Printf("%s ", msg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fmt.Printf("%s ", msg)
fmt.Printf("%s: ", msg)

@msteinert msteinert merged commit 6bce85f into master Nov 30, 2023
6 checks passed
@msteinert msteinert deleted the update-example branch November 30, 2023 18:58
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.

2 participants