Credentials#unset method

* Unset was broken, not resetting the variables
* Also added a rspec example

It worked but wasn't clean as if an authentication method failed some fields
could have been left un-blanked, creating bad side effects.

In addition:
* Added tests for #set with both valid and invalid values
* Added negative tests for #service_token_set? and user_password_set?
* Added test for #version
* Fixed #unset test
* Fixed #to_env test
* In several tests, added an OS_NOT_VALID parameter to make sure the
  credentials code correctly ignores it
* Added tests to make sure the fallback in Auth.request works properly
* Added additional tests to make sure the @credentials object is set
  correctly
* Fixed some formatting/alignment issues

Change-Id: I64cb492dc244b975827d2d0d86b4b97d7129088f

lib/puppet/provider/openstack/credentials.rb

@@ -43,8 +43,12 @@ class Puppet::Provider::Openstack::Credentials
   end
 
   def unset
-    list = KEYS.delete_if { |key, val| key == :identity_api_version }
+    KEYS.each do |key|
-    list.each { |key, val| self.set(key, '') if self.class.defined?("@#{key}".to_sym) }
+      if key != :identity_api_version &&
+        self.instance_variable_defined?("@#{key}")
+        set(key, '')
+      end
+    end
   end
 
   def version

spec/unit/provider/openstack/auth_spec.rb

@@ -41,15 +41,18 @@ describe Puppet::Provider::Openstack::Auth do
   describe '#set_credentials' do
     it 'adds keys to the object' do
       credentials = Puppet::Provider::Openstack::CredentialsV2_0.new
-      set = { 'OS_USERNAME' => 'user', 'OS_PASSWORD' => 'secret',
+      set = { 'OS_USERNAME'             => 'user',
+              'OS_PASSWORD'             => 'secret',
               'OS_PROJECT_NAME'         => 'tenant',
               'OS_AUTH_URL'             => 'http://127.0.0.1:5000',
               'OS_TOKEN'                => 'token',
               'OS_URL'                  => 'http://127.0.0.1:35357',
-        'OS_IDENTITY_API_VERSION' => '2.0'
+              'OS_IDENTITY_API_VERSION' => '2.0',
+              'OS_NOT_VALID'            => 'notvalid'
         }
       klass.set_credentials(credentials, set)
-      expect(credentials.to_env).to eq("OS_AUTH_URL" => "http://127.0.0.1:5000",
+      expect(credentials.to_env).to eq(
+        "OS_AUTH_URL"             => "http://127.0.0.1:5000",
         "OS_IDENTITY_API_VERSION" => '2.0',
         "OS_PASSWORD"             => "secret",
         "OS_PROJECT_NAME"         => "tenant",
@@ -73,7 +76,11 @@ describe Puppet::Provider::Openstack::Auth do
         ENV['OS_PROJECT_NAME'] = 'test'
         ENV['OS_USERNAME']     = 'test'
         response = klass.get_os_vars_from_env
-        expect(response).to eq({"OS_AUTH_URL" => "http://127.0.0.1:5000","OS_PASSWORD" => "abc123","OS_PROJECT_NAME" => "test","OS_USERNAME" => "test"})
+        expect(response).to eq({
+          "OS_AUTH_URL"     => "http://127.0.0.1:5000",
+          "OS_PASSWORD"     => "abc123",
+          "OS_PROJECT_NAME" => "test",
+          "OS_USERNAME"     => "test"})
       end
     end
   end
@@ -87,7 +94,11 @@ describe Puppet::Provider::Openstack::Auth do
         File.expects(:open).with('file').returns(StringIO.new(mock))
 
         response = klass.get_os_vars_from_rcfile(filename)
-        expect(response).to eq({"OS_AUTH_URL" => "http://127.0.0.1:5000","OS_PASSWORD" => "abc123","OS_PROJECT_NAME" => "test","OS_USERNAME" => "test"})
+        expect(response).to eq({
+          "OS_AUTH_URL"     => "http://127.0.0.1:5000",
+          "OS_PASSWORD"     => "abc123",
+          "OS_PROJECT_NAME" => "test",
+          "OS_USERNAME"     => "test"})
       end
     end
 
@@ -113,6 +124,7 @@ describe Puppet::Provider::Openstack::Auth do
     context 'with no valid credentials' do
       it 'fails to authenticate' do
         expect { klass.request('project', 'list', ['--long']) }.to raise_error(Puppet::Error::OpenstackAuthInputError, "Insufficient credentials to authenticate")
+        expect(klass.instance_variable_get(:@credentials).to_env).to eq({})
       end
     end
 
@@ -122,7 +134,8 @@ describe Puppet::Provider::Openstack::Auth do
              .returns({ 'OS_USERNAME'     => 'test',
                         'OS_PASSWORD'     => 'abc123',
                         'OS_PROJECT_NAME' => 'test',
-                        'OS_AUTH_URL' => 'http://127.0.0.1:5000' })
+                        'OS_AUTH_URL'     => 'http://127.0.0.1:5000',
+                        'OS_NOT_VALID'    => 'notvalid' })
         klass.expects(:openstack)
              .with('project', 'list', '--quiet', '--format', 'csv', ['--long'])
              .returns('"ID","Name","Description","Enabled"
@@ -130,6 +143,12 @@ describe Puppet::Provider::Openstack::Auth do
 ')
         response = klass.request('project', 'list', ['--long'])
         expect(response.first[:description]).to eq("Test tenant")
+        expect(klass.instance_variable_get(:@credentials).to_env).to eq({
+          'OS_USERNAME'     => 'test',
+          'OS_PASSWORD'     => 'abc123',
+          'OS_PROJECT_NAME' => 'test',
+          'OS_AUTH_URL'     => 'http://127.0.0.1:5000'
+        })
       end
     end
 
@@ -137,7 +156,8 @@ describe Puppet::Provider::Openstack::Auth do
       it 'is successful' do
         klass.expects(:get_os_vars_from_env)
              .returns({ 'OS_TOKEN'     => 'test',
-                        'OS_URL' => 'http://127.0.0.1:5000' })
+                        'OS_URL'       => 'http://127.0.0.1:5000',
+                        'OS_NOT_VALID' => 'notvalid' })
         klass.expects(:openstack)
              .with('project', 'list', '--quiet', '--format', 'csv', ['--long'])
              .returns('"ID","Name","Description","Enabled"
@@ -145,12 +165,20 @@ describe Puppet::Provider::Openstack::Auth do
 ')
         response = klass.request('project', 'list', ['--long'])
         expect(response.first[:description]).to eq("Test tenant")
+        expect(klass.instance_variable_get(:@credentials).to_env).to eq({
+          'OS_TOKEN' => 'test',
+          'OS_URL'   => 'http://127.0.0.1:5000',
+        })
       end
     end
 
     context 'with a RC file containing user credentials' do
       it 'is successful' do
-        mock = "export OS_USERNAME='test'\nexport OS_PASSWORD='abc123'\nexport OS_PROJECT_NAME='test'\nexport OS_AUTH_URL='http://127.0.0.1:5000'"
+        # return incomplete creds from env
+        klass.expects(:get_os_vars_from_env)
+             .returns({ 'OS_USERNAME' => 'incompleteusername',
+                        'OS_AUTH_URL' => 'incompleteauthurl' })
+        mock = "export OS_USERNAME='test'\nexport OS_PASSWORD='abc123'\nexport OS_PROJECT_NAME='test'\nexport OS_AUTH_URL='http://127.0.0.1:5000'\nexport OS_NOT_VALID='notvalid'"
         File.expects(:exists?).with("#{ENV['HOME']}/openrc").returns(true)
         File.expects(:open).with("#{ENV['HOME']}/openrc").returns(StringIO.new(mock))
         klass.expects(:openstack)
@@ -160,12 +188,21 @@ describe Puppet::Provider::Openstack::Auth do
 ')
         response = provider.class.request('project', 'list', ['--long'])
         expect(response.first[:description]).to eq("Test tenant")
+        expect(klass.instance_variable_get(:@credentials).to_env).to eq({
+          'OS_USERNAME'     => 'test',
+          'OS_PASSWORD'     => 'abc123',
+          'OS_PROJECT_NAME' => 'test',
+          'OS_AUTH_URL'     => 'http://127.0.0.1:5000'
+        })
       end
     end
 
     context 'with a RC file containing service token credentials' do
       it 'is successful' do
-        mock = "export OS_TOKEN='test'\nexport OS_URL='abc123'\n"
+        # return incomplete creds from env
+        klass.expects(:get_os_vars_from_env)
+             .returns({ 'OS_TOKEN' => 'incomplete' })
+        mock = "export OS_TOKEN='test'\nexport OS_URL='abc123'\nexport OS_NOT_VALID='notvalid'\n"
         File.expects(:exists?).with("#{ENV['HOME']}/openrc").returns(true)
         File.expects(:open).with("#{ENV['HOME']}/openrc").returns(StringIO.new(mock))
         klass.expects(:openstack)
@@ -175,6 +212,10 @@ describe Puppet::Provider::Openstack::Auth do
 ')
         response = klass.request('project', 'list', ['--long'])
         expect(response.first[:description]).to eq("Test tenant")
+        expect(klass.instance_variable_get(:@credentials).to_env).to eq({
+          'OS_TOKEN' => 'test',
+          'OS_URL'   => 'abc123',
+        })
       end
     end
   end

spec/unit/provider/openstack/credentials_spec.rb

@@ -10,12 +10,37 @@ describe Puppet::Provider::Openstack::Credentials do
     creds = Puppet::Provider::Openstack::CredentialsV2_0.new
   end
 
+  describe "#set with valid value" do
+    it 'works with valid value' do
+      expect(creds.class.defined?('auth_url')).to be_truthy
+      creds.set('auth_url', 'http://localhost:5000/v2.0')
+      expect(creds.auth_url).to eq('http://localhost:5000/v2.0')
+    end
+  end
+
+  describe "#set with invalid value" do
+    it 'works with invalid value' do
+      expect(creds.class.defined?('foo')).to be_falsey
+      creds.set('foo', 'junk')
+      expect(creds.respond_to?(:foo)).to be_falsey
+      expect(creds.instance_variable_defined?(:@foo)).to be_falsey
+      expect { creds.foo }.to raise_error(NoMethodError, /undefined method/)
+    end
+  end
+
   describe '#service_token_set?' do
     context "with service credentials" do
       it 'is successful' do
         creds.token = 'token'
         creds.url = 'url'
         expect(creds.service_token_set?).to be_truthy
+        expect(creds.user_password_set?).to be_falsey
+      end
+
+      it 'fails' do
+        creds.token = 'token'
+        expect(creds.service_token_set?).to be_falsey
+        expect(creds.user_password_set?).to be_falsey
       end
     end
   end
@@ -28,6 +53,15 @@ describe Puppet::Provider::Openstack::Credentials do
         creds.project_name = 'project_name'
         creds.username = 'username'
         expect(creds.user_password_set?).to be_truthy
+        expect(creds.service_token_set?).to be_falsey
+      end
+
+      it 'fails' do
+        creds.auth_url = 'auth_url'
+        creds.password = 'password'
+        creds.project_name = 'project_name'
+        expect(creds.user_password_set?).to be_falsey
+        expect(creds.service_token_set?).to be_falsey
       end
     end
   end
@@ -40,6 +74,36 @@ describe Puppet::Provider::Openstack::Credentials do
     end
   end
 
+  describe '#version' do
+    it 'is version 2' do
+      expect(creds.version).to eq('2.0')
+    end
+  end
+
+  describe '#unset' do
+    context "with all instance variables set" do
+      it 'resets all but the identity_api_version' do
+        creds.auth_url = 'auth_url'
+        creds.password = 'password'
+        creds.project_name = 'project_name'
+        creds.username = 'username'
+        creds.token = 'token'
+        creds.url = 'url'
+        creds.identity_api_version = 'identity_api_version'
+        creds.unset
+        expect(creds.auth_url).to eq('')
+        expect(creds.password).to eq('')
+        expect(creds.project_name).to eq('')
+        expect(creds.username).to eq('')
+        expect(creds.token).to eq('')
+        expect(creds.url).to eq('')
+        expect(creds.identity_api_version).to eq('identity_api_version')
+        newcreds = Puppet::Provider::Openstack::CredentialsV3.new
+        expect(newcreds.identity_api_version).to eq('3')
+      end
+    end
+  end
+
   describe '#to_env' do
     context "with an exhaustive data set" do
       it 'successfully returns content' do
@@ -50,13 +114,15 @@ describe Puppet::Provider::Openstack::Credentials do
         creds.token = 'token'
         creds.url = 'url'
         creds.identity_api_version = 'identity_api_version'
-        expect(creds.auth_url).to eq("auth_url")
+        expect(creds.to_env).to eq({
-        expect(creds.password).to eq("password")
+          'OS_USERNAME'             => 'username',
-        expect(creds.project_name).to eq("project_name")
+          'OS_PASSWORD'             => 'password',
-        expect(creds.username).to eq("username")
+          'OS_PROJECT_NAME'         => 'project_name',
-        expect(creds.token).to eq('token')
+          'OS_AUTH_URL'             => 'auth_url',
-        expect(creds.url).to eq('url')
+          'OS_TOKEN'                => 'token',
-        expect(creds.identity_api_version).to eq('identity_api_version')
+          'OS_URL'                  => 'url',
+          'OS_IDENTITY_API_VERSION' => 'identity_api_version'
+        })
       end
     end
   end