On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote: > On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote: > > On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: > > > On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: > > > > All issues fixed unless noted below... The attached patches are tested > > > > to work. > > > > > > > > On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: > > > > > > > >> - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure > > > >> (although I know slapi_ch_malloc() currently just aborts on failure, I > > > >> think that is plain wrong which is why I would prefer using > > > >> malloc/strdup, but well, I guess this is not something I am going to > > > >> care too much about for now). > > > > Not fixed. > > > > > > > >> - Is the logic with auth_type_used correct ? > > > >> At least the name of the variable sounds misleading, because you set it > > > >> only if the authentication was successful but not set if it was 'used' > > > >> but was unsuccessful. Made me look at it multiple times to reconstuct > > > >> the logic. The var name could be better, however I also want a comment > > > >> that explain exactly how we are going to manage authentication and > > > >> fallbacks here as that logic is critical and is useful for anyone that > > > >> is going to have to change this code later in order not to break it. > > > > The variable is now gone. I re-factored the section to make the logic > > > > clearer and put a nice big comment up top. > > > > > > > >> - General question: how does this PRE_BIND plugin interact with > > > >> ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? > > > >> Are you going to cause that plugin not to run by returning a result > > > >> directly in this function ? > > > >> Or is this plugin configured to run only after the previous one went > > > >> through ? > > > >> I ask because I do not see any ordering information in the cn=config > > > >> plugin configuration so it is not immediately clear to me. > > > > That is a good question for Nathan since he wrote this part of the > > > > code... > > > We would need to set the precedence if you want a predictable/guaranteed > > > execution order. If a pre-BIND plug-in callback returns non-zero (which > > > you should do when the plug-in sends the result to the client directly), > > > it will cause other pre-bind plug-ins to not be called. This is > > > actually how all pre-op plug-ins work. If a pre-op callback returns an > > > error, we don't call the rest of the pre-op plug-ins in the list. > > > > Ok, but this does not answer my question. > > We definitely need to *always* run our other preop plugin as we do > > sanity checks like verifying if the user is enabled/disabled etc... > > Also we need to understand how to deal with migrating password auth when > > OTP is enabeld. > > > > TBH I think we should not have a separate OTP-auth plugin but we should > > probably have a single plugin that handles authentication and the 2 > > should be merged. Keeping them separate is going to cause more harm than > > good with unexpected interactions. > > > > We could still have 2 plugins and simply move the prebind action > > currently don in ipa-pwd-extop to the otp plugin by making some more > > code common. But it is probably easier to just merge OTP into > > ipa-pwd-extop right now than try to do a huge refactoring. We can always > > refactor the ipa-pwd-extop plugin later. > > The attached patches encompass an initial review of the companion daemon > and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp > into ipa-pwd-extop appears to have broken something during install, but > I don't have enough familiarity with 389 to understand what I've broken. > If I upgrade after an install, it appears to work. > > An RPM with the patches is available here: > http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935 > > Nathan / Rob / Simo, could you take a look and see what might be broken > in ipa-pwd-extop?
how did it fail ? Do you have an install log file that shows the error ? Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ Freeipa-devel mailing list Freeipafirstname.lastname@example.org https://www.redhat.com/mailman/listinfo/freeipa-devel