On 12-01-18 10:33 AM, Guilherme Salgado wrote:
On 17/01/12 23:26, Cody A.W. Somerville wrote:
Hi Folks,
I've reviewed the private project functionality currently committed to
trunk. This is some exciting work that I'm looking forward to leveraging
but unfortunately I do not feel the code is ready to live in trunk and
thus intend to move the code into a feature branch to incubate (once
this occurs and we begin discussion on my points below I'll be able to
provide more details on what to expect regarding a timeline for the code
I'd like to know more about what you want to achieve by moving it into
a feature branch to incubate. If it's just to get the bugs and missing
features you describe below fixed, then I think it's not the right
thing to do as there will be less people looking at that branch and
for every bug we fix in the feature branch there will be two other
features added to trunk that won't work for private projects, when we
merge trunk into the feature branch.
The motivation is two-fold:
1. As you mention, there is remaining bugs and features to implement to
complete private projects. More importantly though there is a subset of
features and bugs that must be resolved before I would feel comfortable
deploying the changes - specifically points four through seven. With our
pristine trunk policy and being past due for a deployment for PES (which
aims to stay as close to trunk as possible - a goal that is evidently
also important for you), short term incubation in a feature branch is
immediately warranted in my opinion.
2. Accepting this code into trunk is a commitment to this
implementation. As I mentioned in my original e-mail, such a commitment
has a significant risk and associated cost due to the nature of this
particular feature and the invasiveness of the implementation. For this
reason, I feel incubation is rational until further consideration on
this problem can be scheduled into the roadmap.
I'm willing to commit to keeping this feature branch (or 'experimental
series', which might be a more accurate name) in sync with trunk. That
is, we'll merge trunk into the privacy branch every two weeks at a
minimum and keep it working. To be clear, I'm not trying to move this
code out of trunk so that it can live in an island of murky
responsibilities and maintainership. Please consider this as your code
being accepted 'upstream' into an 'experimental series' which you can
reliably base your deployment off of for the time being.
actually making it into trunk). Here are my findings, when viewed as a
whole, I feel is sufficiently compelling:
N.B. I intend to file bugs for some of the specific bugs I mention below
but didn't want to hold this e-mail up any longer. Apologies - I'll file
bugs and annotate bug numbers in a reply tomorrow morning.
1. All models that are potentially private have been updated to declare
two model managers in the same fashion. The first manager declared is
'all_objects' which is the AccessControlledObjectsManager manager (a
subclass of django-group-access's AccessManager manager) and it does not
restrict the queryset but does provide additional methods related to
access control. The second manager declared is 'objects' (which is the
same name as the default manager for models that do not declare their
own managers) which is the PublicOnlyObjectsManager manager and it only
allows public objects to be returned. Comments in the source code
explicitly explain that this approach is to "ensure that any oversights
when updating existing uses of (<Model>).objects won't leak private
(objects)". Although true, this design decision also makes it too easy
to make such oversights and result in breakage of existing functionality
(especially considering the current state of Offspring's test suite)
I see that as a trade off between making it hard to accidentally leak
private data and making it hard to accidentally restrict ourselves to
only-public data, which could possibly break functionality *for
private projects* (it's important to note that any accidental breakage
caused by this is most likely, by their very nature, to affect only
private projects).
Although it's obviously not our intent to make it easier to introduce
regressions, they happen all the time (even on well-tested code bases)
and can be caught during QA or even dealt with after it reaches
production (given a reasonably agile process it can be fixed before
most users notice it). Leaking private data, on the other hand, may
have severe implications, so we opted to make it hard to leak private
data.
I'm sure everyone would be happy to discuss alternative
implementations that avoided this trade off. Do you have one in mind?
Renaming 'objects' to 'public_objects' may be one possible solution
(with possible cons as well)
which is indeed the case. ex. It is no longer possible to cancel queued
builds for a private project and the 'needs_build' property on the
It was never possible to cancel builds of private projects; this is
just a missing feature. Yeah, sure, some people might expect to be
able to cancel builds (is that really used often, I wonder?),
regardless of whether the project is private or not, but that's far
from a big deal, I'm sure we all agree on that.
I consider it a bug since it is due to not being updated and thus still
only uses the PublicOnlyObjectsManager manager like the next issue.
Project model will not be accurate for private projects. Both are due to
oversight in updating use of objects model manager.
This one is just a bug that needs to be fixed, and as I mentioned
above, only affect private projects/builds.
I've filed LP #919007 for these two issues.
<snip>
3. Additionally, I suspect that this approach will cause problems (and
confusion - calling the public manager something other than objects, ex.
'public_objects', would probably be better to make it clear the model
needs special 'privacy' considerations - e.g. failing explicitly is
That's trivial to change, right?
preferable) down the road when third-party applications or code attempt
to interact with our models. As a result, accepting this approach into
trunk will require us to adapt existing and future code to be
privacy-aware and thus be a commitment that could be extremely costly to
back out, due to the invasive nature, if ever needed.
From this comment I can't help but wonder that either you think
support for private projects does not belong in offspring or you have
a better design that will be less invasive?
I do have some ideas but not the time currently to pursue them further.
Thus why I want to help maintain this implementation with it possibly
making it to trunk in the future but not commit to it by accepting it
into trunk now.
4. It is not currently possible to update the owner or the 'is_private'
flag on a project via the django admin UI. It is, however, possible to
do so via the 'update project' form in the front-end which is accessible
to anyone who has the system wide permission to edit projects. Thus, it
is quite easy to make a project inaccessible to (for all practical
purposes) everyone including admins and the only remedy is to manually
modify the database (as we wouldn't even be able to see who the owner
was mistakenly set to).
Once again, trivial to fix, right?
I've filed LP #919008 on this issue.
5. Admins can not see or interact with private objects in the front-end
unless they've been granted explicit access to the private objects by
the private object owner.
Another missing feature, although hardly essential if we allow admins
to do so via the admin UI. Do we want Django site admins to be able to
see/change private projects, though? I'm not sure everyone would agree
the answer to that is yes.
'Site admins' should be able to see and interact with private objects in
the front-end but we shouldn't get into a situation where we have to
excessively give 'site admin' to people to work around deficiencies in
handing out more refined sets of permissions for individuals who only
require certain elevated privileges.
<snip>
Cheers,
--
Cody A.W. Somerville
Release Engineer
Commercial Engineering
Canonical Canada Ltd.
Phone: +1 781 850 2087
Cell: +1 613 401 5141
Fax: +1 613 687 7368
Email: cody.somervi...@canonical.com
--
Mailing list: https://launchpad.net/~offspring-hackers
Post to : offspring-hackers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~offspring-hackers
More help : https://help.launchpad.net/ListHelp
