Roundup Tracker - Issues

Issue 2550755

classification
Title: exceptions.NotFound(msg) msg is not reported to user in cgi
Type: behavior Severity: normal
Components: Web interface Versions: devel
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: rouilj Nosy List: ber, ced, rouilj
Priority: Keywords: Effort-Low, patch

Created on 2012-05-14 15:51 by ber, last changed 2016-09-05 07:16 by ber.

Files
File name Uploaded Description Edit Remove
NotFound_message.diff rouilj, 2016-07-17 03:15
Messages
msg4554 Author: [hidden] (ber) Date: 2012-05-14 15:51
in roundup/cgi/actions.py
raise exceptions.NotFound(msg) should give the msg to the web browser.
It does not with 4624:21705126dafa (pre 1.4.20).

Should be fixed and then the following code needs to be replaced:
       # check if all columns exist on class
        # the exception must be raised before sending header
        props = klass.getprops()
        for cname in columns:
            if cname not in props:
                # TODO raise exceptions.NotFound(.....) does not give
message
                # so using SeriousError instead
                self.client.response_code = 404
                raise exceptions.SeriousError(
                    self._('Column "%(column)s" not found on
%(class)s')
                    % {'column': cgi.escape(cname), 'class':
request.classname})

in roundup/cgi/actions.py to use NotFound().
msg5864 Author: [hidden] (rouilj) Date: 2016-07-17 03:15
The reason the message doesn't show up is that NotFound is supposed to
generate a 404 response to the client. The template is changed to 404
in client.py and the _generic.404.html has no support for messages.

I have made two changes:

 1) The response_code is set to 400 before raising the NotFound
    error. 400 (Bad Request) indicates that there is a client side
    issue.  I.E. it fed a bogus column value to the server.

 2) client.py's NotFound handling is changed to look at the
    response_code and if it is not 400, the usual 404 response_path is
    taken. If the code is 400, the user is presented with the message
    formatted like it is formatted for SeriousError.

The documentation for the NotFound exception doesn't explain when it
should be used. Arguablly SeriousError is the right way to handle this
if NotFound is supposed to be a pure "404" error code.

Suprisingly asking for a non-existent class looks like it doesn't use
the 404 template. E.G. compare:

   http://issues.roundup-tracker.org/bug2550755 (not using template)
to

   http://issues.roundup-tracker.org/issue2550755776 (using the
     _generic.404.html template)

So Bern should this attached patch be committed?

-- rouilj
msg5891 Author: [hidden] (rouilj) Date: 2016-08-13 00:18
Ping Bern, any feedback. If none, I am going to apply it as is
returning the 400 error code.
msg5893 Author: [hidden] (ber) Date: 2016-08-15 07:24
Hi John,
thanks for the analysis and the patch.
Currently I lack the time to do a review.
Your analysis reads fine, so please apply if you are confident
that there is no bad side effect. You have probably improved the
situation. :)

Thanks and best Regards,
Bernhard
msg5900 Author: [hidden] (rouilj) Date: 2016-09-02 01:20
Hi Bern:

I needed to change two test cases in test/test_cgi.py as well:

   FormTestCase.testCSVExportBadColumnName
   FormTestCase.testCSVExportFailPermission

they both expected exceptions.SeriousError and I am now raising
exceptions.NotFound.

test_cgi.py::testCSVExportFailPermission was changed on issue2550712
(exportcsvaction errors poorly when given invalid columns). Ced worked
on the patch for this as well as you.

The diff for the test on commit 21705126dafa for issue2550712 is:

     def testCSVExportFailPermission(self):
         cl = self._make_client({'@columns': 'id,email,password'},
nodeid=None,
             userid='2')
@@ -937,7 +947,9 @@
         output = StringIO.StringIO()
         cl.request = MockNull()
         cl.request.wfile = output
-        self.assertRaises(exceptions.Unauthorised,
+        # used to be self.assertRaises(exceptions.Unauthorised,
+        # but not acting like the column name is not found
+        self.assertRaises(exceptions.SeriousError,
             actions.ExportCSVAction(cl).handle)

The name of the test makes me think that Unauthorised should be
raised always. If the user user2 can't access the user class, how
could they find out that the column doesn't exist?

I added ced to this ticket since he worked on the patch.
Maybe the patch for that issue needs to be tweaked?

Thoughts?

I have checked in the change from exceptions.SeriousError to
exceptions.NotFound to get the 2 tests to pass again.

I also renamed

  test/test_cgi.py::FormTestCase::testCSVExportFailPermission

to

  test/test_cgi.py::FormTestCase::testCSVExportFailPermissionBadColumn

and added

 test/test_cgi.py::FormTestCase::testCSVExportFailPermissionValidColumn

that verifies that exceptions.Unauthorised is raised.

I am leaving this ticket open pending a decision on what should be
raised with the testCSVExportFailPermissionBadColumn test.
msg5901 Author: [hidden] (ber) Date: 2016-09-05 07:16
Hi John,
from briefly reading the issue:
I believe your thinking is good:
If a user misses permissions, she should not able to find
out much about the data structure of the tracker.

Best,
Bernhard
History
Date User Action Args
2016-09-05 07:16:01bersetmessages: + msg5901
2016-09-02 01:20:53rouiljsetassignee: rouilj
messages: + msg5900
nosy: + ced
2016-08-15 07:24:27bersetmessages: + msg5893
2016-08-13 00:18:33rouiljsetmessages: + msg5891
2016-07-17 03:15:17rouiljsetstatus: new -> open
files: + NotFound_message.diff
nosy: + rouilj
messages: + msg5864
keywords: + patch, Effort-Low
type: behavior
2012-05-14 15:51:52bercreate