On 19/01/12 22:30, Cody A.W. Somerville wrote: > 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 was told this had already been scheduled and had the end of this week as a deadline, but it looks like that's not really the case? I can understand your concerns but, imo, moving this feature into an experimental branch without agreeing on a plan to address your concerns is just going to make it more likely that we'll never do that -- anything will have a higher priority than something that's not used in production. > > 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, > -- 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
