From 2ea4dd30114726e69d8f276de451eecebaed55cb Mon Sep 17 00:00:00 2001 From: ossan Date: Fri, 4 Oct 2024 15:06:57 +0200 Subject: [PATCH] review 2024-10-04 --- Makefile | 2 +- Readme.md | 17 ++++------------- cmd/password.go | 5 +++++ go.mod | 6 ++++-- go.sum | 1 + internal/password/password.go | 6 +++++- internal/password/password_test.go | 6 ++++-- internal/util/data.go | 6 ++++++ 8 files changed, 30 insertions(+), 19 deletions(-) diff --git a/Makefile b/Makefile index d3f20f1..7b75bc1 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,6 @@ # Allow environment variable override for running targets within Dockerfile VERSION ?= $(shell git describe --always --dirty --tags | egrep -o '([0-9]+\.){1,2}[0-9]?[a-zA-Z0-9-]+') BUILD := $(shell date +%FT%T%z) -# [x] fetching git tag here LASTTAG := $(shell git fetch --tags && git describe --abbrev=0 --tags) BUILD_FLAGS := @@ -14,6 +13,7 @@ BINARY := password-${VERSION} REQUIRED_COVERAGE ?= 80 test: +# FIXME: the "race" flag could be omitted since it's a tool running from CLI @go test -v -race ./... install: diff --git a/Readme.md b/Readme.md index 2668196..55f26c9 100644 --- a/Readme.md +++ b/Readme.md @@ -11,11 +11,6 @@ It was made with love using ## How to install - - - There are two ways to install the `password` utility ### Method 1: Download a pre-built binary @@ -82,8 +77,6 @@ The purpose of each option is as follows: If you have followed the [Installation instructions](#how-to-install), you can use the following examples. You can check [All available options](#available-options-for-controlling-the-generated-password). - - - To generate a password with default options ```shell @@ -118,18 +111,16 @@ For safety, the code will not remove an exising file, matching the default passw make test ``` - - - - +### How to test it without installing + + +Simply replace `~/go/bin/password` with `go run main.go` and you can try everything described above. ## New Requirements The next you're asked is to create a hierarchy of commands in this way: - - `password` is the master command - - `generate` is the actual command to generate the password - `validate` checks the password against a predefined set of rules and shares those with the user. diff --git a/cmd/password.go b/cmd/password.go index a7ed44a..febc7f3 100644 --- a/cmd/password.go +++ b/cmd/password.go @@ -2,6 +2,7 @@ package cmd import ( "fmt" + // FIXME: use the log/slog pkg which should be the new standard. "log" "os" @@ -65,7 +66,9 @@ func init() { password.DefaultFilePath, fmt.Sprintf("filepath (when %d is provided for %s)", password.ToFile, FlagNameOutput), ) + // BUG: use structured logging with Fields and so on logOpts := log.LstdFlags | log.Lshortfile | log.Ldate | log.Ltime | log.LUTC + // BUG: `Length:0x7, Count:0x5,` printed on the shell is not user-friendly. logger = log.New(os.Stderr, "password generator: ", logOpts) } @@ -89,6 +92,8 @@ func runPasswordGenerator(cmd *cobra.Command, args []string) error { } logMesg := `generating password(s) with the following options %#v` logger.Printf(logMesg, passOptions) + // FIXME: here, it makes sense to refactor the code by taking out the "count" from the Options struct. + // The Options struct describes the characteristics of the password. The "count" is something else. The Generate function should be invoked as many times as we want from the count and every time must return a single password (to make it more reusable). passwords, err := password.Generate(&passOptions) if err != nil { return err diff --git a/go.mod b/go.mod index e51a5fc..ad30538 100644 --- a/go.mod +++ b/go.mod @@ -2,13 +2,15 @@ module amritsingh183/password go 1.22.0 -require github.com/spf13/cobra v1.8.1 +require ( + github.com/spf13/cobra v1.8.1 + github.com/stretchr/testify v1.9.0 +) require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/spf13/pflag v1.0.5 // indirect - github.com/stretchr/testify v1.9.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index c7da2f0..4353b61 100644 --- a/go.sum +++ b/go.sum @@ -12,6 +12,7 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/password/password.go b/internal/password/password.go index 56f1165..203113e 100644 --- a/internal/password/password.go +++ b/internal/password/password.go @@ -1,11 +1,12 @@ package password import ( - "amritsingh183/password/internal/util" "errors" "io" "os" "unsafe" + + "amritsingh183/password/internal/util" ) // // FIXME: let's get rid of this struct. Let's group the parameters of the password generator in another struct with a different name. For the target, you can leverage the interface io.Writer that can be accepted as a parameter of the function that has to write the password somewhere. @@ -40,6 +41,8 @@ func Generate(o *Options) ([][]byte, error) { // Write writes the password to destination. // It closes the file when destination is a file +// FIXME: the signature might be: func Write(w io.writer, passwords []string) error {} +// where io.Writer might be: StdOut, a file handle, a mock, etc. func Write(data [][]byte, o *Options) error { var w io.Writer var err error @@ -56,6 +59,7 @@ func Write(data [][]byte, o *Options) error { } var stringPassword string for _, bytePassword := range data { + // [Q]: can you explain me why are you using so often the "unsafe" pkg? It should be used seldomly. Maybe I'm not getting your thoughts. usPtr := unsafe.Pointer(&bytePassword) strPtr := (*string)(usPtr) stringPassword = *strPtr diff --git a/internal/password/password_test.go b/internal/password/password_test.go index 9ac94f3..204f25a 100644 --- a/internal/password/password_test.go +++ b/internal/password/password_test.go @@ -1,7 +1,6 @@ package password import ( - "amritsingh183/password/internal/util" "bytes" "fmt" "io" @@ -10,6 +9,8 @@ import ( "testing" "unsafe" + "amritsingh183/password/internal/util" + "github.com/stretchr/testify/assert" ) @@ -164,11 +165,12 @@ func TestWrite(t *testing.T) { msg := fmt.Sprintf("Expected password to be of length %d", passwordLength) assert.Equal(t, passwordLength, len(passwd), msg) }) - } // tapStdOut provides mechanism to tap into the stdout // useful for testing only +// FIXME:you can easily take advantage of the io.Writer interface and save the values in memory. +// https://quii.gitbook.io/learn-go-with-tests/go-fundamentals/dependency-injection#write-enough-code-to-make-it-pass type tapStdOut struct { outChan chan string errChan chan error diff --git a/internal/util/data.go b/internal/util/data.go index 66e5b1e..11e5f07 100644 --- a/internal/util/data.go +++ b/internal/util/data.go @@ -60,6 +60,9 @@ func GenerateKey(n int, includeSpecials bool) ([]byte, error) { } // isValid validates the length of key to be generated +// TODO: this could also be replaced by some validation pkg. +// BUG: I'd like to generate unsecure keys. You should allow the users to do so. +// Maybe, you can print a warning that the password is not secure enough and the reason. func IsValidKeyLength(n int) error { if n > MaxKeyLength { return fmt.Errorf("maximum key length is %d but %d was provided", MaxKeyLength, n) @@ -69,6 +72,9 @@ func IsValidKeyLength(n int) error { } return nil } + +// TODO: this is really needed? Or we can solve it with an easier way? +// I mean the key and all of the other stuff. The code will be much more simplified. func generate(n int, letterBytes string) ([]byte, error) { randBytes := make([]byte, 10240) _, err := io.ReadFull(cryptoRand.Reader, randBytes)