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
>From 91a8c950c706a445df0dc9aac96810b75dbb2fa4 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Fri, 13 Apr 2012 15:19:32 -0400
Subject: [PATCH] Return consistent expiration message for forms-based login

We need to inform users when a forms-based login fails due to the
password needing to be reset. Currently there is no way to distinguish
a reset case vs an incorrect password.

This will bind the user using a simple LDAP bind over ldapi (by default)
and if that is successful, check the expiration date against the current
time.

The UI portion of this that uses this message will come later.

https://fedorahosted.org/freeipa/ticket/2608
---
 ipaserver/rpcserver.py                 |   41 ++++++++++++++++++++++++++++++-
 tests/test_ipaserver/test_rpcserver.py |    5 ++-
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
index 2fbd79f208320664ec3acb6c8c6004cff683650d..917ddd6b29f2d7dbba5a9afe7df7800c94c91282 100644
--- a/ipaserver/rpcserver.py
+++ b/ipaserver/rpcserver.py
@@ -32,6 +32,8 @@ from ipalib.errors import PublicError, InternalError, CommandError, JSONError, C
 from ipalib.request import context, Connection, destroy_context
 from ipalib.rpc import xml_dumps, xml_loads
 from ipalib.util import make_repr, parse_time_duration
+from ipalib.dn import DN
+from ipaserver.plugins.ldap2 import ldap2
 from ipapython.compat import json
 from ipalib.session import session_mgr, AuthManager, get_ipa_ccache_name, load_ccache_data, bind_ipa_ccache, release_ipa_ccache, fmt_time, default_max_session_duration
 from ipalib.backend import Backend
@@ -45,6 +47,7 @@ import string
 import datetime
 from decimal import Decimal
 import urlparse
+import time
 
 HTTP_STATUS_SUCCESS = '200 Success'
 HTTP_STATUS_SERVER_ERROR = '500 Internal Server Error'
@@ -136,12 +139,14 @@ class HTTP_Status(plugable.Plugin):
         output = _internal_error_template % dict(message=escape(message))
         return [output]
 
-    def unauthorized(self, environ, start_response, message):
+    def unauthorized(self, environ, start_response, message, reason):
         """
         Return a 401 Unauthorized error.
         """
         status = '401 Unauthorized'
         response_headers = [('Content-Type', 'text/html; charset=utf-8')]
+        if reason:
+            response_headers.append(('X-rejection-reason', reason))
 
         self.info('%s: %s', status, message)
 
@@ -935,10 +940,42 @@ class login_password(Backend, KerberosSession, HTTP_Status):
 
         # Get the ccache we'll use and attempt to get credentials in it with user,password
         ipa_ccache_name = get_ipa_ccache_name()
+        reason = 'invalid-password'
         try:
             self.kinit(user, self.api.env.realm, password, ipa_ccache_name)
         except InvalidSessionPassword, e:
-            return self.unauthorized(environ, start_response, str(e))
+            # Ok, now why is this bad. Is the password simply bad or is the
+            # password expired?
+            try:
+                dn = str(DN(('uid', user),
+                            self.api.env.container_user,
+                            self.api.env.basedn))
+                conn = ldap2(shared_instance=False,
+                             ldap_uri=self.api.env.ldap_uri)
+                conn.connect(bind_dn=dn, bind_pw=password)
+
+                # password is ok, must be expired, lets double-check
+                (userdn, entry_attrs) = conn.get_entry(dn,
+                    ['krbpasswordexpiration'])
+                if 'krbpasswordexpiration' in entry_attrs:
+                    expiration = entry_attrs['krbpasswordexpiration'][0]
+                    try:
+                        exp = time.strptime(expiration, '%Y%m%d%H%M%SZ')
+                        if exp <= time.gmtime():
+                            reason = 'password-expired'
+                    except ValueError, v:
+                        self.error('Unable to convert %s to a time string'
+                            % expiration)
+
+            except Exception:
+                # It doesn't really matter how we got here but the user's
+                # password is not accepted or the user is unknown.
+                pass
+            finally:
+                if conn.isconnected():
+                    conn.destroy_connection()
+
+            return self.unauthorized(environ, start_response, str(e), reason)
 
         return self.finalize_kerberos_acquisition('login_password', ipa_ccache_name, environ, start_response)
 
diff --git a/tests/test_ipaserver/test_rpcserver.py b/tests/test_ipaserver/test_rpcserver.py
index 96d4614a1325b3b21a6f58a59c472248c33b24bc..39374f839e845251f8bdbc97e3639cb63fb3a4b7 100644
--- a/tests/test_ipaserver/test_rpcserver.py
+++ b/tests/test_ipaserver/test_rpcserver.py
@@ -102,11 +102,12 @@ def test_unauthorized_error():
     s = StartResponse()
 
     assert_equal(
-        f.unauthorized(None, s, 'unauthorized'),
+        f.unauthorized(None, s, 'unauthorized', 'password-expired'),
         [t % dict(message='unauthorized')]
     )
     assert s.status == '401 Unauthorized'
-    assert s.headers == [('Content-Type', 'text/html; charset=utf-8')]
+    assert s.headers == [('Content-Type', 'text/html; charset=utf-8'),
+                         ('X-rejection-reason', 'password-expired')]
 
 
 def test_params_2_args_options():
-- 
1.7.6

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to