Mercurial > p > roundup > code
changeset 5121:894aa07be6cb
issue2550785: Using login from search (or logout) fails. when
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.
| author | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Sun, 03 Jul 2016 12:32:35 -0400 |
| parents | 722394a48d7b |
| children | 1c90f15a177f |
| files | CHANGES.txt doc/upgrading.txt roundup/anypy/urllib_.py roundup/cgi/actions.py share/roundup/templates/classic/html/page.html share/roundup/templates/devel/html/page.html share/roundup/templates/minimal/html/page.html share/roundup/templates/responsive/html/page.html test/test_actions.py |
| diffstat | 9 files changed, 207 insertions(+), 10 deletions(-) [+] |
line wrap: on
line diff
--- a/CHANGES.txt Sun Jul 03 12:23:36 2016 -0400 +++ b/CHANGES.txt Sun Jul 03 12:32:35 2016 -0400 @@ -239,6 +239,13 @@ 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
--- a/doc/upgrading.txt Sun Jul 03 12:23:36 2016 -0400 +++ b/doc/upgrading.txt Sun Jul 03 12:32:35 2016 -0400 @@ -107,6 +107,44 @@ # 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 ----------------------------------------------------
--- a/roundup/anypy/urllib_.py Sun Jul 03 12:23:36 2016 -0400 +++ b/roundup/anypy/urllib_.py Sun Jul 03 12:32:35 2016 -0400 @@ -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
--- a/roundup/cgi/actions.py Sun Jul 03 12:23:36 2016 -0400 +++ b/roundup/cgi/actions.py Sun Jul 03 12:32:35 2016 -0400 @@ -1029,12 +1029,63 @@ 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 @@ # 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
--- a/share/roundup/templates/classic/html/page.html Sun Jul 03 12:23:36 2016 -0400 +++ b/share/roundup/templates/classic/html/page.html Sun Jul 03 12:32:35 2016 -0400 @@ -133,7 +133,12 @@ <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')"
--- a/share/roundup/templates/devel/html/page.html Sun Jul 03 12:23:36 2016 -0400 +++ b/share/roundup/templates/devel/html/page.html Sun Jul 03 12:32:35 2016 -0400 @@ -153,7 +153,7 @@ <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 @@ <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>
--- a/share/roundup/templates/minimal/html/page.html Sun Jul 03 12:23:36 2016 -0400 +++ b/share/roundup/templates/minimal/html/page.html Sun Jul 03 12:32:35 2016 -0400 @@ -133,7 +133,12 @@ <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')"
--- a/share/roundup/templates/responsive/html/page.html Sun Jul 03 12:23:36 2016 -0400 +++ b/share/roundup/templates/responsive/html/page.html Sun Jul 03 12:32:35 2016 -0400 @@ -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>
--- a/test/test_actions.py Sun Jul 03 12:23:36 2016 -0400 +++ b/test/test_actions.py Sun Jul 03 12:32:35 2016 -0400 @@ -237,6 +237,23 @@ 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 @@ 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 @@ 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)