From 6d9a4fa593a9c8194b71b6c26fad3138fd31a7ea Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Fri, 3 Dec 2021 19:02:01 +0100 Subject: [PATCH 1/8] Read config from env --- cli/config/init.go | 9 +-- internal/config/config.go | 152 +++++++++++++++++++++++++++++++++---- internal/config/default.go | 4 +- 3 files changed, 142 insertions(+), 23 deletions(-) diff --git a/cli/config/init.go b/cli/config/init.go index 3002a7ee..82e51046 100644 --- a/cli/config/init.go +++ b/cli/config/init.go @@ -34,11 +34,6 @@ import ( "github.com/spf13/viper" ) -const ( - clientIDLen = 32 - clientSecretLen = 64 -) - var initFlags struct { destDir string overwrite bool @@ -122,7 +117,7 @@ func paramsPrompt() (id, key string, err error) { prompt := promptui.Prompt{ Label: "Please enter the Client ID", Validate: func(s string) error { - if len(s) != clientIDLen { + if len(s) != config.ClientIDLen { return errors.New("client-id not valid") } return nil @@ -137,7 +132,7 @@ func paramsPrompt() (id, key string, err error) { Mask: '*', Label: "Please enter the Client Secret", Validate: func(s string) error { - if len(s) != clientSecretLen { + if len(s) != config.ClientSecretLen { return errors.New("client secret not valid") } return nil diff --git a/internal/config/config.go b/internal/config/config.go index 1d8f212f..719f09df 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -25,6 +25,13 @@ import ( "github.com/spf13/viper" ) +const ( + ClientIDLen = 32 + ClientSecretLen = 64 + + EnvPrefix = "ARDUINO_CLOUD" +) + // Config contains all the configuration parameters // known by arduino-cloud-cli. type Config struct { @@ -32,53 +39,170 @@ type Config struct { Secret string `map-structure:"secret"` // Secret ID of the user, unique for each Client ID } -// Retrieve returns the actual parameters contained in the -// configuration file, if any. Returns error if no config file is found. +// Validate the config +// If config is not valid, it returns an error explaining the reason +func (c *Config) Validate() error { + if len(c.Client) != ClientIDLen { + return fmt.Errorf( + "client id not valid, expected len %d but got %d", + ClientIDLen, + len(c.Client), + ) + } + if len(c.Secret) != ClientSecretLen { + return fmt.Errorf( + "client secret not valid, expected len %d but got %d", + ClientSecretLen, + len(c.Secret), + ) + } + return nil +} + +// IsEmpty checks if config has no params set +func (c *Config) IsEmpty() bool { + if len(c.Client) != 0 { + return false + } + if len(c.Secret) != 0 { + return false + } + return true +} + +// Retrieve looks for configuration parameters in +// environment variables or in configuration file +// Returns error if no config is found func Retrieve() (*Config, error) { + // Config extracted from environment has highest priority + c, err := fromEnv() + if err != nil { + return nil, fmt.Errorf("reading config from environment variables: %w", err) + } + // Return the config only if it has been found + if c != nil { + return c, nil + } + + c, err = fromFile() + if err != nil { + return nil, fmt.Errorf("reading config from file: %w", err) + } + if c != nil { + return c, nil + } + + return nil, fmt.Errorf( + "config has not been found neither in environment variables " + + "nor in the current directory, its parents or in arduino15", + ) +} + +// fromFile looks for a configuration file +// If a config file is not found, it returns a nil config without raising errors. +// If invalid config file is found, it returns an error. +func fromFile() (*Config, error) { + // Looks for a configuration file configDir, err := searchConfigDir() if err != nil { return nil, fmt.Errorf("can't get config directory: %w", err) } + // Return nil config if no config file is found + if configDir == nil { + return nil, nil + } v := viper.New() v.SetConfigName(Filename) - v.AddConfigPath(configDir) + v.AddConfigPath(*configDir) err = v.ReadInConfig() if err != nil { - err = fmt.Errorf("%s: %w", "retrieving config file", err) + err = fmt.Errorf( + "config file found at %s but cannot read its content: %w", + *configDir, + err, + ) return nil, err } conf := &Config{} - v.Unmarshal(conf) + err = v.Unmarshal(conf) + if err != nil { + return nil, fmt.Errorf( + "config file found at %s but cannot unmarshal it: %w", + *configDir, + err, + ) + } + if err = conf.Validate(); err != nil { + return nil, fmt.Errorf( + "config file found at %s but is not valid: %w", + *configDir, + err, + ) + } + return conf, nil +} + +// fromEnv looks for configuration credentials in environment variables. +// If credentials are not found, it returns a nil config without raising errors. +// If invalid credentials are found, it returns an error. +func fromEnv() (*Config, error) { + v := viper.New() + SetDefaults(v) + v.SetEnvPrefix("ARDUINO_CLOUD") + v.AutomaticEnv() + + conf := &Config{} + err := v.Unmarshal(conf) + if err != nil { + return nil, fmt.Errorf("cannot unmarshal config from environment variables: %w", err) + } + + if conf.IsEmpty() { + return nil, nil + } + + if err = conf.Validate(); err != nil { + return nil, fmt.Errorf( + "config retrieved from environment variables with prefix '%s' are not valid: %w", + EnvPrefix, + err, + ) + } return conf, nil } -func searchConfigDir() (string, error) { +// searchConfigDir configuration file in different directories in the following order: +// current working directory, parents of the current working directory, arduino15 default directory +// Returns a nil string if no config file has been found, without raising errors +// Returns an error if any problem is encountered during the file research which prevents +// to understand whether a config file exists or not +func searchConfigDir() (*string, error) { // Search in current directory and its parents. cwd, err := paths.Getwd() if err != nil { - return "", err + return nil, err } // Don't let bad naming mislead you, cwd.Parents()[0] is cwd itself so // we look in the current directory first and then on its parents. for _, path := range cwd.Parents() { if path.Join(Filename+".yaml").Exist() || path.Join(Filename+".json").Exist() { - return path.String(), nil + p := path.String() + return &p, nil } } // Search in arduino's default data directory. arduino15, err := arduino.DataDir() if err != nil { - return "", err + return nil, err } if arduino15.Join(Filename+".yaml").Exist() || arduino15.Join(Filename+".json").Exist() { - return arduino15.String(), nil + p := arduino15.String() + return &p, nil } - return "", fmt.Errorf( - "didn't find config file in the current directory, its parents or in %s", - arduino15.String(), - ) + // Didn't find config file in the current directory, its parents or in arduino15" + return nil, nil } diff --git a/internal/config/default.go b/internal/config/default.go index ee0698a3..d593d317 100644 --- a/internal/config/default.go +++ b/internal/config/default.go @@ -26,7 +26,7 @@ var ( // SetDefaults sets the default values for configuration keys. func SetDefaults(settings *viper.Viper) { // Client ID - settings.SetDefault("client", "xxxxxxxxxxxxxx") + settings.SetDefault("client", "") // Secret - settings.SetDefault("secret", "xxxxxxxxxxxxxx") + settings.SetDefault("secret", "") } From 95f709cc00d761b671f019767623a2de0af37c72 Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Fri, 3 Dec 2021 19:02:31 +0100 Subject: [PATCH 2/8] Add test --- internal/config/config_test.go | 273 +++++++++++++++++++++++++++++++++ 1 file changed, 273 insertions(+) create mode 100644 internal/config/config_test.go diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 00000000..e44e2726 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,273 @@ +// This file is part of arduino-cloud-cli. +// +// Copyright (C) 2021 ARDUINO SA (http://www.arduino.cc/) +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published +// by the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package config + +import ( + "os" + "testing" + + "encoding/json" + + "github.com/google/go-cmp/cmp" +) + +func TestRetrieve(t *testing.T) { + var ( + validSecret = "qaRZGEbnQNNvmaeTLqy8Bxs22wLZ6H7obIiNSveTLPdoQuylANnuy6WBOw16XoqH" + validClient = "CQ4iZ5sebOfhGRwUn3IV0r1YFMNrMTIx" + validConfig = &Config{validClient, validSecret} + invalidConfig = &Config{"", validSecret} + clientEnv = EnvPrefix + "_CLIENT" + secretEnv = EnvPrefix + "_SECRET" + clientEnvBackup *string + secretEnvBackup *string + ) + + // Preserve user environment variables when executing this test + _ = func() { + if c, ok := os.LookupEnv(clientEnv); ok { + clientEnvBackup = &c + } + if s, ok := os.LookupEnv(secretEnv); ok { + secretEnvBackup = &s + } + } + _ = func() { + if clientEnvBackup != nil { + os.Setenv(clientEnv, *clientEnvBackup) + clientEnvBackup = nil + } + if secretEnvBackup != nil { + os.Setenv(secretEnv, *secretEnvBackup) + secretEnvBackup = nil + } + } + + tests := []struct { + name string + pre func() + post func() + wantedConfig *Config + wantedErr bool + }{ + { + name: "valid config written in env", + pre: func() { + // pushEnv() + os.Setenv(clientEnv, validConfig.Client) + os.Setenv(secretEnv, validConfig.Secret) + }, + post: func() { + os.Unsetenv(clientEnv) + os.Unsetenv(secretEnv) + // popEnv() + }, + wantedConfig: validConfig, + wantedErr: false, + }, + + { + name: "invalid config written in env", + pre: func() { + // pushEnv() + os.Setenv(clientEnv, validConfig.Client) + os.Setenv(secretEnv, "") + }, + post: func() { + os.Unsetenv(clientEnv) + os.Unsetenv(secretEnv) + // popEnv() + }, + wantedConfig: nil, + wantedErr: true, + }, + + { + name: "valid config written in parent of cwd", + pre: func() { + parent := "test-parent" + cwd := "test-parent/test-cwd" + os.MkdirAll(cwd, os.FileMode(0777)) + // Write valid config in parent dir + os.Chdir(parent) + b, _ := json.Marshal(validConfig) + os.WriteFile(Filename+".json", b, os.FileMode(0777)) + // Cwd has no config file + os.Chdir("test-cwd") + }, + post: func() { + os.Chdir("../..") + os.RemoveAll("test-parent") + }, + wantedConfig: validConfig, + wantedErr: false, + }, + + { + name: "invalid config written in cwd, ignore config of parent dir", + pre: func() { + parent := "test-parent" + cwd := "test-parent/test-cwd" + os.MkdirAll(cwd, os.FileMode(0777)) + // Write valid config in parent dir + os.Chdir(parent) + b, _ := json.Marshal(validConfig) + os.WriteFile(Filename+".json", b, os.FileMode(0777)) + // Write invalid config in cwd + os.Chdir("test-cwd") + b, _ = json.Marshal(invalidConfig) + os.WriteFile(Filename+".json", b, os.FileMode(0777)) + }, + post: func() { + os.Chdir("../..") + os.RemoveAll("test-parent") + os.Unsetenv(clientEnv) + os.Unsetenv(secretEnv) + }, + wantedConfig: nil, + wantedErr: true, + }, + + { + name: "invalid config written in env, ignore valid config of cwd", + pre: func() { + cwd := "test-cwd" + os.MkdirAll(cwd, os.FileMode(0777)) + // Write valid config in cwd + os.Chdir(cwd) + b, _ := json.Marshal(validConfig) + os.WriteFile(Filename+".json", b, os.FileMode(0777)) + // Write invalid config in env + os.Setenv(clientEnv, validConfig.Client) + os.Setenv(secretEnv, "") + }, + post: func() { + os.Chdir("..") + os.RemoveAll("test-cwd") + }, + wantedConfig: nil, + wantedErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.pre() + got, err := Retrieve() + tt.post() + + if tt.wantedErr && err == nil { + t.Errorf("Expected an error, but got nil") + } + if !tt.wantedErr && err != nil { + t.Errorf("Expected nil error, but got: %v", err) + } + + if !cmp.Equal(got, tt.wantedConfig) { + t.Errorf("Wrong config received, diff:\n%s", cmp.Diff(tt.wantedConfig, got)) + } + }) + } +} + +func TestValidate(t *testing.T) { + var ( + validSecret = "qaRZGEbnQNNvmaeTLqy8Bxs22wLZ6H7obIiNSveTLPdoQuylANnuy6WBOw16XoqH" + validClient = "CQ4iZ5sebOfhGRwUn3IV0r1YFMNrMTIx" + ) + tests := []struct { + name string + config *Config + valid bool + }{ + { + name: "valid config", + config: &Config{ + Client: validClient, + Secret: validSecret, + }, + valid: true, + }, + { + name: "invalid client id", + config: &Config{Client: "", Secret: validSecret}, + valid: false, + }, + { + name: "invalid client secret", + config: &Config{Client: validClient, Secret: ""}, + valid: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.config.Validate() + if tt.valid && err != nil { + t.Errorf( + "Wrong validation, the config was correct but an error was received: \nconfig: %v\nerr: %v", + tt.config, + err, + ) + } + if !tt.valid && err == nil { + t.Errorf( + "Wrong validation, the config was invalid but no error was received: \nconfig: %v", + tt.config, + ) + } + }) + } +} + +func TestIsEmpty(t *testing.T) { + var ( + validSecret = "qaRZGEbnQNNvmaeTLqy8Bxs22wLZ6H7obIiNSveTLPdoQuylANnuy6WBOw16XoqH" + validClient = "CQ4iZ5sebOfhGRwUn3IV0r1YFMNrMTIx" + ) + tests := []struct { + name string + config *Config + want bool + }{ + { + name: "empty config", + config: &Config{Client: "", Secret: ""}, + want: true, + }, + { + name: "config without id", + config: &Config{Client: "", Secret: validSecret}, + want: false, + }, + { + name: "config without secret", + config: &Config{Client: validClient, Secret: ""}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.config.IsEmpty() + if got != tt.want { + t.Errorf("Expected %v but got %v, with config: %v", tt.want, got, tt.config) + } + }) + } +} From 84259490c4ea42efe32ee2931b87d86d47bcb88a Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Fri, 3 Dec 2021 19:02:43 +0100 Subject: [PATCH 3/8] Improve token error --- internal/iot/token.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/iot/token.go b/internal/iot/token.go index 5361e48a..883bc11d 100644 --- a/internal/iot/token.go +++ b/internal/iot/token.go @@ -19,7 +19,9 @@ package iot import ( "context" + "fmt" "net/url" + "strings" "golang.org/x/oauth2" cc "golang.org/x/oauth2/clientcredentials" @@ -37,5 +39,9 @@ func token(client, secret string) (*oauth2.Token, error) { EndpointParams: additionalValues, } // Get the access token in exchange of client_id and client_secret - return config.Token(context.Background()) + t, err := config.Token(context.Background()) + if err != nil && strings.Contains(err.Error(), "401") { + return nil, fmt.Errorf("wrong credentials") + } + return t, err } From a41ac79e819c690cbe39b23f90c135a89e596729 Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Mon, 6 Dec 2021 17:06:08 +0100 Subject: [PATCH 4/8] Remove useless test funcs --- internal/config/config_test.go | 38 ++++++---------------------------- 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e44e2726..2fb7b47f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -28,36 +28,14 @@ import ( func TestRetrieve(t *testing.T) { var ( - validSecret = "qaRZGEbnQNNvmaeTLqy8Bxs22wLZ6H7obIiNSveTLPdoQuylANnuy6WBOw16XoqH" - validClient = "CQ4iZ5sebOfhGRwUn3IV0r1YFMNrMTIx" - validConfig = &Config{validClient, validSecret} - invalidConfig = &Config{"", validSecret} - clientEnv = EnvPrefix + "_CLIENT" - secretEnv = EnvPrefix + "_SECRET" - clientEnvBackup *string - secretEnvBackup *string + validSecret = "qaRZGEbnQNNvmaeTLqy8Bxs22wLZ6H7obIiNSveTLPdoQuylANnuy6WBOw16XoqH" + validClient = "CQ4iZ5sebOfhGRwUn3IV0r1YFMNrMTIx" + validConfig = &Config{validClient, validSecret} + invalidConfig = &Config{"", validSecret} + clientEnv = EnvPrefix + "_CLIENT" + secretEnv = EnvPrefix + "_SECRET" ) - // Preserve user environment variables when executing this test - _ = func() { - if c, ok := os.LookupEnv(clientEnv); ok { - clientEnvBackup = &c - } - if s, ok := os.LookupEnv(secretEnv); ok { - secretEnvBackup = &s - } - } - _ = func() { - if clientEnvBackup != nil { - os.Setenv(clientEnv, *clientEnvBackup) - clientEnvBackup = nil - } - if secretEnvBackup != nil { - os.Setenv(secretEnv, *secretEnvBackup) - secretEnvBackup = nil - } - } - tests := []struct { name string pre func() @@ -68,14 +46,12 @@ func TestRetrieve(t *testing.T) { { name: "valid config written in env", pre: func() { - // pushEnv() os.Setenv(clientEnv, validConfig.Client) os.Setenv(secretEnv, validConfig.Secret) }, post: func() { os.Unsetenv(clientEnv) os.Unsetenv(secretEnv) - // popEnv() }, wantedConfig: validConfig, wantedErr: false, @@ -84,14 +60,12 @@ func TestRetrieve(t *testing.T) { { name: "invalid config written in env", pre: func() { - // pushEnv() os.Setenv(clientEnv, validConfig.Client) os.Setenv(secretEnv, "") }, post: func() { os.Unsetenv(clientEnv) os.Unsetenv(secretEnv) - // popEnv() }, wantedConfig: nil, wantedErr: true, From 8fe13f84b43084aff917607ae66e7905bc323f3d Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Mon, 6 Dec 2021 17:44:54 +0100 Subject: [PATCH 5/8] Update readme --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 9fcf2556..63b2c3a2 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,8 @@ Configuration file is supported in two different format: json and yaml. Use the `$ arduino-cloud-cli config init --config-format json` +It is also possible to specify credentials directly in `ARDUINO_CLOUD_CLIENT` and `ARDUINO_CLOUD_SECRET` environment variables. Credentials specified in environment variables have higher priority than the ones specified in config files. + ## Device provisioning When provisioning a device, you can optionally specify the port to which the device is connected to and its fqbn. If they are not given, then the first device found will be provisioned. From 0132bcad0579dd340fe9983712389a418a403371 Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Mon, 13 Dec 2021 14:45:40 +0100 Subject: [PATCH 6/8] Update internal/config/config.go Co-authored-by: Giuseppe Lumia --- internal/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index 719f09df..d2450f6d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -150,7 +150,7 @@ func fromFile() (*Config, error) { func fromEnv() (*Config, error) { v := viper.New() SetDefaults(v) - v.SetEnvPrefix("ARDUINO_CLOUD") + v.SetEnvPrefix(EnvPrefix) v.AutomaticEnv() conf := &Config{} From d277785765edf63bd57b4d6c48cf32ab3ecfd900 Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Mon, 13 Dec 2021 14:51:47 +0100 Subject: [PATCH 7/8] refactor config isempty --- internal/config/config.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index d2450f6d..02f55007 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -61,13 +61,7 @@ func (c *Config) Validate() error { // IsEmpty checks if config has no params set func (c *Config) IsEmpty() bool { - if len(c.Client) != 0 { - return false - } - if len(c.Secret) != 0 { - return false - } - return true + return len(c.Client) == 0 && len(c.Secret) == 0 } // Retrieve looks for configuration parameters in From 10835a8283601f54b103029fbf12d65f234a7f17 Mon Sep 17 00:00:00 2001 From: Paolo Calao Date: Mon, 13 Dec 2021 15:25:28 +0100 Subject: [PATCH 8/8] Update comments --- internal/config/config.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 02f55007..57536820 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -26,9 +26,13 @@ import ( ) const ( - ClientIDLen = 32 + // ClientIDLen specifies the length of Arduino IoT Cloud client ids. + ClientIDLen = 32 + // ClientSecretLen specifies the length of Arduino IoT Cloud client secrets. ClientSecretLen = 64 + // EnvPrefix is the prefix environment variables should have to be + // fetched as config parameters during the config retrieval. EnvPrefix = "ARDUINO_CLOUD" ) @@ -39,8 +43,8 @@ type Config struct { Secret string `map-structure:"secret"` // Secret ID of the user, unique for each Client ID } -// Validate the config -// If config is not valid, it returns an error explaining the reason +// Validate the config. +// If config is not valid, it returns an error explaining the reason. func (c *Config) Validate() error { if len(c.Client) != ClientIDLen { return fmt.Errorf( @@ -59,14 +63,14 @@ func (c *Config) Validate() error { return nil } -// IsEmpty checks if config has no params set +// IsEmpty checks if config has no params set. func (c *Config) IsEmpty() bool { return len(c.Client) == 0 && len(c.Secret) == 0 } // Retrieve looks for configuration parameters in -// environment variables or in configuration file -// Returns error if no config is found +// environment variables or in configuration file. +// Returns error if no config is found. func Retrieve() (*Config, error) { // Config extracted from environment has highest priority c, err := fromEnv() @@ -92,7 +96,7 @@ func Retrieve() (*Config, error) { ) } -// fromFile looks for a configuration file +// fromFile looks for a configuration file. // If a config file is not found, it returns a nil config without raising errors. // If invalid config file is found, it returns an error. func fromFile() (*Config, error) { @@ -168,10 +172,10 @@ func fromEnv() (*Config, error) { } // searchConfigDir configuration file in different directories in the following order: -// current working directory, parents of the current working directory, arduino15 default directory -// Returns a nil string if no config file has been found, without raising errors +// current working directory, parents of the current working directory, arduino15 default directory. +// Returns a nil string if no config file has been found, without raising errors. // Returns an error if any problem is encountered during the file research which prevents -// to understand whether a config file exists or not +// to understand whether a config file exists or not. func searchConfigDir() (*string, error) { // Search in current directory and its parents. cwd, err := paths.Getwd()