diff options
| -rw-r--r-- | cobbler/modules/authz_ownership.py | 69 | ||||
| -rw-r--r-- | cobbler/remote.py | 5 | ||||
| -rw-r--r-- | tests/tests.py | 31 |
3 files changed, 85 insertions, 20 deletions
diff --git a/cobbler/modules/authz_ownership.py b/cobbler/modules/authz_ownership.py index 3befc4a..ff5c016 100644 --- a/cobbler/modules/authz_ownership.py +++ b/cobbler/modules/authz_ownership.py @@ -50,6 +50,47 @@ def __parse_config(): alldata[g][o] = 1 return alldata +def __authorize_kickstart(api_handle, user, user_groups, file): + # the authorization rules for kickstart editing are a bit + # of a special case. Non-admin users can edit a kickstart + # only if all objects that depend on that kickstart are + # editable by the user in question. + # + # Example: + # if Pinky owns ProfileA + # and the Brain owns ProfileB + # and both profiles use the same kickstart template + # and neither Pinky nor the Brain is an admin + # neither is allowed to edit the kickstart template + # because they would make unwanted changes to each other + # + # In the above scenario the UI will explain the problem + # and ask that the user asks the admin to resolve it if required. + # NOTE: this function is only called by authorize so admin users are + # cleared before this function is called. + + lst = api_handle.find_profile(kickstart=kickstart, return_list=True) + lst.extend(api_handle.find_system(kickstart=kickstart, return_list=True)) + for obj in lst: + if not __is_user_allowed(obj, user, user_groups): + return 0 + return 1 + +def __is_user_allowed(obj, user, user_groups): + if obj.owners == []: + # no ownership restrictions, cleared + return 1 + for allowed in obj.owners: + if user == allowed: + # user match + return 1 + # else look for a group match + for group in user_groups: + if group == allowed and user in user_groups[group]: + return 1 + return 0 + + def authorize(api_handle,user,resource,arg1=None,arg2=None): """ @@ -60,10 +101,10 @@ def authorize(api_handle,user,resource,arg1=None,arg2=None): user_groups = __parse_config() # classify the type of operation - save_or_remove = False - for criteria in ["save_","remove_","modify_"]: + modify_operation = False + for criteria in ["save","remove","modify","write","edit"]: if resource.find(criteria) != -1: - save_or_remove = True + modify_operation = True # FIXME: is everyone allowed to copy? I think so. # FIXME: deal with the problem of deleted parents and promotion @@ -83,14 +124,19 @@ def authorize(api_handle,user,resource,arg1=None,arg2=None): # if the user isn't anywhere in the file, reject regardless # they can still use read-only XMLRPC return 0 - if not save_or_remove: + if not modify_operation: # sufficient to allow access for non save/remove ops to all # users for now, may want to refine later. return 1 - # now we have a save_or_remove op, so we must check ownership + # now we have a modify_operation op, so we must check ownership # of the object. remove ops pass in arg1 as a string name, # saves pass in actual objects, so we must treat them differently. + # kickstarts are even more special so we call those out to another + # function, rather than going through the rest of the code here. + + if resource.find("kickstart"): + return authorize_kickstart(api_handle,user,user_groups,arg1) obj = None if resource.find("remove") != -1: @@ -109,18 +155,7 @@ def authorize(api_handle,user,resource,arg1=None,arg2=None): if obj.owners is None or obj.owners == []: return 1 - # otherwise, ownership by user/group - for allowed in obj.owners: - if user == allowed: - # user match - return 1 - for group in user_groups: - if group == allowed and user in user_groups[group]: - return 1 - - # can't find user or group in ownership list and ownership is defined - # so reject the operation - return 0 + return __is_user_allowed(user,group,user_groups) if __name__ == "__main__": diff --git a/cobbler/remote.py b/cobbler/remote.py index 5e3fb83..ebb01f7 100644 --- a/cobbler/remote.py +++ b/cobbler/remote.py @@ -1050,7 +1050,10 @@ class CobblerReadWriteXMLRPCInterface(CobblerXMLRPCInterface): """ self.log("read_or_write_kickstart_template",name=kickstart_file,token=token) - self.check_access(token,"read_or_write_kickstart_templates",kickstart_file,is_read) + if is_read: + self.check_access(token,"read_kickstart",kickstart_file) + else: + self.check_access(token,"modify_kickstart",kickstart_file) if kickstart_file.find("..") != -1 or not kickstart_file.startswith("/"): raise CX(_("tainted file location")) diff --git a/tests/tests.py b/tests/tests.py index c3cbea7..f9bc368 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -105,7 +105,9 @@ class Ownership(BootTest): def test_ownership_params(self): - # initially just test that we can set ownership on various components + fd = open("/tmp/test_cobbler_kickstart","w+") + fd.write("") + fd.close() distro = self.api.find_distro(name="testdistro0") profile = self.api.find_profile(name="testprofile0") @@ -113,7 +115,8 @@ class Ownership(BootTest): repo = self.api.find_repo(name="test_repo") self.assertTrue(distro.set_owners(["superlab","basement1"])) self.assertTrue(profile.set_owners(["superlab","basement1"])) - self.assertTrue(system.set_owners(["superlab","basement1"])) + self.assertTrue(profile.set_kickstart("/tmp/test_cobbler_kickstart")) + self.assertTrue(system.set_owners(["superlab","basement1","basement3"])) self.assertTrue(repo.set_owners([])) self.api.add_distro(distro) self.api.add_profile(profile) @@ -136,17 +139,41 @@ class Ownership(BootTest): fd.write("\n") fd.write("[superlab]\n") fd.write("superlab1 = 1\n") + fd.write("superlab2 = 1\n") fd.write("\n") fd.write("[basement]\n") fd.write("basement1 = 1\n") fd.write("basement2 = 1\n") + fd.write("basement3 = 1\n") fd.close() + xo = self.api.find_distro("testdistro0") xn = "testdistro0" ro = self.api.find_repo("test_repo") rn = "test_repo" + # WARNING: complex test explanation follows! + # we must ensure those who can edit the kickstart are only those + # who can edit all objects that depend on the said kickstart + # in this test, superlab & basement1 can edit test_profile0 + # superlab & basement1/3 can edit test_system0 + # the systems share a common kickstart record (in this case + # explicitly set, which is a bit arbitrary as they are parent/child + # nodes, but the concept is not limited to this). + # Therefore the correct result is that the following users can edit: + # admin1, superlab1, superlab2 + # And these folks can't + # basement1, basement2 + # Basement2 is rejected because the kickstart is shared by something + # basmeent2 can not edit. + + for user in [ "admin1", "superlab1", "superlab2", "basement1" ]: + self.assertTrue(1==authorize(self.api, user, "modify_kickstart", xo), "%s can modify_kickstart" % user) + + for user in [ "basement2", "dne" ]: + self.assertTrue(0==authorize(self.api, user, "modify_kickstart", xo), "%s can modify_kickstart" % user) + # ensure admin1 can edit (he's an admin) and do other tasks # same applies to basement1 who is explicitly added as a user # and superlab1 who is in a group in the ownership list |