View permissions for != 'Current Page' may not work as expected. #1113

Closed
kezabelle opened this Issue Dec 9, 2011 · 16 comments

3 participants

@kezabelle

So far, I have tested this with:

  • 2.2 release
  • 2.2.x from develop
  • mptt 0.4.2
  • mptt 0.5.1

I have produced a test case in this branch in commits #2b12de480b56095077ff45770aa9e49f30c79345 and #fd5c9d99ac8664e27bc13b01fa4be408d0d8262f though it now strikes me that the test will probably fail for reasons unrelated (the test case asserts page ancestry, and there are other pages used by the test suite that may affect that).

I have a standalone demonstration of the same code without being a unit test, if you can sacrifice the time to create a database. Note that the standalone would require you modify the template and language variables to match your own.

The premise is, given the following page structure:

  • page 1
    • page 2
      • page 3
        • page 4
    • page 5
      • page 6

If I assign any view restrictions to page 6, then only that and it's children/descendents should get those view restrictions. Instead, page 4, the child of a sibling (page 3) also gets those view restrictions. This seems borne out for any of the view permissions other than current page (ACCESS_PAGE)

The crux of the problem is PagePermission.objects.for_page() logic seems to allow for other elements in the same root tree_id to affect the display of otherwise unrelated nodes.

@ojii

How, if at all, is this related to #1073?

@kezabelle

It is related, insofar as #1106 doesn't modify has_view_permission in any way that will fix it. It also changes get_any_page_view_permissions to use the same [potentially problematic, if this ticket is valid] code. This would mean, I think, that the changelist view would display the same view restrictions decision as the change form, but wouldn't fix the root issue.

@kezabelle

The root issue, to clarify, is the following code:

q = Q(page__tree_id=page.tree_id) & (
            Q(page=page) 
            | (Q(page__level__lt=page.level)  & (Q(grant_on=ACCESS_DESCENDANTS) | Q(grant_on=ACCESS_PAGE_AND_DESCENDANTS)))
            | (Q(page__level=page.level - 1) & (Q(grant_on=ACCESS_CHILDREN) | Q(grant_on=ACCESS_PAGE_AND_CHILDREN)))  
        ) 

I think. Which looks one level up for any page who has granted children access, even if it's not a direct ancestor, and does the same for descdendents for any previous level. Making sure the grant_on is limited to a direct ancestor would, I think, fix it.

If I'm reading it correctly, the logic is:

  • find pages in the same Tree
  • and either:
    • this page
    • pages of any lower level (higher in the tree) who have access to descdenents, or this page and descdendents.
    • pages one level lower (one parent) who have access to children, or this page and children.
@kezabelle

I'm documenting this here because I do not have a test environment to ensure everything works correctly, but the following seems to fix the specific case I've demonstrated:

q_tree = Q(page__tree_id=page.tree_id)
q_page = Q(page=page)
left_right = {
    'page__%s__lte' % page._meta.left_attr: getattr(page, page._meta.left_attr),
    'page__%s__gte' % page._meta.right_attr: getattr(page, page._meta.right_attr),
}
q_parents = Q(**left_right)
q_desc = (Q(page__level__lt=page.level) & (Q(grant_on=ACCESS_DESCENDANTS) | Q(grant_on=ACCESS_PAGE_AND_DESCENDANTS)))
q_kids = (Q(page__level=page.level - 1) & (Q(grant_on=ACCESS_CHILDREN) | Q(grant_on=ACCESS_PAGE_AND_CHILDREN)))
q = q_tree & q_parents & (q_page | q_desc | q_kids)
return self.filter(q).order_by('page__level')

Specifically, q_parents uses the same logic as mptt's get_ancestors to find parent nodes. The code may well count as shockingly bad, but I don't have the facilities or time to test/refine further at this moment.

@kezabelle

To continue documenting my quest to figure this out, with minor modifications, I can run the full test suite without any failures. Specifically, in the test case, before asserting the ancestry is what we expect, I needed to add:

page4 = Page.objects.get(pk=page4.pk) 
page6 = Page.objects.get(pk=page6.pk)

Because otherwise the ancestors are just plain wrong. I'm not committing the changes to the test case until someone can confirm that I should need to do that, because it seems like mptt is doing half-magic.

Additionally, to get the test suite to pass, the above suggested fix needs to be modified to:

left_right = {
    'page__%s__lte' % page._meta.left_attr: getattr(page, page._meta.left_attr) or 0,
    'page__%s__gte' % page._meta.right_attr: getattr(page, page._meta.right_attr) or 0,
}

I'm not sure if the addition of or 0 should be there in any final fix, because it's [probably] only required by the test suite, which currently creates what ought to be considered invalid pages (see any pages in the ViewPermissionTests) because they don't use the cms.api, and don't set left/right values for mptt.

Finally, because I'm an idiot, the test case needs a minor fix because it tests for broken functionality, instead of failing. So:

self.assertEqual([u'page 6', u'page 6'], [x.page.get_title() for x in PagePermission.objects.for_page(page4)])

becomes

self.assertEqual([], [x.page.get_title() for x in PagePermission.objects.for_page(page4)])

Thereafter, all tests pass, including the one this ticket covers.

@FrankBie2

Hi kezabelle,
here you find a big testcase around different page permissions grant on types, combined with PagePermissions.
https://github.com/FrankBie2/django-cms/blob/develop/cms/tests/menu_page_viewperm.py

Hope this gets more light into the darkness.
Frank

@FrankBie2

Here is another possiblity to get only the permissions from the anchestors.
https://github.com/FrankBie/django-cms/blob/permissions-merge2/cms/models/managers.py#L348

@kezabelle

Thanks for taking the time to get involved, Frank.

If I'm reading your code correctly, the PagePermissionManager.for_page_upper_tree() does the same as my proposed fix for PagePermissionManager.for_page() - that is, modify the queryset returned to only contain elements who are direct ancestors. Is there a reason that branch keeps for_page, or is it just vestigial?

Slightly different SQL logic I expect (yours does an IN query, mine just filters as if get_ancestors() were called, no idea which is better), but same intention/result, yes?

Is there a pull request for that somewhere? It doesn't appear to be part of #1106, probably rightly if that's tackling menus. But given you authored it in April, I assume there's a reason it's not in as a fix already?

(Edit: rather than clog up your pull request with pointless remarks, I'll addendum this here: I'm super stoked to see that many tests being added)

@FrankBie2

Yes you are right, same intention, only the anchestors are taken. In April the pull request was on an outdated version.

The many tests do not add the scenario you cover in your test. - I will add that. - I think a combined solution will be the best.
Your version of PagePermissionManager.for_page is db friendliery.

@kezabelle

I have updated & documented the test case in cc45509
I've added and documented the fix in 11f7751
I've added a warning to the 2.2 release notes about the problem in 66fc8c4
All tests pass, docs build.

Apparently I accidently pressed close. The lack of confirmation is a bit lame, thanks GitHub!

@kezabelle kezabelle closed this Dec 10, 2011
@kezabelle kezabelle reopened this Dec 10, 2011
@FrankBie2

I think you could close it and add the menu issue pull request.

@FrankBie2 FrankBie2 added a commit to FrankBie2/django-cms that referenced this issue Jan 4, 2012
@FrankBie2 FrankBie2 add test for issue #1113 7107eb4
@ojii

@kezabelle can you confirm that #1106 fixes this issue?

@kezabelle

Nope, because it only attacks potential problems in the menu (which, yes, it appears to fix, but we all know menus + permissions is a beast, I'm depending on what I can see, and what the tests prove.) This patch addresses an issue where a Page is not accessible, even though none of it's ancestors set permissions on it.

By way of example, if I set a view restriction on /page1/members/ and then want to give out a link (email, IM, whatever) to someone to visit /page1/nonmembers/about/, the permissions of the members page (same tree, ancestral cousin) may stop them seeing it.

Currently, the only way of avoiding that is to use the CURRENT_PAGE permission everywhere you need restrictions.

@FrankBie2

@kazabelle
The tests check the menu item visiblitiy, and the accessiblity of the items.
Have you checked https://github.com/divio/django-cms/pull/1106/files#L4R492
as this is a testcase to check the scenario you are speaking about.

My fault, have given the wrong grant_on in the test. Will update it and add your code to the page_for, to get the test running

@FrankBie2

test is running now :-)

@ojii

fixed in 2297552 ?

@ojii ojii closed this Jan 9, 2012
@FrankBie2 FrankBie2 added a commit to VillageAlliance/django-cms that referenced this issue Jan 24, 2014
@FrankBie2 FrankBie2 update testcase "View permissions for != 'Current Page' may not work …
…as expected."

issue divio/django-cms#1113
a9f6a3b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment