Fixes incorrect handling of cli and config file parameter merging (#87)

* The config value should always fall back to `default`

* Specify defaults section as discussed in #88

* Add more verbose error handling

* Don’t include empty values in overrides

* Moved defaults from cli into config file

* Fix flake8

* Fix test cases

* Handle defaults so they are not written to the config

* Updated test cases to handle config precedence

* Removed defaults check

* Make sure duration is an int

onelogin_aws_cli/__init__.py

@@ -29,7 +29,7 @@ def __init__(self, config: Section):
         self.all_roles = None
         self.role_arn = None
         self.credentials = None
-        self.duration_seconds = config['duration_seconds']
+        self.duration_seconds = int(config['duration_seconds'])
         self.user_credentials = UserCredentials(config)
         self.mfa = MFACredentials()
 
@@ -55,6 +55,11 @@ def get_saml_assertion(self):
             self.config['subdomain']
         )
 
+        if saml_resp is None:
+            raise Exception("Onelogin Error: '{error}' '{desc}'".format(
+                error=self.ol_client.error,
+                desc=self.ol_client.error_description
+            ))
         if saml_resp.mfa:
             if not self.mfa.ready():
                 self.mfa.select_device(saml_resp.mfa.devices)

onelogin_aws_cli/argparse.py

@@ -17,7 +17,7 @@ def __init__(self):
         self.add_argument(
             '-C', '--config-name',
             action=EnvDefault, required=False,
-            dest='config_name',
+            dest='config_name', default='defaults',
             help='Switch configuration name within config file'
         )
 
@@ -34,7 +34,7 @@ def __init__(self):
         )
 
         self.add_argument(
-            '-d', '--duration-seconds', type=int, default=3600,
+            '-d', '--duration-seconds', type=int,
             dest='duration_seconds',
             action=EnvDefault, required=False,
             help='Specify duration seconds which depend on IAM role session '
@@ -67,7 +67,7 @@ def __init__(self):
 
         self.add_argument(
             '-c', '--configure', dest='configure', action='store_true',
-            help='Configure OneLogin and AWS settings'
+            help='Configure OneLogin and AWS settings', default=False
         )
 
 

onelogin_aws_cli/configuration.py

@@ -7,12 +7,15 @@
 class ConfigurationFile(configparser.ConfigParser):
     """Represents a configuration ini file on disk"""
 
-    DEFAULTS = dict(save_password=False)
+    DEFAULTS = dict(
+        save_password=False,
+        duration_seconds=3600
+    )
 
     def __init__(self, config_file=None):
-        super().__init__(default_section='defaults')
-
-        self[self.default_section] = dict(self.DEFAULTS)
+        super().__init__(
+            default_section='defaults',
+        )
 
         self.file = config_file
 
@@ -22,9 +25,7 @@ def __init__(self, config_file=None):
     @property
     def is_initialised(self) -> bool:
         """True if there is at least one section"""
-        return len(
-            self.sections()
-        ) > 0
+        return (len(self.sections()) > 0)
 
     def load(self):
         self.read_file(self.file)
@@ -103,15 +104,19 @@ def set_overrides(self, overrides: dict):
         values, but will not overwrite them in the config file.
         :param overrides:
         """
-        self._overrides = overrides
+        self._overrides = {k: v for k, v in overrides.items() if v is not None}
 
     def __setitem__(self, key, value):
         self.config.set(self.section_name, key, value)
 
     def __getitem__(self, item):
         if item in self._overrides:
             return self._overrides[item]
-        return self.config.get(self.section_name, item)
+
+        if item in self:
+            return self.config.get(self.section_name, item)
+
+        return self.config.DEFAULTS[item]
 
     def __contains__(self, item):
         return self.config.has_option(self.section_name, item)

onelogin_aws_cli/tests/test_configurationFile.py

@@ -70,7 +70,6 @@ def test_initialise(self):
         str.seek(0)
 
         self.assertEqual("""[defaults]
-save_password = False
 base_uri = https://api.eu.onelogin.com/
 client_id = mock_client_id
 client_secret = mock_client_secret

onelogin_aws_cli/tests/test_oneLoginAWSArgumentParser.py

@@ -62,11 +62,6 @@ def test_add_cli_options(self):
         self.assertTrue(args.configure)
         self.assertEqual(args.duration_seconds, 43200)
 
-    def test_default_duration(self):
-        parser = OneLoginAWSArgumentParser()
-        args = parser.parse_args([])
-        self.assertEqual(args.duration_seconds, 3600)
-
     def test_legacy_renew_seconds(self):
         parser = OneLoginAWSArgumentParser()
         args = parser.parse_args([

onelogin_aws_cli/tests/test_section.py

@@ -20,8 +20,9 @@ def test___contains__false(self):
         self.assertFalse('mock' in sec)
 
     def test_set_overrides(self):
-        sec = Section('mock-section', Namespace(
-            get=MagicMock(return_value='world')
+        sec = Section('mock-section', MagicMock(
+            get=MagicMock(return_value='world'),
+            has_option=MagicMock(return_value=True)
         ))
 
         self.assertEqual(sec['foo'], 'world')