Skip to content
Permalink
Browse files Browse the repository at this point in the history
[api] move write permission checks from controller to package and pro…
…ject model

This is just the first part doing the checks. Removing the old ones is another step.
  • Loading branch information
adrianschroeter committed Jul 5, 2013
1 parent d2c11a9 commit 06ad7fd
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 3 deletions.
8 changes: 8 additions & 0 deletions src/api/app/controllers/application_controller.rb
Expand Up @@ -442,6 +442,14 @@ def pass_to_backend( path = nil )
render :text => xml_text, :status => http_status
end

rescue_from Project::WritePermissionError do |exception|
render_error :status => 403, :errorcode => "modify_project_no_permission", :message => exception.message
end

rescue_from Package::WritePermissionError do |exception|

This comment has been minimized.

Copy link
@coolo

coolo Jul 5, 2013

Member

you don't need those if you call setup with 403

render_error :status => 403, :errorcode => "modify_package_no_permission", :message => exception.message
end

rescue_from Suse::Backend::NotFoundError, ActiveRecord::RecordNotFound do |exception|
render_error message: exception.message, status: 404, errorcode: 'not_found'
end
Expand Down
22 changes: 22 additions & 0 deletions src/api/app/models/package.rb
Expand Up @@ -15,6 +15,9 @@ class DeleteError < APIException
class SaveError < APIException
setup "package_save_error"
end
class WritePermissionError < APIException
setup "package_write_permission_error"
end
class ReadAccessError < APIException
setup 'unknown_package', 404, "Unknown package"
end
Expand Down Expand Up @@ -239,6 +242,14 @@ def is_locked?
return self.project.is_locked?
end

def check_write_access!
return if Rails.env.test? and User.current.nil? # for unit tests

unless User.current.can_modify_package? self
raise WritePermissionError, "No permission to modify package '#{self.name}' for user '#{User.current.login}'"
end
end

# NOTE: this is no permission check, should it be added ?
def can_be_deleted?
# check if other packages have me as devel package
Expand Down Expand Up @@ -285,14 +296,17 @@ def sources_changed
end

def add_package_kind( kinds )
check_write_access!
private_set_package_kind( kinds, nil, true )
end

def set_package_kind( kinds = nil )
check_write_access!
private_set_package_kind( kinds )
end

def set_package_kind_from_commit( commit )
check_write_access!
private_set_package_kind( nil, commit )
end

Expand Down Expand Up @@ -419,6 +433,7 @@ def resolve_devel_package
end

def update_from_xml( xmlhash )
check_write_access!
self.title = xmlhash.value('title')
self.description = xmlhash.value('description')
self.bcntsynctag = nil
Expand Down Expand Up @@ -616,6 +631,7 @@ def write_attributes(comment=nil)
end

def store(opts = {})
# no write access check here, since this operation may will disable this permission ...
@commit_opts = opts
save!
end
Expand Down Expand Up @@ -649,6 +665,7 @@ def find_attribute( namespace, name, binary=nil )
end

def add_user( user, role )
check_write_access!
unless role.kind_of? Role
role = Role.get_by_title(role)
end
Expand All @@ -669,6 +686,7 @@ def add_user( user, role )
end

def add_group( group, role )
check_write_access!
unless role.kind_of? Role
role = Role.get_by_title(role)
end
Expand Down Expand Up @@ -933,14 +951,17 @@ def expand_flags
end

def remove_all_persons
check_write_access!
self.package_user_role_relationships.delete_all
end

def remove_all_groups
check_write_access!
self.package_group_role_relationships.delete_all
end

def remove_role(what, role)
check_write_access!
if what.kind_of? Group
rel = self.package_group_role_relationships.where(bs_group_id: what.id)
else
Expand All @@ -954,6 +975,7 @@ def remove_role(what, role)
end

def add_role(what, role)
check_write_access!
self.transaction do
if what.kind_of? Group
self.package_group_role_relationships.create!(role: role, group: what)
Expand Down
41 changes: 38 additions & 3 deletions src/api/app/models/project.rb
Expand Up @@ -19,6 +19,9 @@ class UnknownObjectError < APIException
class SaveError < APIException
setup "project_save_error"
end
class WritePermissionError < APIException
setup "project_write_permission_error"
end
class ForbiddenError < APIException
setup("change_project_protection_level", 403,
"admin rights are required to raise the protection level of a project (it won't be safe anyway)")
Expand Down Expand Up @@ -247,6 +250,15 @@ def find_remote_project(name, skip_access=false)
end
end

def check_write_access!
return if Rails.env.test? and User.current.nil? # for unit tests

# the can_create_check is inconsistent with package class check_write_access! check
unless User.current.can_modify_project?(self) || User.current.can_create_project?(self.name)
raise WritePermissionError, "No permission to modify project '#{self.name}' for user '#{User.current.login}'"
end
end

def find_linking_projects
sql =<<-END_SQL
SELECT prj.*
Expand Down Expand Up @@ -289,6 +301,8 @@ def can_be_deleted?
end

def update_from_xml(xmlhash, force=nil)
check_write_access!

# check for raising read access permissions, which can't get ensured atm
unless self.new_record? || self.disabled_for?('access', nil, nil)
if FlagHelper.xml_disabled_for?(xmlhash, 'access')
Expand Down Expand Up @@ -892,6 +906,8 @@ def find_parent
end

def add_user( user, role )
check_write_access!

unless role.kind_of? Role
role = Role.get_by_title(role)
end
Expand All @@ -912,6 +928,8 @@ def add_user( user, role )
end

def add_group( group, role )
check_write_access!

unless role.kind_of? Role
role = Role.get_by_title(role)
end
Expand Down Expand Up @@ -1523,6 +1541,8 @@ def project_type
end

def set_project_type(project_type_name)
check_write_access!

mytype = DbProjectType.find_by_name(project_type_name)
return false unless mytype
self.type_id = mytype.id
Expand All @@ -1535,6 +1555,8 @@ def maintenance_project
end

def set_maintenance_project(project)
check_write_access!

if project.class == Project
self.maintenance_project_id = project.id
self.save!
Expand Down Expand Up @@ -1583,6 +1605,11 @@ def repositories_linking_project(tproj, backend)

# called either directly or from delayed job
def do_project_copy( params )
# set user if nil, needed for delayed job in Package model
User.current ||= User.find_by_login(params[:user])

check_write_access!

# copy entire project in the backend
begin
path = "/source/#{URI.escape(self.name)}"
Expand All @@ -1593,9 +1620,6 @@ def do_project_copy( params )
# we need to check results of backend in any case (also timeout error eg)
end

# set user if nil, needed for delayed job in Package model
User.current ||= User.find_by_login(params[:user])

# restore all package meta data objects in DB
backend_pkgs = Collection.find :package, :match => "@project='#{self.name}'"
backend_pkgs.each_package do |package|
Expand All @@ -1611,6 +1635,8 @@ def do_project_copy( params )
def do_project_release( params )
User.current ||= User.find_by_login(params[:user])

check_write_access!

packages.each do |pkg|
pkg.project.repositories.each do |repo|
next if params[:repository] and params[:repository] != repo.name
Expand Down Expand Up @@ -1647,6 +1673,8 @@ def user_has_role?(user, role)
end

def remove_role(what, role)
check_write_access!

if what.kind_of? Group
rel = self.project_group_role_relationships.where(bs_group_id: what.id)
else
Expand All @@ -1660,6 +1688,8 @@ def remove_role(what, role)
end

def add_role(what, role)
check_write_access!

self.transaction do
if what.kind_of? Group
self.project_group_role_relationships.create!(role: role, group: what)
Expand All @@ -1684,6 +1714,8 @@ def valid_name
end

def update_patchinfo(patchinfo, opts = {})
check_write_access!

opts[:enfore_issue_update] ||= false

# collect bugnumbers from diff
Expand Down Expand Up @@ -1721,6 +1753,8 @@ def update_patchinfo(patchinfo, opts = {})
end

def create_patchinfo_from_request(req)
check_write_access!

patchinfo = Package.new(:name => "patchinfo", :title => "Patchinfo", :description => "Collected packages for update")
self.packages << patchinfo
patchinfo.add_flag("build", "enable", nil, nil)
Expand Down Expand Up @@ -1755,6 +1789,7 @@ def create_patchinfo_from_request(req)

# updates packages automatically generated in the backend after submitting a product file
def update_product_autopackages
check_write_access!

backend_pkgs = Collection.find :id, :what => 'package', :match => "@project='#{self.name}' and starts-with(@name,'_product:')"
b_pkg_index = backend_pkgs.each_package.inject(Hash.new) {|hash,elem| hash[elem.name] = elem; hash}
Expand Down
11 changes: 11 additions & 0 deletions src/api/test/unit/project_test.rb
Expand Up @@ -28,6 +28,7 @@ def test_flags_to_axml


def test_add_new_flags_from_xml
User.current = users( :Iggy )

#precondition check
@project.flags.delete_all
Expand Down Expand Up @@ -88,6 +89,8 @@ def test_add_new_flags_from_xml


def test_delete_flags_through_xml
User.current = users( :Iggy )

#check precondition
assert_equal 2, @project.type_flags('build').size
assert_equal 2, @project.type_flags('publish').size
Expand All @@ -107,6 +110,8 @@ def test_delete_flags_through_xml


def test_store_axml
User.current = users( :Iggy )

original = @project.to_axml

#project is given as axml
Expand All @@ -131,6 +136,8 @@ def test_store_axml
end

def test_ordering
User.current = users( :Iggy )

#project is given as axml
axml = Xmlhash.parse(
"<project name='home:Iggy'>
Expand Down Expand Up @@ -175,6 +182,7 @@ def test_ordering
end

test "duplicated repos" do
User.current = users( :king )
orig = @project.render_axml

axml = Xmlhash.parse(
Expand All @@ -199,6 +207,7 @@ def test_ordering
end

test "duplicated repos with remote" do
User.current = users( :Iggy )
orig = @project.render_axml

xml = <<END
Expand All @@ -225,6 +234,7 @@ def test_ordering
assert_equal orig, @project.render_axml
end
test "not duplicated repos with remote" do
User.current = users( :Iggy )
xml = <<END
<project name="home:Iggy">
<title>Iggy"s Home Project</title>
Expand All @@ -250,6 +260,7 @@ def test_ordering
end

def test_create_maintenance_project_and_maintained_project
User.current = users( :king )
maintenance_project = Project.new(:name => 'Maintenance:Project')
assert_equal true, maintenance_project.set_project_type('maintenance')
assert_equal 'maintenance', maintenance_project.project_type()
Expand Down

0 comments on commit 06ad7fd

Please sign in to comment.