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.
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?
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.
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.
2. Unfortunately, the model managers alone are not sufficient to
necessarily protect the privacy of private objects. For example, the
very commonly used django.shortcuts.get_{object,list}_or_404 functions
and other pieces of internal Django machinery use the 'default' manager
for a model, which is the first manager declared, which provides
unrestricted access objects. Some views have been updated to use a
wrapper function called 'get_possibly_private_object' or methods such as
'accessible_by_user' on the all_objects manager but failure to do so
causes odd behaviour, breakage of existing functionality, and/or
Again, just to be clear, failure to do so will cause some stuff not to
work for private projects, but never for public ones. It's easy to audit
the existing code and fix any places that are still using the
non-privacy-aware helpers, and there are various ways we can prevent new
code from using the non-privacy-aware helpers. Again, something that can
be fixed easily but I don't think a feature branch would help.
disclose of (or permit operations on) private objects. Ex. It is
possible for a user who has the necessary system wide permission to
queue builds to queue builds for a private project by directly accessing
URL due to oversight in updating use of get_object_or_404.
Indeed, that's a good catch! Although there's no leakage of private
data, and as I mentioned above, there are ways we can make automatically
catch those oversights.
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?
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?
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.
6. Idle builders erroneouslylist the current job as 'private build'.
A bug, trivial to fix.
7. Not much consideration given to UI- specifically, the columns on the
project details page are broken (due to using buggy css/html that I
borrowed from Launchpad). Additionally, the label for the 'is_private'
field on the project forms is simply 'is private' which looks weird. On
the same thread, not all deployment would want users to be able to
select owners and make projects private as they see fit.
Ditto
--
Guilherme Salgado <https://launchpad.net/~salgado>
--
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
