On 01/18/2016 12:23 PM, Pavel Březina wrote:
On 01/18/2016 10:54 AM, Lukas Slebodnik wrote:On (18/01/16 10:18), Jakub Hrozek wrote:On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:On (15/01/16 15:06), Sumit Bose wrote:On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:On (15/01/16 10:21), Pavel Březina wrote:On 01/14/2016 06:37 PM, Sumit Bose wrote:On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:On 01/13/2016 05:12 PM, Sumit Bose wrote:@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item, if (ctx == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n"); - ctx->ret = ERR_INTERNAL; return false; } @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item, if (ctx == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n"); - ctx->ret = ERR_INTERNAL; return false; }it looks like the last 2 hunks are not available in the latest tar ball.Sorry about that. Added.and there are some trailing whitespaces in patch 9 at line 41.Fixed by git. I also attached two more patches then converts usn into number and then use in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) && (entryUSN != cur) as per Thierry request. It could probably be squashed and replace some previous patches but I think sending it as new patches is easier for you to review, given the previous patches are basically acked.+ errno = 0; usn_number = strtoul(usn, &endptr, 10); - if ((endptr == NULL || (*endptr == '\0' && endptr != usn)) - && (usn_number > srv_opts->last_usn)) { - srv_opts->last_usn = usn_number; + if (endptr != NULL && *endptr != '\0') {Currently an empty string in usn would result in usn_number==0, I wonder if this is valid or if it would be better to error out in this case?Zero value will not change anything since we always compare if the current usn value is lower than the new one, we switch them only in such case. So I think it is not necessary to error out in this case since strtoul succeeds.Otherwise all looks good, CI and Coverity test are currently running. bye, Sumit+ DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn); + return; + } else if (errno != 0) { + ret = errno; + DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]: %s\n", + usn, ret, sss_strerror(ret)); + return; }There are 4 failed test from freeipa sudo test suite. test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group =================================== FAILURES =================================== ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________ self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0> def test_sudo_rule_restricted_to_one_hostmask(self): if self.__class__.skip_hostmask_based: raise pytest.skip("Hostmask could not be detected") result1 = self.list_sudo_commands("testuser1")assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_textE assert '(ALL : ALL) NOPASSWD: ALL' in '' E + where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text test_integration/test_sudo.py:295: AssertionError ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______ self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990> def test_sudo_rule_restricted_to_running_as_single_local_user(self): result1 = self.list_sudo_commands("testuser1", verbose=True)assert "RunAsUsers: localuser" in result1.stdout_textThere are just too many options. I tested RunAsUsers and RunAsUsers but only with IPA users and groups. I looks like the handling for the ipasudorunasextuser and ipasudorunasextgroup attributes are missing.That's the purpose of automated tests :-) To cover all use-case.These use cases are omitted by design.We might still need to at least document this change. But with a recent IPA server, I see: [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --externaluser=jhrozek readfiles ipa: ERROR: invalid 'externaluser': this option has been deprecated. [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternalgroup=jhrozek readfiles ipa: ERROR: invalid 'runasexternalgroup': this option has been deprecated. [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternaluser=jhrozek readfiles ipa: ERROR: invalid 'runasexternaluser': this option has been deprecated.We might ask whether Dreamworks need this feature. Otherwise these patches will not help them.We talked with Jakub about it and decided that we will add support of ipaSudoRunAsExt* attributes. So with this patch set only hostmask test should fail, we will not add support for hostmask unless requested.
The previous tarball contained one patch that was already pushed...
patches.tgz
Description: GNU Zip compressed data
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
