[api] Refactor SourceAttributeController

Rely more on routes and on the model classes doing the proper things,
this way we avoid duplicating code

src/api/app/controllers/source_attribute_controller.rb

@@ -11,6 +11,10 @@ class RemoteProject < APIException
   class InvalidAttribute < APIException
   end
 
+  class ChangeAttributeNoPermission < APIException
+    setup 403
+  end
+
   # GET
   # /source/:project/_attribute/:attribute
   # /source/:project/:package/_attribute/:attribute
@@ -32,27 +36,18 @@ def show
   # /source/:project/:package/:binary/_attribute/:attribute
   #--------------------------------------------------------
   def delete
-    # init
-    if params[:namespace].blank? || params[:name].blank?
-      render_error status: 400, errorcode: 'missing_attribute',
-                   message: 'No attribute got specified for delete'
-      return
-    end
-    ac = @attribute_container.find_attribute(params[:namespace], params[:name], @binary)
+    attrib = @attribute_container.find_attribute(@at.namespace, @at.name, @binary)
 
     # checks
-    unless ac
-      render_error(status: 404, errorcode: 'not_found',
-                   message: "Attribute #{params[:attribute]} does not exist") && return
+    unless attrib
+      raise ActiveRecord::RecordNotFound, "Attribute #{params[:attribute]} does not exist"
     end
-    unless User.current.can_create_attribute_in? @attribute_container, namespace: params[:namespace], name: params[:name]
-      render_error status: 403, errorcode: 'change_attribute_no_permission',
-                   message: "user #{user.login} has no permission to change attribute"
-      return
+    unless User.current.can_create_attribute_in? @attribute_container, @at
+      raise ChangeAttributeNoPermission, "User #{user.login} has no permission to change attribute"
     end
 
     # exec
-    ac.destroy
+    attrib.destroy
     render_ok
   end
 
@@ -95,12 +90,16 @@ def update
 
   protected
 
+  def attribute_type(name)
+    return if name.blank?
+    # if an attribute param is given, it needs to exist
+    AttribType.find_by_name!(name)
+  end
+
   def find_attribute_container
     # init and validation
     #--------------------
-    params[:user] = User.current.login if User.current
-    @binary = nil
-    @binary = params[:binary] if params[:binary]
+    @binary = params[:binary]
     # valid post commands
     if params[:package] && params[:package] != '_project'
       @attribute_container = Package.get_by_project_and_name(params[:project], params[:package], use_source: false)
@@ -110,19 +109,6 @@ def find_attribute_container
       @attribute_container = Project.get_by_name(params[:project])
     end
 
-    # is the attribute type defined at all ?
-    return if params[:attribute].blank?
-
-    # Valid attribute
-    aname = params[:attribute]
-    name_parts = aname.split(/:/)
-    if name_parts.length != 2
-      raise InvalidAttribute, "attribute '#{aname}' must be in the $NAMESPACE:$NAME style"
-    end
-    # existing ?
-    AttribType.find_by_name!(params[:attribute])
-    # only needed for a get request
-    params[:namespace] = name_parts[0]
-    params[:name] = name_parts[1]
+    @at = attribute_type(params[:attribute])
   end
 end

src/api/app/controllers/webui/project_controller.rb

@@ -429,7 +429,7 @@ def monitor
     @packagenames = @packagenames.flatten.uniq.sort
 
     ## Filter for PackageNames ####
-    @packagenames.reject! { |name| !filter_matches?(name, @name_filter) } if @name_filter.present?
+    @packagenames.select! { |name| filter_matches?(name, @name_filter) } if @name_filter.present?
 
     packagename_hash = {}
     @packagenames.each { |p| packagename_hash[p.to_s] = 1 }
@@ -590,13 +590,13 @@ def edit_comment_form
   def edit_comment
     @package = @project.find_package(params[:package])
 
-    unless User.current.can_create_attribute_in? @package, namespace: 'OBS', name: 'ProjectStatusPackageFailComment'
+    at = AttribType.find_by_namespace_and_name!('OBS', 'ProjectStatusPackageFailComment')
+    unless User.current.can_create_attribute_in? @package, at
       @comment = params[:last_comment]
       @error = "Can't create attributes in #{@package}"
       return
     end
 
-    at = AttribType.find_by_namespace_and_name!('OBS', 'ProjectStatusPackageFailComment')
     attr = @package.attribs.where(attrib_type: at).first_or_initialize
     v = attr.values.first_or_initialize
     v.value = params[:text]

src/api/app/models/attrib_type.rb

@@ -6,6 +6,8 @@ class AttribType < ApplicationRecord
   class UnknownAttributeTypeError < APIException
     setup 'unknown_attribute_type', 404, 'Unknown Attribute Type'
   end
+  class InvalidAttributeError < APIException
+  end
 
   #### Attributes
   #### Associations macros (Belongs to, Has one, Has many)
@@ -29,7 +31,7 @@ def self.find_by_name!(name)
   def self.find_by_name(name, or_fail = false)
     name_parts = name.split(/:/)
     if name_parts.length != 2
-      raise ArgumentError, "attribute '#{name}' must be in the $NAMESPACE:$NAME style"
+      raise InvalidAttributeError, "Attribute '#{name}' must be in the $NAMESPACE:$NAME style"
     end
     find_by_namespace_and_name(name_parts[0], name_parts[1], or_fail)
   end

src/api/app/models/user.rb

@@ -450,6 +450,11 @@ def can_modify_package?(package, ignore_lock = nil)
     false
   end
 
+  def can_modify_attribute_container?(object)
+    return can_modify_project?(object) if object.is_a? Project
+    return can_modify_package?(object)
+  end
+
   def can_modify_user?(user)
     is_admin? || self == user
   end
@@ -484,6 +489,12 @@ def can_modify_attribute_definition?(object)
     can_create_attribute_definition?(object)
   end
 
+  def attribute_modifier_rule_matches?(rule)
+    return false if rule.user && rule.user != self
+    return false if rule.group && !is_in_group?(rule.group)
+    true
+  end
+
   def can_create_attribute_definition?(object)
     object = object.attrib_namespace if object.is_a? AttribType
     unless object.is_a? AttribNamespace
@@ -493,49 +504,28 @@ def can_create_attribute_definition?(object)
     return true if is_admin?
 
     abies = object.attrib_namespace_modifiable_bies.includes([:user, :group])
-    abies.each do |mod_rule|
-      next if mod_rule.user && mod_rule.user != self
-      next if mod_rule.group && !is_in_group?(mod_rule.group)
-      return true
-    end
+    abies.any? { |rule| attribute_modifier_rule_matches?(rule) }
+  end
 
-    false
+  def attribute_modification_rule_matches?(rule, object)
+    return false unless attribute_modifier_rule_matches?(rule)
+    return false if rule.role && !has_local_role?(rule.role, object)
+    true
   end
 
-  # rubocop:disable Style/GuardClause
-  def can_create_attribute_in?(object, opts)
+  def can_create_attribute_in?(object, atype)
     if !object.is_a?(Project) && !object.is_a?(Package)
       raise ArgumentError, "illegal parameter type to User#can_change?: #{object.class.name}"
     end
-    raise ArgumentError, 'no namespace given' unless opts[:namespace]
-    raise ArgumentError, 'no name given' unless opts[:name]
-
-    # find attribute type definition
-    atype = AttribType.find_by_namespace_and_name!(opts[:namespace], opts[:name])
 
     return true if is_admin?
 
-    # check modifiable_by rules
     abies = atype.attrib_type_modifiable_bies.includes([:user, :group, :role])
-    if abies.empty?
-      # no rules set for attribute, just check package maintainer rules
-      if object.is_a? Project
-        return can_modify_project?(object)
-      else
-        return can_modify_package?(object)
-      end
-    else
-      abies.each do |mod_rule|
-        next if mod_rule.user && mod_rule.user != self
-        next if mod_rule.group && !is_in_group?(mod_rule.group)
-        next if mod_rule.role && !has_local_role?(mod_rule.role, object)
-        return true
-      end
-    end
-    # never reached
-    false
+    # no rules -> maintainer
+    return can_modify_attribute_container?(object) if abies.empty?
+
+    abies.any? { |rule| attribute_modification_rule_matches?(rule, object) }
   end
-  # rubocop:enable Style/GuardClause
 
   def can_download_binaries?(package)
     can?(:download_binaries, package)

src/api/config/routes.rb

@@ -731,7 +731,7 @@ def self.public_or_about_path?(request)
   controller :source_attribute do
     get 'source/:project(/:package(/:binary))/_attribute(/:attribute)' => :show, constraints: cons
     post 'source/:project(/:package(/:binary))/_attribute(/:attribute)' => :update, constraints: cons, as: :change_attribute
-    delete 'source/:project(/:package(/:binary))/_attribute(/:attribute)' => :delete, constraints: cons
+    delete 'source/:project(/:package(/:binary))/_attribute/:attribute' => :delete, constraints: cons
   end
 
   # project level

src/api/test/functional/attributes_test.rb

@@ -312,7 +312,7 @@ def test_attrib_delete_permissions
     delete '/source/home:tom/_attribute/OBS:VeryImportantProject'
     assert_response 403
     delete '/source/home:tom/_attribute/?namespace=OBS&name=VeryImportantProject'
-    assert_response 403
+    assert_response 404
   end
 
   def test_create_attributes_project
@@ -474,8 +474,7 @@ def test_create_attributes_package
 
     # invalid operations
     delete '/source/kde4/kdelibs/kdelibs-devel/_attribute'
-    assert_response 400
-    assert_xml_tag tag: 'status', attributes: { code: 'missing_attribute' }
+    assert_response 404
     delete '/source/kde4/kdelibs/kdelibs-devel/_attribute/OBS_Maintained'
     assert_response 400
     assert_xml_tag tag: 'status', attributes: { code: 'invalid_attribute' }