[PATCH] Changes allowing a privileged user creates a user account for somebody else

[Mo Morsi]

---
 src/app/controllers/users_controller.rb       |   10 +++++++---
 src/spec/controllers/users_controller_spec.rb |   25 ++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/app/controllers/users_controller.rb b/src/app/controllers/users_controller.rb
index 6c6c021..3536c0e 100644
--- a/src/app/controllers/users_controller.rb
+++ b/src/app/controllers/users_controller.rb
@@ -20,7 +20,6 @@
 # Likewise, all the methods added will be available for all controllers.

class UsersController < ApplicationController
-  before_filter :require_no_user, :only => [:new, :create]
   before_filter :require_user, :only => [:show, :edit, :update]

def new
@@ -28,18 +27,23 @@ class UsersController < ApplicationController
   end

def create
+    require_privilege(Privilege::USER_MODIFY) unless current_user.nil?
     @user = User.new(params[:user])
     @registration = RegistrationService.new(@user)
     if @registration.save
       flash[:notice] = "User registered!"
-      redirect_back_or_default account_url
+      redirect_back_or_default url_for(:action => :show, :id => @user.id)
     else
       render :action => :new
     end
   end

def show
-    @user = @current_user
+    if params.has_key?(:id) && params[:id] != "show"
+      @user = User.find(params[:id])
+    else
+      @user = current_user
+    end
   end

def edit
diff --git a/src/spec/controllers/users_controller_spec.rb b/src/spec/controllers/users_controller_spec.rb
index 5f010cc..1c3a8fd 100644
--- a/src/spec/controllers/users_controller_spec.rb
+++ b/src/spec/controllers/users_controller_spec.rb
@@ -4,6 +4,8 @@ describe UsersController do
   fixtures :all
   before(:each) do
     @tuser = Factory :tuser
+    @admin_permission = Factory :admin_permission
+    @admin = @admin_permission.user
     activate_authlogic
   end

@@ -35,7 +37,8 @@ describe UsersController do
         p.permissions.any? {
           |perm| perm.role.name.eql?('Self-service Pool User')
         }.should be_true
-        response.should redirect_to(account_path)
+        id = User.find(:first, :conditions => ['login = ?', "tuser2"]).id
+        response.should redirect_to("http://test.host/users/show/#%7Bid%7D")
       end

it "fails to create pool" do
@@ -59,6 +62,26 @@ describe UsersController do
     end
   end

+  it "should allow an admin to create user" do
+    UserSession.create(@admin)
+    lambda {
+      post :create, :user => { :login => "tuser3", :email => "[e-mail]",
+                               :password => "testpass",
+                               :password_confirmation => "testpass" }
+    }.should change{ User.count }
+    id = User.find(:first, :conditions => ['login = ?', "tuser3"]).id
+    response.should redirect_to("http://test.host/users/show/#%7Bid%7D")
+  end
+
+  it "should not allow a regular user to create user" do
+    UserSession.create(@tuser)
+    lambda {
+      post :create, :user => { :login => "tuser4", :email => "[e-mail]",
+                               :password => "testpass",
+                               :password_confirmation => "testpass" }
+    }.should_not change{ User.count }
+  end
+
   it "should show user" do
     UserSession.create(@tuser)
     get :show

[PATCH] 'privileged user adds other user to pool' form/backend/specs

[Mo Morsi]

---
 src/app/controllers/portal_pool_controller.rb      |   27 ++++++++++++++++--
 src/app/views/portal_pool/new_user.html.erb        |   29 ++++++++++++++++++++
 src/app/views/portal_pool/show.html.erb            |    1 +
 .../controllers/portal_pool_controller_spec.rb     |   20 +++++++++++++
 4 files changed, 74 insertions(+), 3 deletions(-)
 create mode 100644 src/app/views/portal_pool/new_user.html.erb

diff --git a/src/app/controllers/portal_pool_controller.rb b/src/app/controllers/portal_pool_controller.rb
index 69208fa..a897bb4 100644
--- a/src/app/controllers/portal_pool_controller.rb
+++ b/src/app/controllers/portal_pool_controller.rb
@@ -64,9 +64,6 @@ class PortalPoolController < ApplicationController
   def create
     require_privilege(Privilege::POOL_MODIFY)

-    #FIXME: owner is set to current user for self-service account creation,
-    # but in the more general case we need a way for the admin to pick
-    # a user
     params[:portal_pool][:owner_id] = @current_user.id

#FIXME: This should probably be in a transaction
@@ -81,6 +78,30 @@ class PortalPoolController < ApplicationController
     redirect_to :action => 'show', :id => @portal_pool.id
   end

+  def new_user
+    require_privilege(Privilege::POOL_MODIFY)
+    require_privilege(Privilege::USER_MODIFY)
+
+    @users = User.find(:all)
+    @roles = Role.find(:all, :conditions => [ 'scope = ?', 'PortalPool' ])
+    @pool  = PortalPool.find(params[:id])
+  end
+
+  def create_user
+    require_privilege(Privilege::POOL_MODIFY)
+    require_privilege(Privilege::USER_MODIFY)
+
+    pool  = PortalPool.find(params[:pool_id])
+    role  = Role.find(params[:role_id])
+    user  = User.find(params[:user_id])
+    @permission = Permission.create! :user => user,
+                                     :role => role,
+                                     :permission_object => pool
+
+    flash[:notice] = "User added to pool"
+    redirect_to :action => 'show', :id => pool.id
+  end
+
   def delete
   end

diff --git a/src/app/views/portal_pool/new_user.html.erb b/src/app/views/portal_pool/new_user.html.erb
new file mode 100644
index 0000000..1e18cb7
--- /dev/null
+++ b/src/app/views/portal_pool/new_user.html.erb
@@ -0,0 +1,29 @@
+<div class="dcloud_form">
+  <%= error_messages_for 'permission' %>
+
+  <h2>Add user to pool</h2><br />
+
+  <% form_tag :action => 'create_user' do -%>
+    <fieldset>
+    <ul>
+      <li><label>User</label>
+      <select id="user_id" name="user_id">
+      <% @users.each { |user| %>
+        <option value="<%= user.id %>"><%= user.login %></option>
+      <% } %>
+      </select>
+      </li>
+      <li><label>Role</label>
+      <select id="role_id" name="role_id">
+      <% @roles.each { |role| %>
+        <option value="<%= role.id %>"><%= role.name %></option>
+      <% } %>
+      </select>
+      </li>
+      <input type="hidden" name="pool_id" value="<%= @pool.id %>" />
+    <ul>
+    </fieldset>
+
+    <%= submit_tag "Save", :class => "submit" %>
+  <% end %>
+</div>
diff --git a/src/app/views/portal_pool/show.html.erb b/src/app/views/portal_pool/show.html.erb
index 128d74c..80b8636 100644
--- a/src/app/views/portal_pool/show.html.erb
+++ b/src/app/views/portal_pool/show.html.erb
@@ -40,3 +40,4 @@
 <%= link_to "Hardware Profiles",  {:action => "hardware_profiles", :id => @pool.id}, :class=>"actionlink"%>
 <%=link_to "View Images", {:controller => "portal_pool", :action => "images", :portal_pool => @pool}, :class => "actionlink" %>
 <%= link_to "Realms",  {:action => "realms", :id => @pool.id}, :class=>"actionlink"%>
+<%= link_to "Add user",  {:action => "new_user", :id => @pool.id}, :class=>"actionlink"%>
diff --git a/src/spec/controllers/portal_pool_controller_spec.rb b/src/spec/controllers/portal_pool_controller_spec.rb
index 2ea6b4d..7d8e920 100644
--- a/src/spec/controllers/portal_pool_controller_spec.rb
+++ b/src/spec/controllers/portal_pool_controller_spec.rb
@@ -58,4 +58,24 @@ describe PortalPoolController do
      response.should render_template("realms")
   end

+  it "should allow an admin to add a user to a pool" do
+     pool = Factory :tpool
+     UserSession.create(@admin)
+     get :new_user, :id => pool.id
+     response.should be_success
+     response.should render_template("new_user")
+
+     tuser = Factory :tuser
+     role  = Role.find(:first, :conditions => [ 'name = ?', 'Instance Controller'])
+     lambda {
+       post :create_user, :id => pool.id, :user_id => tuser.id, :role_id => role.id
+     }.should change(Permission, :count).by(1)
+     Permission.find(:first, 
+                     :conditions => [ 'user_id                = ? AND ' +  
+                                      'role_id                = ? AND ' +
+                                      'permission_object_id   = ? AND ' +
+                                      'permission_object_type = ?', 
+                                      tuser.id, role.id, pool.id, 'PortalPool' ]).should_not be_nil
+  end
+
 end

[PATCH] 'privileged user adds other user to pool' form/backend/specs

[Michal Fojtik]

On 24/03/10 14:58 -0400, Mohammed Morsi wrote:

Hi,

Just few notes:
> def new_user
> require_privilege(Privilege::POOL_MODIFY)
> require_privilege(Privilege::USER_MODIFY)

Why you can't just simply pass a Array as an argument to this method ?
Something like require_privileges([1, 2]).
> @users = User.find(:all)
> @roles = Role.find(:all, :conditions => [ 'scope = ?', 'PortalPool' ])
> @pool  = PortalPool.find(params[:id])
> end
> def create_user
> require_privilege(Privilege::POOL_MODIFY)
> require_privilege(Privilege::USER_MODIFY)

In other hands, why not create a 'before_filter' for this ?

before_filter :require_privileges
> @@ -40,3 +40,4 @@
> <%= link_to "Hardware Profiles",  {:action => "hardware_profiles", :id => @pool.id}, :class=>"actionlink"%>

This could be: <%= link_to "Hardware Profiles", hardware_profiles_path(@pool), :class => "actionlink"%>

(btw. @pool == hardware_profile ?)

- Michal

[PATCH] 'privileged user adds other user to pool' form/backend/specs

[Scott Seago]

Michal Fojtik wrote:
> On 24/03/10 14:58 -0400, Mohammed Morsi wrote:
>
> Hi,
>
> Just few notes:

Michal,

I'm actually in the process of doing this in a different way that's a 
bit more generic, so this patch won't be pushed as-is -- but I've got a 
couple responses to your comments here.
> > def new_user
> > require_privilege(Privilege::POOL_MODIFY)
> > require_privilege(Privilege::USER_MODIFY)
>
> Why you can't just simply pass a Array as an argument to this method ?
> Something like require_privileges([1, 2]).

Actually I think in this case what we really want is SET_PERMS. However 
making this method accept an array would be useful if we find any other 
cases where we want to verify two different privileges on a single 
object -- I'm not sure we have any current need for that though.
> > @users = User.find(:all)
> > @roles = Role.find(:all, :conditions => [ 'scope = ?', 'PortalPool' ])
> > @pool  = PortalPool.find(params[:id])
> > end
> > def create_user
> > require_privilege(Privilege::POOL_MODIFY)
> > require_privilege(Privilege::USER_MODIFY)
>
> In other hands, why not create a 'before_filter' for this ?
>
> before_filter :require_privileges

We did perm checks with before filters in ovirt, but the end result was 
somewhat messy. Normally require_privilege takes an object as a second 
argument -- for which object we're requiring the privilege on (in fact 
for this patch @pool should be passed in here). So when we did 
permission checks in before_filters we had to have additional 
before_filters to do whatever is required to determine what object we're 
using for permissions checks. In several cases this resulted with an 
action with a completely empty body and a several-line before_filter, 
which made the controller flow rather difficult to follow.
> > @@ -40,3 +40,4 @@
> > <%= link_to "Hardware Profiles",  {:action => "hardware_profiles", :id => @pool.id}, :class=>"actionlink"%>
>
> This could be: <%= link_to "Hardware Profiles", hardware_profiles_path(@pool), :class => "actionlink"%>

I prefer the former link, as it's easier to read -- clearly identifying 
action, object (id), and (when appropriate) controller. But it's more of 
a personal preference -- no big deal either way.
> (btw. @pool == hardware_profile ?)

Pools and HW profiles are different object types. In this case the 
hardware_profiles action on the pool controller displays a list of 
hardware profiles associated with the selected pool.
> Michal

Scott

[PATCH] 'privileged user adds other user to pool' form/backend/specs

[Mo Morsi]

On 03/26/2010 09:37 AM, Scott Seago wrote:
> Michal Fojtik wrote:
> > On 24/03/10 14:58 -0400, Mohammed Morsi wrote:
> >
> > Hi,
> >
> > Just few notes:
>
> Michal,
>
> I'm actually in the process of doing this in a different way that's a 
> bit more generic, so this patch won't be pushed as-is -- but I've got 
> a couple responses to your comments here.
> > > def new_user
> > > require_privilege(Privilege::POOL_MODIFY)
> > > require_privilege(Privilege::USER_MODIFY)
> >
> > Why you can't just simply pass a Array as an argument to this method ?
> > Something like require_privileges([1, 2]).
>
> Actually I think in this case what we really want is SET_PERMS. 
> However making this method accept an array would be useful if we find 
> any other cases where we want to verify two different privileges on a 
> single object -- I'm not sure we have any current need for that though.

Also making it accept an array may be tricky though, as many calls to 
require_privilege require a second, different argument, eg the target 
object which the user needs permissions to in order to do the action (eg 
require_privilege(POOL_VIEW, @pool)).
> > > @users = User.find(:all)
> > > @roles = Role.find(:all, :conditions => [ 'scope = ?',
> > >
> > > 'PortalPool' ])
> > > @pool  = PortalPool.find(params[:id])
> > > end
> > > def create_user
> > > require_privilege(Privilege::POOL_MODIFY)
> > > require_privilege(Privilege::USER_MODIFY)
> >
> > In other hands, why not create a 'before_filter' for this ?
> >
> > before_filter :require_privileges
>
> We did perm checks with before filters in ovirt, but the end result 
> was somewhat messy. Normally require_privilege takes an object as a 
> second argument -- for which object we're requiring the privilege on 
> (in fact for this patch @pool should be passed in here). So when we 
> did permission checks in before_filters we had to have additional 
> before_filters to do whatever is required to determine what object 
> we're using for permissions checks. In several cases this resulted 
> with an action with a completely empty body and a several-line 
> before_filter, which made the controller flow rather difficult to follow.

Also since this method was being added to the portal pool controller, 
and all of those actions do not require USER_MODIFY, it wouldn't work to 
register that check as a filter. I suppose we could add it as a filter 
for only the relevant methods, but this way might be the simplest.
> > > @@ -40,3 +40,4 @@
> > > <%= link_to "Hardware Profiles",  {:action => "hardware_profiles", 
> > > :id => @pool.id}, :class=>"actionlink"%>
> >
> > This could be: <%= link_to "Hardware Profiles", 
> > hardware_profiles_path(@pool), :class => "actionlink"%>

I wasn't editing this line w/ this patch (no preceding +/-), just a 
relic of the diff system.
> I prefer the former link, as it's easier to read -- clearly 
> identifying action, object (id), and (when appropriate) controller. 
> But it's more of a personal preference -- no big deal either way.

Agree, also like the clearer way.
> > (btw. @pool == hardware_profile ?)
>
> Pools and HW profiles are different object types. In this case the 
> hardware_profiles action on the pool controller displays a list of 
> hardware profiles associated with the selected pool.
> > Michal
>
> Scott

[Scott Seago]

Mohammed Morsi wrote:
> src/app/controllers/users_controller.rb       |   10 +++++++---
>  src/spec/controllers/users_controller_spec.rb |   25 ++++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 4 deletions(-)

This works fine, but a couple comments:

1) Right now there's no way to get to the 'create user' action if you're 
already logged in. We need a link in the left nav to 'add new user' that 
only appears for admins with user_modify permission -- you can look at 
the patch I posted yesterday to see how to do a conditonal link there -- 
as I added a different conditional link to that same section.
2) The form is fine for now -- but the 'create user account for someone 
else' form should probably be styled somewhat differently -- this should 
happen after the site redesign, and we should figure out whether we can 
do one-time passwords or some other way of forcing the new user to 
change the password away from the admin-generated one.

So for this patch, add the link and verify that it works for admins and 
is hidden for non-admins. Then push.

Scott
> diff --git a/src/app/controllers/users_controller.rb b/src/app/controllers/users_controller.rb
> index 6c6c021..3536c0e 100644
> --- a/src/app/controllers/users_controller.rb
> +++ b/src/app/controllers/users_controller.rb
> @@ -20,7 +20,6 @@
>  # Likewise, all the methods added will be available for all controllers.
>
> class UsersController < ApplicationController
>
> before_filter :require_no_user, :only => [:new, :create]
> before_filter :require_user, :only => [:show, :edit, :update]
>
> def new
>
> @@ -28,18 +27,23 @@ class UsersController < ApplicationController
>    end
>
> def create
> require_privilege(Privilege::USER_MODIFY) unless current_user.nil?
> @user = User.new(params[:user])
> @registration = RegistrationService.new(@user)
> if @registration.save
>   flash[:notice] = "User registered!"
>  redirect_back_or_default account_url
>  redirect_back_or_default url_for(:action => :show, :id => @user.id)
>
> else
>      render :action => :new
>    end
>  end
>
> def show
> @user = @current_user
>
> if params.has_key?(:id) && params[:id] != "show"
>  @user = User.find(params[:id])
>
> else
>  @user = current_user
>
> end
> end
>
> def edit
>
> diff --git a/src/spec/controllers/users_controller_spec.rb b/src/spec/controllers/users_controller_spec.rb
> index 5f010cc..1c3a8fd 100644
> --- a/src/spec/controllers/users_controller_spec.rb
> +++ b/src/spec/controllers/users_controller_spec.rb
> @@ -4,6 +4,8 @@ describe UsersController do
>    fixtures :all
>    before(:each) do
>      @tuser = Factory :tuser
> @admin_permission = Factory :admin_permission
> @admin = @admin_permission.user
> activate_authlogic
> end
>
> @@ -35,7 +37,8 @@ describe UsersController do
>          p.permissions.any? {
>            |perm| perm.role.name.eql?('Self-service Pool User')
>          }.should be_true
>    response.should redirect_to(account_path)
>    id = User.find(:first, :conditions => ['login = ?', "tuser2"]).id
>    response.should redirect_to("http://test.host/users/show/#{id}")
>  end
>
>  it "fails to create pool" do
>
> @@ -59,6 +62,26 @@ describe UsersController do
>      end
>    end
> it "should allow an admin to create user" do
> UserSession.create(@admin)
> lambda {
>  post :create, :user => { :login => "tuser3", :email => "[e-mail]",
>                           :password => "testpass",
>                           :password_confirmation => "testpass" }
> }.should change{ User.count }
> id = User.find(:first, :conditions => ['login = ?', "tuser3"]).id
> response.should redirect_to("http://test.host/users/show/#%7Bid%7D")
> end
> it "should not allow a regular user to create user" do
> UserSession.create(@tuser)
> lambda {
>  post :create, :user => { :login => "tuser4", :email => "[e-mail]",
>                           :password => "testpass",
>                           :password_confirmation => "testpass" }
> }.should_not change{ User.count }
> end
> it "should show user" do
>    UserSession.create(@tuser)
>    get :show