GroupRequiredMixin bug with sequences in group_required #106

merged 2 commits into from Mar 1, 2014


None yet

3 participants



I've fixed a bug in the GroupRequiredMixin by which the check_membership method failed to validate a user if group_required was a sequence (list or tuple) of groups. The implementation validates ANY user whose group(s) appear in group_required, as specified in #35.

This bug went undetected because of errors in the test suite. I've also fixed and explained them in the commits.

If you feel like you need anything else to accept the pull request I am open to refactorings.

octavifs added some commits Feb 19, 2014
@octavifs octavifs Fixed GroupRequiredMixin.check_membership
Previously, if get_group_required would return a string, the checker would work fine (see if the group string was inside user.groups). If get_group_required returns a sequence, you would be comparing whether the entire sequence (of possible groups) appears as an entry in user.groups.

Now, check_membership checks whether any of the groups in self.group_required appears in user.groups (as intended, see #35).
@octavifs octavifs Fixed the tests (that prevented detection of the check_membership).
Previously, the test modified a group_required property from a GroupRequiredView **instance**, but not from the **class**. When the client is launched to perform a request it instantiates a **new** instance, with the original class variable 'test_groups' and not the intended sequence.
Now the test modified the class variable, so the client can create an instance with the correct attribute. (value is restored before finishing the test).

Similar bug with the instance variable vs class variable, which went undetected because of another bug when modifying the group name. When calling user.groups.all()[0] an object is returned (and then modified), but since its reference is not saved, the changes are lost (rerunning user.groups.all()[0] returns the instance saved in the DB, and not the modified one). That bug prevented the user group from being modified, so the test deceptively passed.
@chrisjones-brack3t chrisjones-brack3t added this to the 1.4 milestone Feb 27, 2014
@chrisjones-brack3t chrisjones-brack3t referenced this pull request Mar 1, 2014

Pr/106 #111

@kennethlove kennethlove merged commit dbe1888 into brack3t:master Mar 1, 2014

We fixed a small Python issue in the code and merged this in. Thanks for your submission!

octavifs commented Mar 1, 2014


The set intersection is certainly a nicer way to check the group match :)

@octavifs octavifs deleted the unknown repository branch Mar 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment