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_text
>> E 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_text
>
>There 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.
Furtunatelly, Tomas wrote quite good test.
But there still might be other use cases which are not covered.
Because we cannot see a code coverage :-(
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
