issue2550785: Using login from search (or logout) fails. when

rouilj · rouilj · commit 7959ecb26888 · 2016-07-03T12:32:35.000-04:00
logging in from a search page or after a logout it fails with an
error.

The fix also keeps the user on the same page they started
from (e.g. search results) before the login. There are two
parts to this:
1) changes to the templates to properly define the __came_from form element.
2) code changes to the LoginAction code in roundup/cgi/actions.py.

New test code added. Needed some additional functions from urllib so
urllib_.py got a change.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -239,6 +239,13 @@ Fixed:
   Diagnosis and fix with patch by R David Murray. Support for
   restoring retired but active queries, html layout changes and doc
   by John Rouillard.
+- issue2550785: Using login from search (or logout) fails.  when
+  logging in from a search page or after a logout it fails with an
+  error. These failures have been fixed. The fix also keeps the user
+  on the same page they started from before the login. There are two
+  parts to this: 1) changes to the templates to properly define the
+  __came_from form element. See ``upgrading.txt``. 2) code changes
+  to the LoginAction code in roundup/cgi/actions.py.  (John Rouillard)
 
 2016-01-11: 1.5.1
 
diff --git a/doc/upgrading.txt b/doc/upgrading.txt
@@ -107,6 +107,44 @@ the email entry in the tracker section::
   # Default: 
   replyto_address = 
 
+Login from a search or after logout works better
+------------------------------------------------
+
+The login form has been improved to work with some back end code
+changes. Now when a user logs in they stay on the same page where they
+started the login. To make this work, you must change the tal that is
+used to set the ``__came_from`` form variable.
+
+Replace the existing code in the tracker's html/page.html page that
+looks similar to (look for name="__came_from")::
+
+  <input type="hidden" name="__came_from" tal:attributes="value string:${request/base}${request/env/PATH_INFO}">
+
+with the following::
+
+  <input type="hidden" name="__came_from"
+    tal:condition="exists:request/env/QUERY_STRING"
+    tal:attributes="value string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}">
+  <input type="hidden" name="__came_from"
+    tal:condition="not:exists:request/env/QUERY_STRING"
+    tal:attributes="value string:${request/base}${request/env/PATH_INFO}">
+
+Now search backwards for the nearest form statement before the code
+that sets __came_from. If it looks like::
+
+     <form method="post" action="#">
+
+replace it with::
+
+     <form method="post" tal:attributes="action request/base">
+
+or with::
+
+     <form method="post" tal:attributes="action string:${request/env/PATH_INFO}">
+
+the important part is that the action field **must not** include any query
+parameters ('#' includes query params).
+
 html/_generic.404.html in trackers use page template
 ----------------------------------------------------
 
diff --git a/roundup/anypy/urllib_.py b/roundup/anypy/urllib_.py
@@ -1,8 +1,8 @@
 
 try:
     # Python 3+
-    from urllib.parse import quote, urlparse
+    from urllib.parse import quote, urlencode, urlparse, parse_qs, urlunparse
 except:
     # Python 2.5-2.7
-    from urllib import quote
-    from urlparse import urlparse
+    from urllib import quote, urlencode
+    from urlparse import urlparse, parse_qs, urlunparse
diff --git a/roundup/cgi/actions.py b/roundup/cgi/actions.py
@@ -1029,12 +1029,63 @@ def handle(self):
         else:
             password = ''
 
+        if '__came_from' in self.form:
+            # On valid or invalid login, redirect the user back to the page
+            # the started on. Searches, issue and other pages
+            # are all preserved in __came_from. Clean out any old feedback
+            # @error_message, @ok_message from the __came_from url.
+            #
+            # 1. Split the url into components.
+            # 2. Split the query string into parts.
+            # 3. Delete @error_message and @ok_message if present.
+            # 4. Define a new redirect_url missing the @...message entries.
+            #    This will be redefined if there is a login error to include
+            #      a new error message
+            redirect_url_tuple = urllib_.urlparse(self.form['__came_from'].value)
+            # now I have a tuple form for the __came_from url
+            try:
+                query=urllib_.parse_qs(redirect_url_tuple.query)
+                if "@error_message" in query:
+                    del query["@error_message"]
+                if "@ok_message" in query:
+                    del query["@ok_message"]
+                if "@action" in query:
+                    # also remove the logout action from the redirect
+                    # there is only ever one @action value.
+                    if query['@action'] == ["logout"]:
+                        del query["@action"]
+            except AttributeError:
+                # no query param so nothing to remove. Just define.
+                query = {}
+                pass
+
+            redirect_url = urllib_.urlunparse( (redirect_url_tuple.scheme,
+                                                redirect_url_tuple.netloc,
+                                                redirect_url_tuple.path,
+                                                redirect_url_tuple.params,
+                                                urllib_.urlencode(query, doseq=True),
+                                                redirect_url_tuple.fragment)
+                                           )
+
         try:
             self.verifyLogin(self.client.user, password)
         except exceptions.LoginError, err:
             self.client.make_user_anonymous()
             for arg in err.args:
                 self.client.add_error_message(arg)
+
+            if '__came_from' in self.form:
+                # set a new error
+                query['@error_message'] = err.args
+                redirect_url = urllib_.urlunparse( (redirect_url_tuple.scheme,
+                                                    redirect_url_tuple.netloc,
+                                                    redirect_url_tuple.path,
+                                                    redirect_url_tuple.params,
+                                                    urllib_.urlencode(query, doseq=True),
+                                                    redirect_url_tuple.fragment )
+                                               )
+                raise exceptions.Redirect(redirect_url)
+            # if no __came_from, send back to base url with error
             return
 
         # now we're OK, re-open the database for real, using the user
@@ -1047,7 +1098,7 @@ def handle(self):
 
         # If we came from someplace, go back there
         if '__came_from' in self.form:
-            raise exceptions.Redirect(self.form['__came_from'].value)
+            raise exceptions.Redirect(redirect_url)
 
     def verifyLogin(self, username, password):
         # make sure the user exists
diff --git a/share/roundup/templates/classic/html/page.html b/share/roundup/templates/classic/html/page.html
@@ -133,7 +133,12 @@ <h2><span metal:define-slot="body_title">body title</span></h2>
     <input type="checkbox" name="remember" id="remember">
     <label for="remember" i18n:translate="">Remember me?</label><br>
     <input type="submit" value="Login" i18n:attributes="value"><br>
-    <input type="hidden" name="__came_from" tal:attributes="value string:${request/base}${request/env/PATH_INFO}">
+    <input type="hidden" name="__came_from"
+	   tal:condition="exists:request/env/QUERY_STRING"
+	   tal:attributes="value string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}">
+    <input type="hidden" name="__came_from"
+	   tal:condition="not:exists:request/env/QUERY_STRING"
+	   tal:attributes="value string:${request/base}${request/env/PATH_INFO}">
     <span tal:replace="structure request/indexargs_form" />
     <a href="user?@template=register"
        tal:condition="python:request.user.hasPermission('Register', 'user')"
diff --git a/share/roundup/templates/devel/html/page.html b/share/roundup/templates/devel/html/page.html
@@ -153,7 +153,7 @@ <h1><a href="/">Roundup Demo Tracker</a></h1>
     <ul class="user">
      <li tal:condition="python:request.user.username=='anonymous'" class="submenu">
       <b i18n:translate="">User</b>
-      <form method="post" action="#">
+      <form method="post" tal:attributes="action request/base">
        <ul>
         <li>
          <tal:span i18n:translate="">Login</tal:span><br/>
@@ -163,7 +163,12 @@ <h1><a href="/">Roundup Demo Tracker</a></h1>
          <input type="checkbox" name="remember" id="remember"/>
          <label for="remember" i18n:translate="">Remember me?</label><br/>
          <input class="form-small" type="submit" value="Login" i18n:attributes="value"/><br/>
-         <input type="hidden" name="__came_from" tal:attributes="value string:${request/env/PATH_INFO}"/>
+	 <input type="hidden" name="__came_from"
+		tal:condition="exists:request/env/QUERY_STRING"
+		tal:attributes="value string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}"/>
+         <input type="hidden" name="__came_from"
+    		tal:condition="not:exists:request/env/QUERY_STRING"
+		tal:attributes="value string:${request/base}${request/env/PATH_INFO}"/>
          <span tal:replace="structure request/indexargs_form" />
         </li>
         <li>
diff --git a/share/roundup/templates/minimal/html/page.html b/share/roundup/templates/minimal/html/page.html
@@ -133,7 +133,12 @@ <h2><span metal:define-slot="body_title">body title</span></h2>
     <input type="checkbox" name="remember" id="remember">
     <label for="remember" i18n:translate="">Remember me?</label><br>
     <input type="submit" value="Login" i18n:attributes="value"><br>
-    <input type="hidden" name="__came_from" tal:attributes="value string:${request/base}${request/env/PATH_INFO}">
+    <input type="hidden" name="__came_from"
+	   tal:condition="exists:request/env/QUERY_STRING"
+	   tal:attributes="value string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}">
+    <input type="hidden" name="__came_from"
+	   tal:condition="not:exists:request/env/QUERY_STRING"
+	   tal:attributes="value string:${request/base}${request/env/PATH_INFO}">
     <span tal:replace="structure request/indexargs_form" />
     <a href="user?@template=register"
        tal:condition="python:request.user.hasPermission('Register', 'user')"
diff --git a/share/roundup/templates/responsive/html/page.html b/share/roundup/templates/responsive/html/page.html
@@ -170,7 +170,7 @@
         <div tal:condition="python:request.user.username=='anonymous'">
           <ul class='nav nav-list'>
             <li>
-              <form method="post" action="#">
+              <form method="post" tal:attributes="action request/base">
                 <fieldset>
                   <legend><i class='icon-user'></i>Login form</legend>
                   <input name="__login_name" type='text' placeholder='Username' i18n:attributes="placeholder">
@@ -180,7 +180,12 @@
                     <input type="checkbox" name="remember" id="remember">Remember me?
                   </label>
                   <input type="submit" value="Login" i18n:attributes="value" class='btn'>
-                  <input type="hidden" name="__came_from" tal:attributes="value string:${request/env/PATH_INFO}"/>
+		  <input type="hidden" name="__came_from"
+			 tal:condition="exists:request/env/QUERY_STRING"
+			 tal:attributes="value string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}">
+		  <input type="hidden" name="__came_from"
+			 tal:condition="not:exists:request/env/QUERY_STRING"
+			 tal:attributes="value string:${request/base}${request/env/PATH_INFO}">
                   <span tal:replace="structure request/indexargs_form" />
                 </fieldset>
               </form>
diff --git a/test/test_actions.py b/test/test_actions.py
@@ -237,6 +237,23 @@ def setUp(self):
         self.client.opendb = lambda a: self.fail(
             "Logged in, but we shouldn't be.")
 
+    def assertRaisesMessage(self, exception, callable, message, *args,
+                            **kwargs):
+        """An extension of assertRaises, which also checks the exception
+        message. We need this because we rely on exception messages when
+        redirecting.
+        """
+        try:
+            callable(*args, **kwargs)
+        except exception, msg:
+            self.assertEqual(str(msg), message)
+        else:
+            if hasattr(exception, '__name__'):
+                excName = exception.__name__
+            else:
+                excName = str(exception)
+            raise self.failureException, excName
+
     def assertLoginLeavesMessages(self, messages, username=None, password=None):
         if username is not None:
             self.form.value.append(MiniFieldStorage('__login_name', username))
@@ -247,6 +264,18 @@ def assertLoginLeavesMessages(self, messages, username=None, password=None):
         LoginAction(self.client).handle()
         self.assertEqual(self.client._error_message, messages)
 
+    def assertLoginRaisesRedirect(self, message, username=None, password=None, came_from=None):
+        if username is not None:
+            self.form.value.append(MiniFieldStorage('__login_name', username))
+        if password is not None:
+            self.form.value.append(
+                MiniFieldStorage('__login_password', password))
+        if came_from is not None:
+            self.form.value.append(
+                MiniFieldStorage('__came_from', came_from))
+
+        self.assertRaisesMessage(Redirect, LoginAction(self.client).handle, message)
+
     def testNoUsername(self):
         self.assertLoginLeavesMessages(['Username required'])
 
@@ -272,6 +301,58 @@ def opendb(username):
 
         self.assertLoginLeavesMessages([], 'foo', 'right')
 
+    def testCorrectLoginRedirect(self):
+        self.client.db.security.hasPermission = lambda *args, **kwargs: True
+        def opendb(username):
+            self.assertEqual(username, 'foo')
+        self.client.opendb = opendb
+
+        # basic test with query
+        self.assertLoginRaisesRedirect("http://whoami.com/path/issue?%40action=search",
+                                 'foo', 'right', "http://whoami.com/path/issue?@action=search")
+
+        # test that old messages are removed
+        self.form.value[:] = []         # clear out last test's setup values
+        self.assertLoginRaisesRedirect("http://whoami.com/path/issue?%40action=search",
+                                 'foo', 'right', "http://whoami.com/path/issue?@action=search&@ok_messagehurrah+we+win&@error_message=blam")
+
+        # test when there is no query
+        self.form.value[:] = []         # clear out last test's setup values
+        self.assertLoginRaisesRedirect("http://whoami.com/path/issue255",
+                                 'foo', 'right', "http://whoami.com/path/issue255")
+
+        # test if we are logged out; should kill the @action=logout
+        self.form.value[:] = []         # clear out last test's setup values
+        self.assertLoginRaisesRedirect("http://localhost:9017/demo/issue39?%40startwith=0&%40pagesize=50",
+                                 'foo', 'right', "http://localhost:9017/demo/issue39?@action=logout&@pagesize=50&@startwith=0")
+
+    def testInvalidLoginRedirect(self):
+        self.client.db.security.hasPermission = lambda *args, **kwargs: True
+
+        def opendb(username):
+            self.assertEqual(username, 'foo')
+        self.client.opendb = opendb
+
+        # basic test with query
+        self.assertLoginRaisesRedirect("http://whoami.com/path/issue?%40error_message=Invalid+login&%40action=search",
+                                 'foo', 'wrong', "http://whoami.com/path/issue?@action=search")
+
+        # test that old messages are removed
+        self.form.value[:] = []         # clear out last test's setup values
+        self.assertLoginRaisesRedirect("http://whoami.com/path/issue?%40error_message=Invalid+login&%40action=search",
+                                 'foo', 'wrong', "http://whoami.com/path/issue?@action=search&@ok_messagehurrah+we+win&@error_message=blam")
+
+        # test when there is no __came_from specified
+        self.form.value[:] = []         # clear out last test's setup values
+        # I am not sure why this produces three copies of the same error.
+        # only one copy of the error is displayed to the user in the web interface.
+        self.assertLoginLeavesMessages(['Invalid login', 'Invalid login', 'Invalid login'], 'foo', 'wrong')
+
+        # test when there is no query
+        self.form.value[:] = []         # clear out last test's setup values
+        self.assertLoginRaisesRedirect("http://whoami.com/path/issue255?%40error_message=Invalid+login",
+                                 'foo', 'wrong', "http://whoami.com/path/issue255")
+
 class EditItemActionTestCase(ActionTestCase):
     def setUp(self):
         ActionTestCase.setUp(self)
