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_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.
>
> 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.
So I guess we should document that if you use a new sssd version with an
old IPA server AND you use these deprecated options you should revert to
the old schema?
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
