530721 - Unauthenticated users can approve/deny configs

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect, P1)

Description

[Eric Henry]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/532.4 (KHTML, like Gecko) Chrome/4.0.237.0 Safari/532.4
Build Identifier: 

The url ^approve/(?P<id>\d*) will accept comments from unauthenticated users even though the page javascript hides the form.

Reproducible: Always

Steps to Reproduce:
1. POST form with info as if logged in

Actual Results:  
Any comments and approval/denial are committed

Expected Results:  
Nothing should change

Comment 1

[Eric Henry]

Attachment: test that should pass if authentication is checked

Comment 2

[Blake Winton (:bwinton) (:☕️)]

We really have to fix this before we go live.

Comment 3

[Eric Henry]

I'll take this one...

Comment 4

[Eric Henry]

Attachment: fix (11.30.09)

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 415195 [details] [diff] [review]
fix (11.30.09)

I like the idea behind this, but I can't get the tests to run successfully…

The errors are:
ERROR: test_post_deny (test_approve.ApproveTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bwinton/Programming/thunderbird/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/ispdb/tests/test_approve.py", line 73, in test_post_deny
    }, follow=True)
  File "/Users/bwinton/Programming/thunderbird/ispdb/ispdb/lib/python2.6/site-packages/django/test/client.py", line 313, in post
  File "/Users/bwinton/Programming/thunderbird/ispdb/ispdb/lib/python2.6/site-packages/django/core/handlers/base.py", line 92, in get_response
  File "/Users/bwinton/Programming/thunderbird/ispdb/ispdb/lib/python2.6/site-packages/django/contrib/auth/decorators.py", line 78, in __call__
  File "/Users/bwinton/Programming/thunderbird/ispdb/config/views.py", line 162, in approve
    config = Config.objects.filter(id=id)[0]
  File "/Users/bwinton/Programming/thunderbird/ispdb/ispdb/lib/python2.6/site-packages/django/db/models/query.py", line 159, in __getitem__
IndexError: list index out of range

Is there something I'm missing?

Oh, and you added some whitespace on the line above "response = client.post("/approve/1", {}, follow=True)"

Later,
Blake.

Comment 6

[Eric Henry]

Aha!

Alright, so this test (and the subsequent fix) is written as if the patch for bug 522251 were committed.

I'm guessing that I need to make the test and the fix for the current repository state, and include changes to it with bug 522251.

Does that make sense?

Eric

Comment 7

[Eric Henry]

Attachment: fix (12.10.09)

Assuming that bug 522251 has not been patched yet

NOTE, there is a new test file that also assumes bug 522251 has not been patched

APPLY THIS PATCH BEFORE PATCH TO 522251

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 417004 [details] [diff] [review]
fix (12.10.09)

>+++ b/tests/test_approval_auth.py
>@@ -0,0 +1,30 @@
>+class ApprovalAuthTest(TestCase):
>+    fixtures = ['xml_testdata']
>+
>+    def test_unauthenticated_user(self):
>+        comment_text = 'Seems fine'
>+
>+        client = Client()
>+        
>+        result = client.post("/approve/1",
>+        {
>+            "approved": True,
>+            "comment": "Other - valid",
>+            "comment_valid": comment_text
>+        })
>+        
>+        config = Config.objects.filter(id=1)[0]
>+        
>+        #should not have changed the config
>+        nose.tools.assert_equal(config.approved, False)
>+        
>+        #used after bug 522251 has been fixed
>+        #assert config.comments.count() == 0

I love it, but could you add another test that ensures that an authenticated user can approve the configs?

Oh, and get rid of the whitespace on the blank lines.  :)

Thanks,
Blake.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 417004 [details] [diff] [review]
fix (12.10.09)

D'oh, I forgot to mark it r-.  And just so you understand, I'm marking it r- not because I don't like the changes, but because I want to take a look at the new test before we send it off to the next reviewer.

Thanks,
Blake.

Comment 10

[Eric Henry]

Attachment: patch (12.17.09)

added test for authenticated user

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 418277 [details] [diff] [review]
patch (12.17.09)

>+++ b/config/views.py
>+++ b/tests/test_approval_auth.py
>@@ -0,0 +1,47 @@
>+from django.test import TestCase
>+from django.test.client import Client
>+from django.contrib.auth.models import User
>+from ispdb.config import views, models
>+from ispdb.config.models import Config
>+
>+import nose.tools
>+
>+class ApprovalAuthTest(TestCase):
>+    fixtures = ['xml_testdata', 'approval_user']
>+
>+    def test_unauthenticated_user(self):

There's a lot of whitespace on this line (and a few other lines) that should be removed, 

>+        result = client.post("/approve/1",
>+        {
>+            "approved": True,
>+        })

I think this (and the other things like it) should be 

        result = client.post("/approve/1", {
                               "approved": True,
                             })

Also, it would be nice to assert something about the result.  (Like it's returning some HTTP status code, or something.)

>+    def test_authenticated_user(self):
>+        #should not have changed the config
>+        nose.tools.assert_equal(config.approved, True)

Surely this test should have changed the config.  :)

r=me with those nits fixed.

Thanks,
Blake.

Comment 12

[Eric Henry]

Attachment: patch (1.19.10)

Should have taken care of all of your nits.

Sorry it took so long!

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 422479 [details] [diff] [review]
patch (1.19.10)

># HG changeset patch
># User ehenry5
># Date 1263957918 18000
>
>+++ b/config/views.py
>@@ -1,13 +1,14 @@
> from django.contrib.auth.decorators import login_required
>+from django.contrib.auth.decorators import permission_required

This change didn't apply cleanly for me, but I figured it out.


>+++ b/tests/test_approval_auth.py
>@@ -0,0 +1,57 @@
>+        #should not have changed the config
>+        nose.tools.assert_equal(config.approved, False)
>+        

Please remove the trailing spaces at the end of the previous line (and the two other lines that have them).

Aside from that, I like it.

(Well, there are some problems, but they seem big enough that we shouldn't tackle them in this patch.  Problems like the formatting of the fixture files, and converting them to use YAML instead of JSON, so that we can leave a comment in the fixture saying what the password is.  But like I said, I don't think those should be part of this patch.)

Later,
Blake.

Comment 14

[Eric Henry]

Attachment: patch (2.13.10)

Same as previous patch, with trailing spaces removed.

Comment 15

[Jeff Balogh (:jbalogh)]

Comment on attachment 426837 [details] [diff] [review]
patch (2.13.10)

r+ with the improvements listed below, which are all cosmetic.

git told me:

    warning: squelched 68 whitespace errors
    warning: 73 lines add whitespace errors.

>diff --git a/tests/test_approval_auth.py b/tests/test_approval_auth.py
>new file mode 100644
>--- /dev/null
>+++ b/tests/test_approval_auth.py
>@@ -0,0 +1,57 @@
>+from django.test import TestCase
>+from django.test.client import Client
>+from django.contrib.auth.models import User
>+from ispdb.config import views, models
>+from ispdb.config.models import Config
>+
>+import nose.tools
>+
>+class ApprovalAuthTest(TestCase):
>+    fixtures = ['xml_testdata', 'approval_user']
>+
>+    def test_unauthenticated_user(self):
>+        client = Client()

Django's TestCase setup creates a Client that you can access at self.client here.  Not a big deal

>+
>+        result = client.post("/approve/1", {
>+                                "approved": True,
>+                            }, follow=True)
>+
>+        config = Config.objects.filter(id=1)[0]

If you only expect one of these (which is probably the case if you're looking up on id) you can use `Config.objects.get(id=1)`.

>+
>+        #should not have changed the config

pep8 says comments should look like `# Should not have changed the config.`, with proper punctuation and capitalization, and a space after the hash.  It looks like you have other comments that need fixing below.

>+        nose.tools.assert_equal(config.approved, False)

It's up to you, but I like eq_.  It's an alias for assert_equal, but saves me a bunch of typing.

>+
>+        #make sure it redirects to openid login
>+        redirect=result.redirect_chain[0][0]
>+        goodRedirect="/openid/login?next=/approve/1"

Equal signs need spaces around them.  Unless you're defining a function's default arguments.

Django provides you with self.assertRedirects, which makes this check easier.

>+
>+        nose.tools.assert_equal(redirect.count(goodRedirect), 1)
>+
>+        #used after bug 522251 has been fixed
>+        #assert config.comments.count() == 0

I like to start these comments with TODO(jbalogh) so I can find them later.

Comment 16

[Eric Henry]

Attachment: patch (2010-06-10)

should have fixed all of the nits from previous review

Comment 17

[Jeff Balogh (:jbalogh)]

Comment on attachment 450283 [details] [diff] [review]
patch (2010-06-10)

>diff -r acae4af967d3 ispdb/config/views.py
>--- a/ispdb/config/views.py	Fri May 07 20:11:57 2010 +0000
>+++ b/ispdb/config/views.py	Wed Jun 09 22:55:00 2010 -0400
>@@ -3,10 +3,14 @@
> import StringIO
> import lxml.etree as ET
> 
>-from django.http import HttpResponse, HttpResponseRedirect, Http404
>+from django.http import HttpResponse
>+from django.http import HttpResponseRedirect
>+from django.http import Http404

Why are you pulling this into 3 lines?  My preferred import would be ``from django import http`` anyways.

Comment 18

[Eric Henry]

Attachment: patch (2010-06-11)

I didn't really care about moving those lines into 1, I just thought it was supposed to be 1 import per line or something like that...  This is one is back the way it started.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Checked in as http://viewvc.svn.mozilla.org/vc?view=revision&revision=68731

Thanks, Eric!