Code

Opened 7 years ago

Closed 7 years ago

Last modified 3 years ago

#4189 closed (fixed)

Fix for COMMENTS_BANNED_USERS_GROUP exploding the ORM when using the templatetag.

Reported by: Brett Hoerner <bretthoerner@…> Owned by: adrian
Component: contrib.comments Version: 0.96
Severity: Keywords: comments, COMMENTS_BANNED_USERS_GROUP
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Using Django comments with COMMENTS_BANNED_USERS_GROUP drops you into the following branch:

if not self.free and settings.COMMENTS_BANNED_USERS_GROUP:
    kwargs['select'] = {'is_hidden': 'user_id IN (SELECT user_id FROM auth_user_groups WHERE group_id = %s)' % settings.COMMENTS_BANNED_USERS_GROUP}
comment_list = get_list_function(**kwargs).order_by(self.ordering + 'submit_date').select_related()

Which in turn shoves the kwarg 'select' into a filter function, thus making the ORM puke violently. A patch is attached:

comment_list = get_list_function(**kwargs).order_by(self.ordering + 'submit_date').select_related()
if not self.free and settings.COMMENTS_BANNED_USERS_GROUP:
    comment_list = comment_list.extra(select={'is_hidden': 'user_id IN (SELECT user_id FROM auth_user_groups WHERE group_id = %s)' % settings.COMMENTS_BANNED_USERS_GROUP})

Also, the Comment/FreeComment models' get_absolute_url() methods can cause a site to fail loudly if an object is erased and latest comments are still referred to. An example is a "latest comments" rail showing comments on different objects. If one of the objects is removed the following code will cause a rendering error, instead of failing silently. (get_content_object returns None and then get_absolute_url explodes)

def get_absolute_url(self):
    return self.get_content_object().get_absolute_url() + "#c" + str(self.id)

This is a problem because non-developers (template creators and/or admin users) can bring a site to a halt using comments (I believe Django prefers templates to fail silently instead of loudly when they can), I recommend a safe/harmless change such as this:

def get_absolute_url(self):
    try:
        return self.get_content_object().get_absolute_url() + "#c" + str(self.id)
    except AttributeError:
        return ""

Attachments (3)

models.py.diff (1.1 KB) - added by Brett Hoerner <bretthoerner@…> 7 years ago.
get_absolute_url() patch
comments.py.diff (1.1 KB) - added by Brett Hoerner <bretthoerner@…> 7 years ago.
COMMENTS_BANNED_USERS_GROUP / ORM patch
djt-4189-combined.diff (3.2 KB) - added by Jeremy Dunck <jdunck@…> 7 years ago.
Supercedes previous 2 patches.

Download all attachments as: .zip

Change History (8)

Changed 7 years ago by Brett Hoerner <bretthoerner@…>

get_absolute_url() patch

Changed 7 years ago by Brett Hoerner <bretthoerner@…>

COMMENTS_BANNED_USERS_GROUP / ORM patch

comment:1 Changed 7 years ago by Jeremy Dunck <jdunck@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

We found one more issue while using comments with 0.96 and settings.COMMENTS_BANNED_USERS_GROUP / settings.COMMENTS_SKETCHY_USERS_GROUP.

I'm attaching a combined patch.

To be clear, the bugs reported here aren't obvious to other people using the comments framework because the framework short-circuits around the problem areas if settings does not include a truth-y related *_USERS_GROUP setting.

There don't seem to be any tests on the comments app, or I'd add tests to cover this particular edge case.

Changed 7 years ago by Jeremy Dunck <jdunck@…>

Supercedes previous 2 patches.

comment:2 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:3 Changed 7 years ago by mtredinnick

On balance, I think the handling for get_absolute_url() is incorrect: the function is now (after patch applcation) returning the wrong answer in the 'except' case, because no URL is valid. A more correct solution is to not call get_absolute_url on non-existent objects and so the callers need to be examined. However, given that all of this code is slated for replacement prior to 1.0, it's not worth worrying about this at the moment. I'll apply the patch as a stop-gap until the comments rewrite.

comment:4 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [5848]) Fixed #4189 -- Fixed crashes in a couple of corner cases in the comments app. Not a perfect fix (see ticket), but it will do as a holdover until the new comments framework is in place.

comment:5 Changed 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.