Closed Bug 530721 Opened 12 years ago Closed 12 years ago

Unauthenticated users can approve/deny configs

Categories

(Webtools :: ISPDB Server, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ehenry5, Assigned: ehenry5)

References

Details

Attachments

(1 file, 7 obsolete files)

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
We really have to fix this before we go live.
Blocks: 526559
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
I'll take this one...
Attached patch fix (11.30.09) (obsolete) — Splinter Review
Attachment #415195 - Flags: review?(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.
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
Assignee: nobody → ehenry5
Attached patch fix (12.10.09) (obsolete) — Splinter Review
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
Attachment #414182 - Attachment is obsolete: true
Attachment #415195 - Attachment is obsolete: true
Attachment #415195 - Flags: review?(bwinton)
Attachment #417004 - Flags: review?(bwinton)
Attachment #417004 - Attachment is patch: true
Attachment #417004 - Attachment mime type: application/octet-stream → text/plain
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 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.
Attachment #417004 - Flags: review?(bwinton) → review-
Attached patch patch (12.17.09) (obsolete) — Splinter Review
added test for authenticated user
Attachment #417004 - Attachment is obsolete: true
Attachment #418277 - Flags: review?(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.
Attachment #418277 - Flags: review?(bwinton) → review+
Attachment #418277 - Flags: review?(gozer)
Attachment #418277 - Flags: review?(gozer)
Attached patch patch (1.19.10) (obsolete) — Splinter Review
Should have taken care of all of your nits.

Sorry it took so long!
Attachment #422479 - Flags: review?(bwinton)
Attachment #418277 - Attachment is obsolete: true
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.
Attachment #422479 - Flags: review?(bwinton) → review+
Attached patch patch (2.13.10) (obsolete) — Splinter Review
Same as previous patch, with trailing spaces removed.
Attachment #422479 - Attachment is obsolete: true
Attachment #426837 - Flags: review?(jbalogh)
Attachment #426837 - Flags: review?(jbalogh) → review+
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.
Attached patch patch (2010-06-10) (obsolete) — Splinter Review
should have fixed all of the nits from previous review
Attachment #426837 - Attachment is obsolete: true
Attachment #450283 - Flags: review?(jbalogh)
Attachment #450283 - Flags: review?(jbalogh) → review+
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.
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.
Attachment #450283 - Attachment is obsolete: true
Attachment #450550 - Flags: review?(jbalogh)
Attachment #450550 - Flags: review?(jbalogh)
Keywords: checkin-needed
Checked in as http://viewvc.svn.mozilla.org/vc?view=revision&revision=68731

Thanks, Eric!
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Component: ispdb → ISPDB Server
Product: Mozilla Messaging → Webtools
You need to log in before you can comment on or make changes to this bug.