1320: Auto-added names cause exceptions.

josh.h*******@gmai***** (Google Code) (Is this you? Claim this profile.)
Sept. 27, 2009 
What version are you running?
1.0

What's the URL of the page containing the problem?
https://reviewboard.example.com/r/1234/
(That is, any review page with the details described below.)

What steps will reproduce the problem?
1. Create a new review.
2. Under "People:", enter something such as "user1; user2".
3. Refresh the page.  Observe the exception.

What is the expected output? What do you see instead?
This seems like a breakdown in input verification.  A user unfamiliar with
ReviewBoard might try to use a semicolon, rather than a comma, to separate
users.  ReviewBoard helpfully adds the string as a user but other parts of
the system seem unable to handle the format.  In my opinion it's reasonable
to disallow semicolons and most other punctuation when verifying usernames.
 Or, if arbitrary characters are allowed, the rest of the system should be
able to handle them.

What operating system are you using? What browser?
Red Hat Enterprise Linux 5.3 and Firefox 3.5.3, but it shouldn't matter.

Please provide any additional information below.
chipx86
#1 chipx86
  • +Confirmed
  • +Milestone-Release1.0.x
    +Component-Reviews
chipx86
#2 chipx86 
I can't get this to generate an exception. Can you reproduce this with 1.0.3?
  • -Confirmed
    +NeedInfo
#3 josh.h*******@gmai***** (Google Code) (Is this you? Claim this profile.) 
We're obviously running a few versions behind and I'm not sure when we'll have a
chance to upgrade; I'll try to get to it soon but I can make no promises.  If you
can't duplicate it, it may be easier to just close it and I'll reopen it with more
details if we run into the problem later.

Thanks!
david
#4 david 
Thanks!
  • -NeedInfo
    +UnableToReproduce
#5 eva***@gmai***** (Google Code) (Is this you? Claim this profile.) 
I can reproduce this on 1.0.3.1 by typing "foo bar" in the People field, and then
publishing the review.  The attached patch fixes it for me.
  • +
    no-auto-create-user.patch 
    diff --git a/reviewboard/webapi/json.py b/reviewboard/webapi/json.py
    index ffd5e70..7448c86 100644
    --- a/reviewboard/webapi/json.py
    +++ b/reviewboard/webapi/json.py
    @@ -737,20 +737,6 @@ def review_request_draft_save(request, review_request_id):
         return WebAPIResponse(request)
     
     
    -def find_user(username):
    -    try:
    -        return User.objects.get(username=username)
    -    except User.DoesNotExist:
    -        for backend in auth.get_backends():
    -            try:
    -                user = backend.get_or_create_user(username)
    -            except:
    -                pass
    -            if user:
    -                return user
    -    return None
    -
    -
     def _prepare_draft(request, review_request):
         if not review_request.is_mutable_by(request.user):
             raise PermissionDenied
    @@ -776,7 +762,7 @@ def _set_draft_field_data(draft, field_name, data):
                         obj = Group.objects.get(Q(name__iexact=value) |
                                                 Q(display_name__iexact=value))
chipx86
#6 chipx86 
That patch breaks functionality that we require. It's crucial that we grab a new user
from the auth backend if it doesn't already exist.

This *should* be fixed in the 1.1 nightlies, so please test that release (without
your patch).
#7 eva***@gmai***** (Google Code) (Is this you? Claim this profile.) 
You're absolutely right.  The problem isn't RB but our auth backend.  It assumed that
any username passed to get_or_create_user would be valid.  Josh and I work on the
same RB instance, so I think this can be closed.