commit b2b4729059ff89327d7cd86d2e47664a2ce57c46
Author: Petr Chalupa <pchalupa@redhat.com>
Date:   Fri Jun 29 11:25:54 2012 +0200

    api v1 - render errors with rabl
    
    better detection of permission failure in model
    fix ApiConstraints
    catch bad routes in api and return json

diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb
index d8444bbdc..ef65dc442 100644
--- a/app/controllers/api/base_controller.rb
+++ b/app/controllers/api/base_controller.rb
@@ -6,37 +6,42 @@ module Api
 
     respond_to :json
 
-    def process_error(options = { })
-      options[:json_code] ||= :unprocessable_entity
-
-      errors = if options[:error]
-                 options[:error]
-               else
-                 options[:object] ||= get_resource || raise("No error to process")
-                 if options[:object].respond_to?(:errors)
-                   #TODO JSON resposne should include the real errors, not the pretty full messages
-                   logger.info "Failed to save: #{options[:object].errors.full_messages.join(", ")}"
-                   options[:object].errors.full_messages
-                 else
-                   raise("No error to process")
-                 end
-               end
-
-      # set 403 status on permission errors
-      if errors.any? { |error| error =~ /You do not have permission/ }
-        options[:json_code] = :forbidden
-      end
+    rescue_from StandardError, :with => lambda { |error|
+      Rails.logger.error "#{error.message} (#{error.class})\n#{error.backtrace.join("\n")}"
+      render_error 'standard_error', :status => 500, :locals => { :error => error }
+    }
+
+    def get_resource
+      instance_variable_get :"@#{resource_name}" or raise 'no resource loaded'
+    end
 
-      render :json => { "errors" => errors }, :status => options[:json_code]
+    def resource_name
+      controller_name.singularize
+    end
+
+    def resource_class
+      @resource_class ||= resource_name.camelize.constantize
     end
 
+    protected
+
+    def process_resource_error(options = { })
+      resource = options[:resource] || get_resource
+
+      raise 'resource have no errors' if resource.errors.empty?
+
+      if resource.permission_failed?
+        deny_access
+      else
+        render_error 'unprocessable_entity', :status => :unprocessable_entity
+      end
+    end
 
-    def process_response(condition, response = nil)
+    def process_response(condition, response = get_resource)
       if condition
-        response ||= get_resource
         respond_with response
       else
-        process_error
+        process_resource_error
       end
     end
 
@@ -45,11 +50,15 @@ module Api
 
       if SETTINGS[:login]
         unless User.current
-          user_to_login = nil
-          if result = authenticate_with_http_basic { |u, p| user_to_login = u; User.try_to_login(u, p) }
+          user_login = nil
+          result     = authenticate_with_http_basic do |u, p|
+            user_login = u
+            User.try_to_login(u, p)
+          end
+          if result
             User.current = result
           else
-            process_error :error => "Unable to authenticate user %s" % user_to_login, :json_code => :unauthorized
+            render_error 'unauthorized', :status => :unauthorized, :locals => { :user_login => user_login }
             return false
           end
         end
@@ -62,23 +71,10 @@ module Api
     end
 
     def deny_access
-      process_error :error => "Access denied", :json_code => :forbidden
+      render_error 'access_denied', :status => :forbidden
       false
     end
 
-    def get_resource
-      instance_variable_get :"@#{resource_name}" or raise 'no resource loaded'
-    end
-
-    def resource_name
-      controller_name.singularize
-    end
-
-    def resource_class
-      @resource_class ||= resource_name.camelize.constantize
-    end
-
-    protected
     # searches for a resource based on its name and assign it to an instance variable
     # required for models which implement the to_param method
     #
@@ -87,26 +83,23 @@ module Api
     def find_resource
       finder, key = case
                       when (id = params[:"#{resource_name}_id"]).present?
-                        [:find_by_id, id]
+                        ['id', id]
                       when (name = params[:"#{resource_name}_name"]).present?
-                        [:find_by_name, name]
+                        ['name', name]
                       else
                         [nil, nil]
                     end
-      resource = resource_class.send(finder, key) if finder
 
-      if finder && resource
+      resource = resource_class.send(:"find_by_#{finder}", key) if finder
+
+      if resource
         return instance_variable_set(:"@#{resource_name}", resource)
       else
-        not_found and return false
+        render_error 'not_found', :status => :not_found, :locals => { :finder => finder, :key => key } and
+            return false
       end
     end
 
-    def not_found(exception = nil)
-      logger.debug "not found: #{exception}" if exception
-      head :status => 404
-    end
-
     def set_default_response_format
       request.format = :json if params[:format].nil?
     end
@@ -119,5 +112,12 @@ module Api
       end
     end
 
+    def api_version
+      raise NotImplementedError
+    end
+
+    def render_error(error, options = { })
+      render options.merge(:template => "/api/v#{api_version}/errors/#{error}")
+    end
   end
 end
diff --git a/app/controllers/api/v1/architectures_controller.rb b/app/controllers/api/v1/architectures_controller.rb
index b3f43cb02..c16b0da74 100644
--- a/app/controllers/api/v1/architectures_controller.rb
+++ b/app/controllers/api/v1/architectures_controller.rb
@@ -1,6 +1,6 @@
 module Api
   module V1
-    class ArchitecturesController < BaseController
+    class ArchitecturesController < V1::BaseController
       include Foreman::Controller::AutoCompleteSearch
       before_filter :find_resource, :only => %w{show update destroy}
 
diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb
index c187f9f58..11185500e 100644
--- a/app/controllers/api/v1/base_controller.rb
+++ b/app/controllers/api/v1/base_controller.rb
@@ -1,6 +1,9 @@
 module Api
   module V1
     class BaseController < Api::BaseController
+      def api_version
+        '1'
+      end
     end
   end
 end
diff --git a/app/controllers/api/v1/bookmarks_controller.rb b/app/controllers/api/v1/bookmarks_controller.rb
index f287efe99..76f7989c2 100644
--- a/app/controllers/api/v1/bookmarks_controller.rb
+++ b/app/controllers/api/v1/bookmarks_controller.rb
@@ -1,6 +1,6 @@
 module Api
   module V1
-    class BookmarksController < BaseController
+    class BookmarksController < V1::BaseController
       before_filter :find_resource, :only => [:show, :update, :destroy]
 
       api :GET, "/bookmarks/", "List all bookmarks."
diff --git a/app/controllers/api/v1/home_controller.rb b/app/controllers/api/v1/home_controller.rb
index 7dbc37f3d..1ee905f19 100644
--- a/app/controllers/api/v1/home_controller.rb
+++ b/app/controllers/api/v1/home_controller.rb
@@ -1,6 +1,6 @@
 module Api
   module V1
-    class HomeController < BaseController
+    class HomeController < V1::BaseController
 
       def index
       end
@@ -8,6 +8,10 @@ module Api
       def status
       end
 
+      def route_error
+        render_error 'route_error', :status => :not_found
+      end
+
     end
   end
 end
diff --git a/app/controllers/api/v1/operatingsystems_controller.rb b/app/controllers/api/v1/operatingsystems_controller.rb
index 5407b87a8..edabb4b94 100644
--- a/app/controllers/api/v1/operatingsystems_controller.rb
+++ b/app/controllers/api/v1/operatingsystems_controller.rb
@@ -1,6 +1,6 @@
 module Api
   module V1
-    class OperatingsystemsController < BaseController
+    class OperatingsystemsController < V1::BaseController
 
       resource_description do
         name 'Operating systems'
diff --git a/app/models/authorization.rb b/app/models/authorization.rb
index 05616cf3d..e19fd6a66 100644
--- a/app/models/authorization.rb
+++ b/app/models/authorization.rb
@@ -31,9 +31,16 @@ module Authorization
     return true if User.current and User.current.allowed_to?("#{operation}_#{klasses}".to_sym)
 
     errors.add :base, "You do not have permission to #{operation} this #{klass}"
+    @permission_failed = operation
     false
   end
 
+  # @return false or name of failed operation
+  def permission_failed?
+    return false unless @permission_failed
+    @permission_failed
+  end
+
   private
   def enforce?
     return false if (User.current and User.current.admin?)
diff --git a/app/views/api/v1/errors/access_denied.json.rabl b/app/views/api/v1/errors/access_denied.json.rabl
new file mode 100644
index 000000000..ca68fe4d9
--- /dev/null
+++ b/app/views/api/v1/errors/access_denied.json.rabl
@@ -0,0 +1,3 @@
+object false
+
+node(:message) { "Access denied" }
diff --git a/app/views/api/v1/errors/not_found.json.rabl b/app/views/api/v1/errors/not_found.json.rabl
new file mode 100644
index 000000000..b3c64fdfb
--- /dev/null
+++ b/app/views/api/v1/errors/not_found.json.rabl
@@ -0,0 +1,3 @@
+object false
+
+node(:message) { "Resource #{controller.resource_name} not found by #{locals[:finder]} with value '#{locals[:key]}'" }
\ No newline at end of file
diff --git a/app/views/api/v1/errors/route_error.json.rabl b/app/views/api/v1/errors/route_error.json.rabl
new file mode 100644
index 000000000..55c75d04f
--- /dev/null
+++ b/app/views/api/v1/errors/route_error.json.rabl
@@ -0,0 +1,4 @@
+object false
+
+node(:message) { 'Not found' }
+
diff --git a/app/views/api/v1/errors/standard_error.json.rabl b/app/views/api/v1/errors/standard_error.json.rabl
new file mode 100644
index 000000000..0bf4131ec
--- /dev/null
+++ b/app/views/api/v1/errors/standard_error.json.rabl
@@ -0,0 +1,6 @@
+error = locals[:error]
+
+object error => :error
+
+attributes :message, :backtrace
+node(:class) { error.class.to_s }
\ No newline at end of file
diff --git a/app/views/api/v1/errors/unauthorized.json.rabl b/app/views/api/v1/errors/unauthorized.json.rabl
new file mode 100644
index 000000000..43309a774
--- /dev/null
+++ b/app/views/api/v1/errors/unauthorized.json.rabl
@@ -0,0 +1,3 @@
+object false
+
+node(:message) { "Unable to authenticate user %s" % locals[:user_login] }
diff --git a/app/views/api/v1/errors/unprocessable_entity.json.rabl b/app/views/api/v1/errors/unprocessable_entity.json.rabl
new file mode 100644
index 000000000..4e47563e3
--- /dev/null
+++ b/app/views/api/v1/errors/unprocessable_entity.json.rabl
@@ -0,0 +1,6 @@
+object resource = controller.get_resource
+
+attributes :id
+
+node(:errors) { resource.errors.to_hash }
+node(:full_messages) { resource.errors.full_messages }
diff --git a/app/views/api/v1/home/index.json.rabl b/app/views/api/v1/home/index.json.rabl
index 90a069657..c45cb70f7 100644
--- a/app/views/api/v1/home/index.json.rabl
+++ b/app/views/api/v1/home/index.json.rabl
@@ -1,5 +1,8 @@
 object false
 child(:links => "links") do
-  node(:bookmarks) { api_bookmarks_path }
   node(:status) { api_status_path }
+
+  %w(bookmarks architectures operatingsystems).each do |name|
+    node(name.to_sym) { send :"api_#{name}_path" }
+  end
 end
diff --git a/config/routes/api/v1.rb b/config/routes/api/v1.rb
index cb74f99f4..30e422935 100644
--- a/config/routes/api/v1.rb
+++ b/config/routes/api/v1.rb
@@ -13,6 +13,7 @@ Rails.application.routes.draw do |map|
 
       match '/', :to => 'home#index'
       match 'status', :to => 'home#status', :as => "status"
+      match '*other', :to => 'home#route_error'
     end
 #
   end
diff --git a/config/routes/api/v2.rb b/config/routes/api/v2.rb
index 39f737d3d..86d99ade0 100644
--- a/config/routes/api/v2.rb
+++ b/config/routes/api/v2.rb
@@ -2,10 +2,10 @@
 
 Rails.application.routes.draw do |map|
 
-#  namespace :api, :defaults => {:format => 'json'} do
-#    scope :module => :v2, :constraints => ApiConstraints.new(:version => 2, :default => false) do
-#      resources :bookmarks, :except => [:new, :edit]
-#    end
-#  end
+  #namespace :api, :defaults => {:format => 'json'} do
+  #  scope :module => :v2, :constraints => ApiConstraints.new(:version => 2) do
+  #    resources :bookmarks
+  #  end
+  #end
 
 end
diff --git a/lib/api_constraints.rb b/lib/api_constraints.rb
index 434d662fb..46f1e87b4 100644
--- a/lib/api_constraints.rb
+++ b/lib/api_constraints.rb
@@ -1,10 +1,15 @@
 class ApiConstraints
   def initialize(options)
     @version = options[:version]
-    @default = options[:default]
+    @default = options.has_key?(:default) ? options[:default] : false
   end
 
   def matches?(req)
-    @default || req.headers['Accept'].each {|h| return true if h.grep("version=#{@version}")}
+    req.accept =~ /version=([\d\.]+)/
+    if (version = $1) # version is specified in header
+      version == @version.to_s # are the versions same
+    else
+      @default # version is not specified, match if it's default version of api
+    end
   end
 end
diff --git a/test/functional/api/v1/architectures_controller_test.rb b/test/functional/api/v1/architectures_controller_test.rb
index c2d006d3c..d95e91871 100644
--- a/test/functional/api/v1/architectures_controller_test.rb
+++ b/test/functional/api/v1/architectures_controller_test.rb
@@ -8,9 +8,12 @@ class Api::V1::ArchitecturesControllerTest < ActionController::TestCase
      users(:one).roles = [Role.find_by_name('Anonymous'), Role.find_by_name('Viewer')]
   end
 
+  def user_one_as_manager
+     users(:one).roles = [Role.find_by_name('Anonymous'), Role.find_by_name('Manager')]
+  end
 
   test "should get index" do
-    as_user :admin do 
+    as_user :admin do
       get :index, {}
     end
     assert_response :success
@@ -18,7 +21,7 @@ class Api::V1::ArchitecturesControllerTest < ActionController::TestCase
   end
 
   test "should show architecture" do
-    as_user :admin do 
+    as_user :admin do
       get :show, {:id => architectures(:x86_64).to_param}
     end
     assert_response :success
@@ -34,7 +37,7 @@ class Api::V1::ArchitecturesControllerTest < ActionController::TestCase
   end
 
   test "should update architecture" do
-    as_user :admin do 
+    as_user :admin do
       put :update, {:id => architectures(:x86_64).to_param, :architecture => {} }
     end
     assert_response :success
@@ -66,9 +69,17 @@ class Api::V1::ArchitecturesControllerTest < ActionController::TestCase
     assert_response :forbidden
   end
 
+  test "user with manager rights should success to update an architecture" do
+    user_one_as_manager
+    as_user :one do
+      put :update, {:id => architectures(:x86_64).to_param, :architecture => {} }
+    end
+    assert_response :success
+  end
+
   test "user with viewer rights should succeed in viewing architectures" do
     user_one_as_anonymous_viewer
-    as_user :one do 
+    as_user :one do
       get :index, {}
     end
     assert_response :success
