Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

permission check for login_required now checks all ancestors for that attribute too #1112

Closed
wants to merge 1 commit into from

5 participants

@evildmp
Owner

No description provided.

@ojii ojii commented on the diff
cms/views.py
@@ -92,13 +92,16 @@ def details(request, slug):
return HttpResponseRedirect(redirect_url)
# permission checks
- if page.login_required and not request.user.is_authenticated():
- if settings.i18n_installed:
- path = urlquote("/%s%s" % (request.LANGUAGE_CODE, request.get_full_path()))
- else:
- path = urlquote(request.get_full_path())
- tup = settings.LOGIN_URL , "next", path
- return HttpResponseRedirect('%s?%s=%s' % tup)
+ # check whether authenticated first; that we don't waste time doing anything else
+ if not request.user.is_authenticated():
+ # check this page and all its ancestors for login_required
+ if page.login_required or [ancestor for ancestor in page.get_ancestors() if ancestor.login_required]:
@ojii Collaborator
ojii added a note

does "page.get_ancestors().filter(login_required=True).exists()" not work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii
Collaborator

this would really benefit from some sort of test case

@yakky yakky referenced this pull request from a commit in yakky/django-cms
@yakky yakky Test PagesTestCase.test_login_restriction to check for #1112 bug. b5d5ad6
@yakky
Collaborator

Commit above is the test for this issue.
Confirms @evildmp report and proposed fix
page.get_ancestors().filter(login_required=True).exists()-variant of the patch works as well

@evildmp
Owner
@yakky
Collaborator

I wrote the test before any answer was posted :)
Test is in commit yakky@b5d5ad6

@evildmp
Owner

This appears to have been left out of the release candidate - was that deliberate, or an oversight?

@yakky
Collaborator

I guess @ojii would like me to rework the test without Client object before merging :)

@ojii
Collaborator

Yes, this is targeted for a 2.3.1 release.

@piquadrat
Collaborator

If I read the code correctly, this check adds one query per page view for unauthenticated users. We already are quite heavy on the database, I'm not sure if the trade of is worth it in this case.

How about propagating the login_required down the descendants chain on save? e.g.:

if self.login_required:
    self.get_descendants().update(login_required=True)
@ojii
Collaborator

while not 100% happy with the solution, I'm +1 on what @piquadrat suggested.

@yakky
Collaborator

Looks like a really simple solution: any idea on how this could interact with moderation? I know it's in deprecated, but in 2.3 branch it's still supposed to work.

@ojii
Collaborator

@yakky was viewperms/login-required ever covered by moderation?

@piquadrat
Collaborator

hrm, another problem with my update approach: it breaks the audit trail for the descendants. It's not possible anymore to figure out from the version history when a page was made login_required.

@yakky
Collaborator

@ojii Not on the view side, but on the save side.
Just wondering if login_attribute is involved in moderation: I haven't really digged the moderator side of the code.
Test I wrote for this does not cover moderator ATM

@ojii
Collaborator

@piquadrat solution: create history objects? Or for-loop-save?

@yakky my concern is, if we want to make this moderator enabled, this ticket is dead.

@yakky
Collaborator

@ojii Just thinking out loud.
I'm all for killing moderation and having something better: it's a mess to implement on a site and explain to the users, I can barely understand how messy is to maintain this.
Just updating moderation docs to explain this behavior should be fine: something like:

Please note that login restriction is immediately applied to the page and all its descendants

@piquadrat
Collaborator

OK, how about this: if we get rid of another query in this execution path, we'll let this one in :)

I already turned this:

SELECT "cms_page"."id", "cms_page"."created_by", "cms_page"."changed_by", "cms_page"."parent_id", "cms_page"."creation_date", "cms_page"."changed_date", "cms_page"."publication_date", "cms_page"."publication_end_date", "cms_page"."in_navigation", "cms_page"."soft_root", "cms_page"."reverse_id", "cms_page"."navigation_extenders", "cms_page"."published", "cms_page"."template", "cms_page"."site_id", "cms_page"."moderator_state", "cms_page"."level", "cms_page"."lft", "cms_page"."rght", "cms_page"."tree_id", "cms_page"."login_required", "cms_page"."limit_visibility_in_menu", "cms_page"."publisher_is_draft", "cms_page"."publisher_public_id", "cms_page"."publisher_state" FROM "cms_page" LEFT OUTER JOIN "cms_page" T2 ON ("cms_page"."parent_id" = T2."id") INNER JOIN "django_site" ON ("cms_page"."site_id" = "django_site"."id") WHERE T2."id" IS NULL ORDER BY "django_site"."domain" ASC, "cms_page"."tree_id" ASC, "cms_page"."lft" ASC

into this:

SELECT (1) AS "a" FROM "cms_page" LEFT OUTER JOIN "cms_page" T2 ON ("cms_page"."parent_id" = T2."id") WHERE T2."id" IS NULL LIMIT 1

in get_page_from_path :)

@ojii
Collaborator

all I see a much shorter scroll bar on github, so that looks very nice to me

@piquadrat
Collaborator

I'd like to cash in one of the roughly ten imperial bazillion[1] queries saved by #1362 to fix this the proper way.

[1] according to @ojii, 1 imperial bazillion == 3.

@ojii
Collaborator

go!

@digi604
Collaborator

what is the status of this PR?

@evildmp
Owner

This can be closed now surely. I am not sure what has happened with the code, but certainly since 2.3.something it has not been possible to view - incorrectly - descendants of pages with view restrictions

@evildmp evildmp closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 9, 2011
  1. @evildmp
This page is out of date. Refresh to see the latest.
Showing with 10 additions and 7 deletions.
  1. +10 −7 cms/views.py
View
17 cms/views.py
@@ -92,13 +92,16 @@ def details(request, slug):
return HttpResponseRedirect(redirect_url)
# permission checks
- if page.login_required and not request.user.is_authenticated():
- if settings.i18n_installed:
- path = urlquote("/%s%s" % (request.LANGUAGE_CODE, request.get_full_path()))
- else:
- path = urlquote(request.get_full_path())
- tup = settings.LOGIN_URL , "next", path
- return HttpResponseRedirect('%s?%s=%s' % tup)
+ # check whether authenticated first; that we don't waste time doing anything else
+ if not request.user.is_authenticated():
+ # check this page and all its ancestors for login_required
+ if page.login_required or [ancestor for ancestor in page.get_ancestors() if ancestor.login_required]:
@ojii Collaborator
ojii added a note

does "page.get_ancestors().filter(login_required=True).exists()" not work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if settings.i18n_installed:
+ path = urlquote("/%s%s" % (request.LANGUAGE_CODE, request.get_full_path()))
+ else:
+ path = urlquote(request.get_full_path())
+ tup = settings.LOGIN_URL , "next", path
+ return HttpResponseRedirect('%s?%s=%s' % tup)
template_name = get_template_from_request(request, page, no_current_page=True)
# fill the context
Something went wrong with that request. Please try again.