On Wed, Aug 24, 2016 at 10:48:21AM +0200, Fabiano Fidêncio wrote:
> On Wed, Aug 24, 2016 at 10:43 AM, Lukas Slebodnik <lslebodn(a)redhat.com> wrote:
> > On (24/08/16 10:37), Fabiano Fidêncio wrote:
> >>On Wed, Aug 24, 2016 at 10:33 AM, Lukas Slebodnik <lslebodn(a)redhat.com>
wrote:
> >>> On (24/08/16 00:59), Fabiano Fidêncio wrote:
> >>>>When saving the user there is a comparison between the "cased
alias"
> >>>>and the "lowercase password name". However, the first
doesn't use fully
> >>>>qualified name while the second does, resulting in a not expected
> >>>>override of the "nameAlias" attribute of a stored user when
trying to
> >>>>authenticate more than once using an alias.
> >>>>
> >>>>Resolves:
> >>>>https://fedorahosted.org/sssd/ticket/3134
> >>>>
> >>>>CI has passed:
http://sssd-ci.duckdns.org/logs/job/52/26/summary.html
> >>>>
> >>>>Best Regards,
> >>>>--
> >>>>Fabiano Fidêncio
> >>>
> >>> >From a6a38a856a15ec9854797c1df4369288f1bec6c1 Mon Sep 17 00:00:00
2001
> >>>>From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?=
<fidencio(a)redhat.com>
> >>>>Date: Tue, 23 Aug 2016 23:46:59 +0200
> >>>>Subject: [PATCH] PROXY: Use the fqname when converting to lowercase
> >>>>MIME-Version: 1.0
> >>>>Content-Type: text/plain; charset=UTF-8
> >>>>Content-Transfer-Encoding: 8bit
> >>>>
> >>>>When saving the user there is a comparison between the "cased
alias"
> >>>>and the "lowercase password name". However, the first
doesn't use fully
> >>>>qualified name while the second does, resulting in a not expected
> >>>>override of the "nameAlias" attribute of a stored user when
trying to
> >>>>authenticate more than once using an alias.
> >>>>
> >>>>Resolves:
> >>>>https://fedorahosted.org/sssd/ticket/3134
> >>>>
> >>>>Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
> >>>>---
> >>>> src/providers/proxy/proxy_id.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/src/providers/proxy/proxy_id.c
b/src/providers/proxy/proxy_id.c
> >>>>index 26f4d74..b0c8280 100644
> >>>>--- a/src/providers/proxy/proxy_id.c
> >>>>+++ b/src/providers/proxy/proxy_id.c
> >>>>@@ -256,7 +256,7 @@ static int save_user(struct sss_domain_info
*domain,
> >>>> }
> >>>>
> >>>> if (lowercase) {
> >>>>- lc_pw_name = sss_tc_utf8_str_tolower(attrs,
pwd->pw_name);
> >>>>+ lc_pw_name = sss_tc_utf8_str_tolower(attrs, real_name);
> >>>> if (lc_pw_name == NULL) {
> >>>> DEBUG(SSSDBG_OP_FAILURE, "Cannot convert name to
lowercase.\n");
> >>>> ret = ENOMEM;
> >>> I thought it would be a one-liner.
> >>> Nice catch and good job.
> >>>
> >>> I checked the code and we do the same in save_group.
> >>>
> >>> But I noticed that we do not use lowercase in save_group
> >>> but directly !dom->case_sensitive.
> >>>
> >>> I think you can do sa small refactoring and a little bit
> >>> unify functions save_user and save_group (of cource in separate patch.
> >>>
> >>> I would prefer if parameter "bool lowercase" could be removed
> >>> from function save_user. We can still use it as a local variable.
> >>> And we can use local variable "bool lowercase" in save_group
> >>> to improve readability inside save_group.
> >>
> >>Okay, I'll do the refactoring.
> >>May I consider this patch ACKed and ready for RHEL?
> >>
> > Sure
> >
> >>The refactoring patches will come later :-)
> >>
> > I do not assume that it will took a long time to do a refactoring.
> > That's the reason why I would prefer to do it sooner
> > and as part of this ticket :-)
>
> It always can be "Related to" :-)
> I just believe the fix can be pushed (and backported) as soon as
> possible, while the refactoring can be delayed by a few days if needed
> (mainly because I've returned the beaker machines and it will take me
> a day or so to get another ones ... and I want to be sure I won't
> break anything with that)
Yes, I would also prefer to push this patch now if it fixes the bug and
continue on the refactoring (without delay, it seems to be small and
would improve the proxy provider code).
>
> >
> > But If you are busy we can do it later
> > which usually means almost never(deferred).
>
> That's not the case.
+1 to fixing downstream now (and 'hot-patching' upstream also with this
patch) and continuing on improving upstream.