[Freeipa-devel] [PATCH] 1006 detect expired passwords in forms login

[Martin Kosek]

On Tue, 2012-04-17 at 10:13 -0400, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Mon, 2012-04-16 at 09:34 -0400, Rob Crittenden wrote:
> >> Rob Crittenden wrote:
> >>> Petr Vobornik wrote:
> >>>> On 04/13/2012 09:28 PM, Rob Crittenden wrote:
> >>>>> When doing a forms-based login there is no notification that a password
> >>>>> needs to be reset. We don't currently provide a facility for that but we
> >>>>> should at least tell users what is going on.
> >>>>>
> >>>>> This patch adds an LDAP bind to test the password to see if it is
> >>>>> expired and returns the string "Password Expired" along with the 401 if
> >>>>> it is. I'm told this is all the UI will need to be able to identify this
> >>>>> condition.
> >>>>>
> >>>>> rob
> >>>>>
> >>>>
> >>>> UI can work with it. I have a patch ready. I'll send it when this will
> >>>> be ACKed.
> >>>>
> >>>> Some notes:
> >>>>
> >>>> 1) The error templates and the 'Password Expired' message are hardcoded
> >>>> to be English. It's fine at the moment. Will we internationalize them
> >>>> sometime in future? If so, we will run into the same problem again.
> >>>
> >>> No plans to. I can update the patch with a comment specifically to not
> >>> internationalize it if you'd like.
> >>>
> >>>> 2) conn.destroy_connection() won't be called if an exception occurs. Not
> >>>> sure if it is a problem, GC and __del__ should take care of it.
> >>>
> >>> Hmm, this is due to a late stage change I made. I originally had this
> >>> broken out into two blocks where the only thing done in the first
> >>> try/except block was the connection, so the only exception that could
> >>> happen was a failed connection.
> >>>
> >>> That isn't true any more. I'll update the patch.
> >>
> >> And here you go.
> >>
> >> rob
> >
> > I still think that deciding based on error message string is not right,
> > we may want to have it internationalized and thus hit the same error.
> > What about returning a custom HTTP header specifying the reason?
> >
> > Something like that:
> >
> > X-rejection-reason: password-expired
> > OR
> > X-rejection-reason: password-invalid
> > OR
> > X-rejection-reason: account-locked
> >
> > Web UI could customize next actions based on this header instead of
> > parsing the error message.
> >
> > If you decide to rather stick with current solution and file my proposal
> > as an enhancement ticket (or discard it entirely), then ACK.
> >
> > Martin
> >
> 
> I think this is a better solution. I've updated my patch.
> 
> rob

ACK. Worked for me nice, header was there. Now, the ball is on WebUI
side.

I would just rename the header from "X-rejection-reason" to
"X-Rejection-Reason" (camel case used) in order to be consistent with
other headers.

Honza also suggested to add "IPA" prefix so that people know where this
headers comes from. So the final header name would be:
X-IPA-Rejection-Reason

Martin