From 9830f18927de6e3e62c08569e09e8ada861de9e7 Mon Sep 17 00:00:00 2001
From: Ohad Levy <ohadlevy@gmail.com>
Date: Wed, 29 Sep 2010 13:23:35 +0200
Subject: [PATCH] my mods/suggestions

---
 app/controllers/application_controller.rb  |   31 ++++++++++++--------
 app/controllers/notices_controller.rb      |    4 +-
 app/models/notice.rb                       |   16 ++++++++--
 app/models/user.rb                         |   16 +++++++++-
 app/views/common/_notice.erb               |   42 +++++++++++++--------------
 app/views/layouts/standard.rhtml           |    2 +-
 config/routes.rb                           |    2 +-
 test/functional/notices_controller_test.rb |    9 ++---
 test/unit/user_test.rb                     |    4 ++
 9 files changed, 77 insertions(+), 49 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index d99f82a..bb2b21d 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -13,10 +13,8 @@ class ApplicationController < ActionController::Base
   helper 'layout'
 
   before_filter :require_ssl, :require_login
+  before_filter :welcome, :detect_notices, :only => :index
   before_filter :load_tabs, :manage_tabs
-  before_filter :welcome, :only => :index
-  # We only really need to see these when listing, (with a GET)
-  before_filter :detect_notices, :only => :index
 
   # host list AJAX methods
   # its located here, as it might be requested from the dashboard controller or via the hosts controller
@@ -79,24 +77,31 @@ class ApplicationController < ActionController::Base
   end
 
 
-  # Force a user to login if ldap authentication is enabled
+  # Force a user to login if authentication is enabled
   # Sets @user and User.current to the logged in user, or to admin if logins are not used
   def require_login
-    if SETTINGS[:login]
-      unless (session[:user] and (@user = User.find(session[:user])))
-        session[:original_uri] = request.request_uri
-        redirect_to login_users_path
+    unless session[:user] and @user = User.find(session[:user])
+      # User is not found or first login
+      if SETTINGS[:login]
+        # authentication is enabled
+        session[:original_uri] = request.request_uri # keep the old request uri that we can redirect later on
+        redirect_to login_users_path and return
+      else
+        # We assume we always have a user logged in, if authentication is disabled, the user is the build-in admin account.
+        if @user = User.find_by_login("admin")
+          session[:user] = @user.id
+        else
+          # TODO: Test this!
+          flash[:foreman_error] = "Unable to find internal system admin account - do you want to fix this?..."
+        end
       end
-    else
-      session[:user] ||= User.find_by_login "admin"
-      @user = session[:user]
     end
     User.current = @user
   end
 
   # returns current user
   def current_user
-    @username
+    @user
   end
 
   def invalid_request
@@ -127,7 +132,7 @@ class ApplicationController < ActionController::Base
 
   private
   def detect_notices
-    @notices = User.current.try :notices
+    @notices = current_user.notices
   end
 
   def active_tab=(value); @active_tab = session[:controller_active_tabs][controller_name] = value; end
diff --git a/app/controllers/notices_controller.rb b/app/controllers/notices_controller.rb
index 1ad83a7..234106d 100644
--- a/app/controllers/notices_controller.rb
+++ b/app/controllers/notices_controller.rb
@@ -1,10 +1,10 @@
 class NoticesController < ApplicationController
-  def acknowledge
+  def destroy
     @notice = Notice.find(params[:id])
     if @notice.global
       @notice.destroy
     else
-      @notice.users.delete(User.current)
+      @notice.users.delete current_user
       @notice.destroy unless @notice.users.any?
     end
     redirect_to :back
diff --git a/app/models/notice.rb b/app/models/notice.rb
index 27490f1..1e1595e 100644
--- a/app/models/notice.rb
+++ b/app/models/notice.rb
@@ -1,15 +1,25 @@
 class Notice < ActiveRecord::Base
   has_and_belongs_to_many :users, :join_table =>'user_notices'
 
-  validates_inclusion_of :level, :in => %w{warning message}
+  TYPES = %w{message warning}
+  before_validation :set_default_notice_level
+  validates_inclusion_of :level, :in => TYPES
 
+  validates_presence_of :content
   before_save :add_to_all_users
 
+  def to_s
+    "#{global? ? "global" : "individual"} #{content}"
+  end
+
+  private
+
   def add_to_all_users
     self.users = User.all
   end
 
-  def to_s
-    "#{global? ? "global" : "individual"} #{content[0,20]}"
+  def set_default_notice_level
+    self.level ||= TYPES.first
   end
+
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 46706f6..402aad5 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -21,7 +21,8 @@ class User < ActiveRecord::Base
   validates_format_of :mail, :with => /^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$/i, :allow_nil => true
   validates_length_of :mail, :maximum => 60, :allow_nil => true
 
-  before_destroy Ensure_not_used_by.new(:hosts)
+  before_destroy Ensure_not_used_by.new(:hosts), :ensure_admin_is_not_deleted
+
   validate :name_used_in_a_usergroup
   before_validation :prepare_password
 
@@ -94,7 +95,7 @@ class User < ActiveRecord::Base
     auth_source and auth_source.can_set_password?
   end
 
-  private
+ private
 
   def prepare_password
     unless password.blank?
@@ -113,4 +114,15 @@ class User < ActiveRecord::Base
     end
   end
 
+  # The internal Admin Account is always available
+  # this is required as when not using external authentication, the systems logs you in with the
+  # admin account automatically
+  def ensure_admin_is_not_deleted
+    if login == "admin"
+      errors.add_to_base "Can't Delete Internal Admin account"
+      logger.warn "Unable to delete Internal Admin Account"
+      return false
+    end
+  end
+
 end
diff --git a/app/views/common/_notice.erb b/app/views/common/_notice.erb
index b3aed1d..67de688 100644
--- a/app/views/common/_notice.erb
+++ b/app/views/common/_notice.erb
@@ -1,22 +1,20 @@
-<% unless (@notices.empty?) -%>
-  <div id="notice" style="clear:both;">
-    <% field_set_tag "Notifications" do %>
-      <table width="100%">
-        <% for notice in  @notices-%>
-          <tr>
-            <td style="width:1%"><img src="images/<%= notice.level -%>.png"></td>
-            <td>
-              <%= notice.content %>
-            </td>
-            <td align="right" style="width:1%">
-              <% link_to notice_ack_path(notice) do -%>
-                <img src="images/close_hl.png">
-              <% end -%>
-            </td>
-          </tr>
-        <% end %>
-      </table>
-    <% end %>
-  </div>
-  <%= link_to_function "Toggle notifications section", toggle_div(:notice), :style => "float:right;color:red"%>
-<% end %>
+<div id="notice" style="clear:both;">
+  <% field_set_tag "Notifications" do %>
+    <table width="100%">
+      <% for notice in  @notices-%>
+        <tr>
+          <td style="width:1%"><img src="images/<%= notice.level -%>.png"></td>
+          <td>
+            <%= truncate notice.content, 100 %>
+          </td>
+          <td align="right" style="width:1%">
+            <% link_to notice, :method => :delete do -%>
+              <img src="images/close_hl.png">
+            <% end -%>
+          </td>
+        </tr>
+      <% end %>
+    </table>
+  <% end %>
+</div>
+<%= link_to_function "Toggle notifications section", toggle_div(:notice), :style => "float:right;color:red"%>
diff --git a/app/views/layouts/standard.rhtml b/app/views/layouts/standard.rhtml
index dffa414..64b833c 100644
--- a/app/views/layouts/standard.rhtml
+++ b/app/views/layouts/standard.rhtml
@@ -17,7 +17,7 @@
     <%= render "common/searchbar" rescue "<!-- The search facility is not available in this controllers -->" unless params[:action] == 'show' %>
     <%= content_tag('div', flash[:foreman_error],  :class => 'flash error')  if flash[:foreman_error] -%>
     <%= content_tag('div', flash[:foreman_notice], :class => 'flash notice') if flash[:foreman_notice] -%>
-    <%= render 'common/notice' -%>
+    <%= render 'common/notice' unless @notices.empty? -%>
     <div id="content">
       <%- if show_title? -%>
         <h1><%=h yield(:title) %></h1>
diff --git a/config/routes.rb b/config/routes.rb
index bc98c41..5989c53 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -18,7 +18,7 @@ ActionController::Routing::Routes.draw do |map|
       :multiple_enable => :get, :submit_multiple_enable => :post}
   map.dashboard '/dashboard', :controller => 'dashboard'
   map.statistics '/statistics', :controller => 'statistics'
-  map.notice_ack '/acknowlege/:id', :controller => 'notices', :action => 'acknowledge'
+  map.resources :notices, :only => :destroy
   map.resources :audits
   map.resources :usergroups
   map.resources :lookup_keys
diff --git a/test/functional/notices_controller_test.rb b/test/functional/notices_controller_test.rb
index 8f780ef..2ec4b68 100644
--- a/test/functional/notices_controller_test.rb
+++ b/test/functional/notices_controller_test.rb
@@ -2,22 +2,21 @@ require 'test_helper'
 
 class NoticesControllerTest < ActionController::TestCase
   def setup
-    #User.current = User.first
     @notice = Notice.create :global => false, :content => "hello", :level => "message"
     @request.env['HTTP_REFERER'] = hosts_path
   end
 
   def test_acknowledge_for_global
-    @notice = Notice.create :global => true, :content => "hello", :level => "message"
+    @new_notice = Notice.create :global => true, :content => "hello", :level => "warning"
     original = Notice.count
-    get :acknowledge, {:id => @notice.id}, set_session_user
+    delete :destroy, {:id => @new_notice}, set_session_user
     final = Notice.count
     assert original == final + 1
   end
 
   def test_acknowledge_for_individual
     original = User.current.notices.count
-    get :acknowledge, {:id => @notice.id}, set_session_user
+    delete :destroy, {:id => @notice}, set_session_user
     final = User.current.notices.count
     assert (original == final + 1)
   end
@@ -25,7 +24,7 @@ class NoticesControllerTest < ActionController::TestCase
   def test_notice_is_finally_deleted
     for user in User.all do
       User.current = user
-      get :acknowledge, {:id => @notice.id}, {:user => user}
+      delete :destroy, {:id => @notice}, set_session_user
     end
     assert Notice.count == 0
   end
diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb
index ca4616f..22be82e 100644
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -82,5 +82,9 @@ class UserTest < ActiveSupport::TestCase
     assert_equal nil, User.try_to_login("anything", "")
   end
   # couldn't continue testing the rest of login method cause use auth_source.authenticate, which is not implemented yet
+
+  test  "should not be able to delete the admin account" do
+    assert !User.find_by_login("admin").delete
+  end
 end
 
-- 
1.7.2.3

