Patch WFM and looks clean. My only question is on the overall prompting. Currently we have: * 1FA: - "Password" * 2FA (required): - "First Factor" - "Second Factor" * 2FA (optional): - "First Factor or Password" - "Second Factor, press return for Password authentication"
This all seems a little verbose to me. It also seems like it reveals technical details that don't matter and/or might confuse the user. Is there a reason not to do the following? * 1FA: - "Password" * 2FA (required): - "Password" - "Token Code" * 2FA (optional): - "Password" - "Token Code (optional)" This seems to me simpler, shorter and more descriptive. Alternatively: * 1FA: - "Password" * 2FA (required): - "Password" - "Second Factor" * 2FA (optional): - "Password" - "Second Factor (optional)" Whatever we decide to do, this patch has my ACK. So feel free to just change the string and merge it. On Mon, 2016-06-13 at 14:01 +0200, Sumit Bose wrote: > and now with patch ... > > On Mon, Jun 13, 2016 at 02:00:37PM +0200, Sumit Bose wrote: > > > > On Fri, Jun 10, 2016 at 02:28:16PM +0200, Lukas Slebodnik wrote: > > > > > > On (30/05/16 17:07), Sumit Bose wrote: > > > > > > > > Hi, > > > > > > > > this patch is the SSSD part of the Authentication Indicator > > > > related > > > > changes in FreeIPA. The basic part is that now it is possible > > > > to > > > > authenticate at will with either password or 2FA as described > > > > on > > > > https://fedorahosted.org/sssd/wiki/DesignDocs/PromptingForMulti > > > > pleAuthenticationTypes. > > > > > > > > To test you need a FreeIPA server build from the FreeIPA master > > > > branch > > > > and optionally Nathaniel's '[PATCH 0093] Enable service > > > > authentication > > > > indicator management' which is currently under review for the > > > > kvno > > > > related test. Additionally you need MIT Kerberos packages which > > > > contain > > > > the fix for https://bugzilla.redhat.com//show_bug.cgi?id=134030 > > > > 4. > > > > > > > > Since this patch changes how libkrb5 gets the password for > > > > password > > > > authentication with newer version of MIT Kerberos it would be > > > > nice if > > > > someone can run some regression tests with the AD or plain KRB5 > > > > auth_provider. > > > > > > > I can see failures in password change for AD provider. > > > I haven't tried KRB5 or IPA yet. > > > > > > > Thank you for testing and reviewing. I guess it was a general > > issue, the > > new patch should fix it. > > > > > > > > > > ... > > > > > > > > > > > + DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", > > > > c, > > > ^^^ > > > type of "c" is size_t an > > > therefore > > > there should be "%zu". > > > otherwise > > > there is a warning on 64 > > > bit platforms > > > CC src/providers/krb5/krb5_child-krb5_child.o > > > ../sssd/src/providers/krb5/krb5_child.c: In function > > > ‘sss_krb5_prompter’: > > > ../sssd/src/providers/krb5/krb5_child.c:665:17: error: format > > > ‘%u’ expects argument of type ‘unsigned int’, but argument 6 has > > > type ‘size_t {aka long unsigned int}’ [-Werror=format=] > > > DEBUG(SSSDBG_TRACE_ALL, "Prompt [%u][%s].\n", c, > > > ^~~~~ > > > cc1: all warnings being treated as errors > > > > Fixed as well. New version attached. > > > > bye, > > Sumit > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahos > ted.org _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
