Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(309)

Issue 13525044: Check ACL permissions when getting subscribers (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by ana.balica
Modified:
10 years, 7 months ago
Reviewers:
thomas.j.waldmann
Visibility:
Public.

Description

Related to this issue - https://bitbucket.org/ana-balica/moin-2.0/issue/14/get_subscribers-avoids-the-protection

Patch Set 1 #

Total comments: 3

Patch Set 2 : Fix order and acl permissions in tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -4 lines) Patch
M MoinMoin/util/_tests/test_subscriptions.py View 1 2 chunks +8 lines, -1 line 0 comments Download
M MoinMoin/util/subscriptions.py View 1 2 chunks +5 lines, -3 lines 0 comments Download

Messages

Total messages: 1
Thomas.J.Waldmann
10 years, 8 months ago (2013-09-23 17:17:15 UTC) #1
https://codereview.appspot.com/13525044/diff/1/MoinMoin/util/_tests/test_subs...
File MoinMoin/util/_tests/test_subscriptions.py (right):

https://codereview.appspot.com/13525044/diff/1/MoinMoin/util/_tests/test_subs...
MoinMoin/util/_tests/test_subscriptions.py:71: ACL: u"-{0}:read,write
All:read,write".format(user1.name0)}
maybe rather use

"{0}: All:read,write"

meaning that that user isn't allowed to do anything.

https://codereview.appspot.com/13525044/diff/1/MoinMoin/util/subscriptions.py
File MoinMoin/util/subscriptions.py (right):

https://codereview.appspot.com/13525044/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:53: from MoinMoin.user import User
maybe don't do imports here without special reason.

https://codereview.appspot.com/13525044/diff/1/MoinMoin/util/subscriptions.py...
MoinMoin/util/subscriptions.py:58: if email:
looks a bit like wrong order.

"user.get(EMAIL)" and "if email" are cheap operations, so should be done first.

and then, if that is True, create the User object and call .may() (which is not
cheap).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b