From ad3e93eaaea9b3eca30bb74c4d1b50fae5eef2b1 Mon Sep 17 00:00:00 2001
From: Arne Welzel <awelzel@lastline.com>
Date: Thu, 29 Jan 2015 17:18:46 -0800
Subject: [PATCH] administrator/admin and tag related fixes

While working with this puppet module, I observed a few glitches:

- setting admin to true and tags to an empty list, a puppet run would
  either attempt to set the user tags to an empty list, or adding the
  adminstrator tag again. Therefore, do not expose the administrator
  tag through tags if admin is true. Further, do not allow setting
  the administrator explicitly in tags.

- providing tags that are not sorted to rabbitmq::user would make
  puppet try to apply the non sorted list over and over again. Also,
  default to the empty array for tags. This will remove existing tags
  from users if they weren't set through puppet.

- provide nicer output when changing tags using tags.join(' ,')
---
 .../provider/rabbitmq_user/rabbitmqctl.rb     |  9 +++--
 lib/puppet/type/rabbitmq_user.rb              | 35 +++++++++++++++++--
 .../rabbitmq_user/rabbitmqctl_spec.rb         | 18 ++++++++++
 spec/unit/puppet/type/rabbitmq_user_spec.rb   | 10 ++++++
 4 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb b/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb
index 9cedd2aa6..3980e7556 100644
--- a/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb
+++ b/lib/puppet/provider/rabbitmq_user/rabbitmqctl.rb
@@ -30,7 +30,7 @@ def create
     if resource[:admin] == :true
       make_user_admin()
     end
-    if !resource[:tags].nil?
+    if ! resource[:tags].empty?
       set_user_tags(resource[:tags])
     end
   end
@@ -67,7 +67,12 @@ def exists?
 
 
   def tags
-    get_user_tags.entries.sort
+    tags = get_user_tags
+    # do not expose the administrator tag for admins
+    if resource[:admin] == :true
+      tags.delete('administrator')
+    end
+    tags.entries.sort
   end
 
 
diff --git a/lib/puppet/type/rabbitmq_user.rb b/lib/puppet/type/rabbitmq_user.rb
index 66eef92c2..baf479585 100644
--- a/lib/puppet/type/rabbitmq_user.rb
+++ b/lib/puppet/type/rabbitmq_user.rb
@@ -32,10 +32,10 @@ def change_to_s(current, desired)
   end
 
   newproperty(:admin) do
-    desc 'rather or not user should be an admin'
+    desc 'whether or not user should be an admin'
     newvalues(/true|false/)
     munge do |value|
-      # converting to_s incase its a boolean
+      # converting to_s in case its a boolean
       value.to_s.to_sym
     end
     defaultto :false
@@ -43,6 +43,37 @@ def change_to_s(current, desired)
 
   newproperty(:tags, :array_matching => :all) do
     desc 'additional tags for the user'
+    validate do |value|
+      unless value =~ /^\S+$/
+        raise ArgumentError, "Invalid tag: #{value.inspect}"
+      end
+
+      if value == "administrator"
+        raise ArgumentError, "must use admin property instead of administrator tag"
+      end
+    end
+    defaultto []
+
+    def insync?(is)
+      self.is_to_s(is) == self.should_to_s
+    end
+
+    def is_to_s(currentvalue = @is)
+      if currentvalue
+        "[#{currentvalue.sort.join(', ')}]"
+      else
+        '[]'
+      end
+    end
+
+    def should_to_s(newvalue = @should)
+      if newvalue
+        "[#{newvalue.sort.join(', ')}]"
+      else
+        '[]'
+      end
+    end
+
   end
 
   validate do
diff --git a/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb b/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb
index 2012ccc05..ed828ea94 100644
--- a/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb
+++ b/spec/unit/puppet/provider/rabbitmq_user/rabbitmqctl_spec.rb
@@ -194,4 +194,22 @@
     @provider.create
   end
 
+  it 'should not return the administrator tag in tags for admins' do
+    @resource[:tags] = []
+    @resource[:admin]  = true
+    @provider.expects(:rabbitmqctl).with('-q', 'list_users').returns <<-EOT
+foo [administrator]
+EOT
+    @provider.tags.should == []
+  end
+
+  it 'should return the administrator tag for non-admins' do
+    # this should not happen though.
+    @resource[:tags] = []
+    @resource[:admin]  = :false
+    @provider.expects(:rabbitmqctl).with('-q', 'list_users').returns <<-EOT
+foo [administrator]
+EOT
+    @provider.tags.should == ["administrator"]
+  end
 end
diff --git a/spec/unit/puppet/type/rabbitmq_user_spec.rb b/spec/unit/puppet/type/rabbitmq_user_spec.rb
index 92e690557..a73d9c936 100644
--- a/spec/unit/puppet/type/rabbitmq_user_spec.rb
+++ b/spec/unit/puppet/type/rabbitmq_user_spec.rb
@@ -39,4 +39,14 @@
       @user[:admin] = 'yes'
     }.to raise_error(Puppet::Error, /Invalid value/)
   end
+  it 'should not accept tags with spaces' do
+    expect {
+      @user[:tags] = ['policy maker']
+    }.to raise_error(Puppet::Error, /Invalid tag/)
+  end
+  it 'should not accept the administrator tag' do
+    expect {
+      @user[:tags] = ['administrator']
+    }.to raise_error(Puppet::Error, /must use admin property/)
+  end
 end