Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Wed, Aug 4, 2010 at 9:47 PM, James Westby jw+deb...@jameswestby.net wrote: ... An alternative to turning it off would be to only produce one warning per method, per run. That would still provide the warning but also be much less output. https://code.edge.launchpad.net/~jml/launchpad/show-warnings-once/+merge/31831 jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 08/05/2010 06:18 AM, Jonathan Lange wrote: On Wed, Aug 4, 2010 at 9:47 PM, James Westby jw+deb...@jameswestby.net wrote: ... An alternative to turning it off would be to only produce one warning per method, per run. That would still provide the warning but also be much less output. https://code.edge.launchpad.net/~jml/launchpad/show-warnings-once/+merge/31831 It's still nagging us to fix something we haven't agreed needs fixing, though. Personally, I think we probably should use proxified objects in tests, but I don't think nagging is the right way to go about it. My solution is here: https://code.edge.launchpad.net/~abentley/launchpad/no-proxy-warnings/+merge/31781 Those who want the warnings can have them, and everyone else doesn't have to put up with them. Aaron -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkxaxysACgkQ0F+nu1YWqI1smACcD+WNmD20scrYK9AfzDFcEFQ8 oLoAn3ru4Trk2e1NGaodb17cUvLJuZL3 =Uxvi -END PGP SIGNATURE- ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Thu, 05 Aug 2010 10:14:03 -0400, Aaron Bentley aa...@canonical.com wrote: It's still nagging us to fix something we haven't agreed needs fixing, though. Personally, I think we probably should use proxified objects in tests, but I don't think nagging is the right way to go about it. Agreed. Certainly if we don't want to do this we shouldn't nag, and the current output isn't helpful either way. However, assuming that we want proxied object in tests... My solution is here: https://code.edge.launchpad.net/~abentley/launchpad/no-proxy-warnings/+merge/31781 Those who want the warnings can have them, and everyone else doesn't have to put up with them. this is a fine stopgap solution if we have people using the environment variable. jml's solution is better once we have fixed all known cases though, as it doesn't require you to have an environment variable set to know that you are adding a new problem. Thanks, James ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Thu, Aug 5, 2010 at 3:43 PM, James Westby jw+deb...@jameswestby.net wrote: On Thu, 05 Aug 2010 10:14:03 -0400, Aaron Bentley aa...@canonical.com wrote: ... My solution is here: https://code.edge.launchpad.net/~abentley/launchpad/no-proxy-warnings/+merge/31781 Those who want the warnings can have them, and everyone else doesn't have to put up with them. +1 this is a fine stopgap solution if we have people using the environment variable. jml's solution is better once we have fixed all known cases though, as it doesn't require you to have an environment variable set to know that you are adding a new problem. FWIW, mine didn't actually work. I'll have another stab at it soon. jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/21/2010 12:54 PM, Abel Deuring wrote: The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1 is at present in ec2 and will land soon. This stuff is interfering with work. I just lost a set of test results because I ran them under screen, and the warnings pushed the actual test results out of screen's buffer. Even when it doesn't make test results impossible to read, it can make them very hard to read among the noise. sorry for any inconvenience... This is more than merely inconvenient, it's wasting hours of time, and I'm strongly inclined fix it by silencing the warnings. Would people reject such a change in code review? Aaron -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkxZaRQACgkQ0F+nu1YWqI0R5QCfcbWDi4EXDhSw/xfqQvDR+cgC y+IAnRHp+v+7XUuHfSE2zIm1A32mASDD =a36T -END PGP SIGNATURE- ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Wednesday 04 August 2010 14:20:23 Aaron Bentley wrote: On 07/21/2010 12:54 PM, Abel Deuring wrote: The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1 is at present in ec2 and will land soon. This stuff is interfering with work. I just lost a set of test results because I ran them under screen, and the warnings pushed the actual test results out of screen's buffer. Even when it doesn't make test results impossible to read, it can make them very hard to read among the noise. sorry for any inconvenience... This is more than merely inconvenient, it's wasting hours of time, and I'm strongly inclined fix it by silencing the warnings. Would people reject such a change in code review? Not me. I never find nagging an incentive to change anything, just ask my wife. ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
I'm still unconvinced that this is *actually* closer to how the code runs in production, based on jml's response earlier in this thread. With that in mind I'm entirely happy with disabling this check pending future discussion, particularly as it is causing developers to feel lots of friction. -Rob ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Thu, 2010-08-05 at 08:06 +1200, Robert Collins wrote: I'm still unconvinced that this is *actually* closer to how the code runs in production, based on jml's response earlier in this thread. I landed a few changes because of this. I think everyone should be aware of two discoveries that concerned me. 1. The while looking at one report on a naked object I saw a test description disagreed with a test result. I was really puzzled. Really, because I wrote all the code and I knew re-reading it that it was wrong. There was a typo in the attr assignment. Login with the wrapped object showed that that attr did not exist. Using the real attribute did make the test work as I expected: items orders from 'hot' to 'cold' 2. While wrapping the milestone I saw factory.makeMilestone() rarely made a valid milestone. A milestone must have a product or distro series. No test with a distro milestone could render a milestone, release, or distribution view. We did not have a test that exercised these conditions with the factory made milestone, but lp engineers have tried to help users be creating milestones using SQL, and these same milestones definitely cause exceptions last year. With that in mind I'm entirely happy with disabling this check pending future discussion, particularly as it is causing developers to feel lots of friction. It certainly stole a day from me. I think the registry team have branches that fixed or fix all the issues reported running registry tests. -- __Curtis C. Hovey_ http://launchpad.net/ signature.asc Description: This is a digitally signed message part ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Wed, 04 Aug 2010 16:25:31 -0400, Curtis Hovey curtis.ho...@canonical.com wrote: I landed a few changes because of this. I think everyone should be aware of two discoveries that concerned me. 1. The while looking at one report on a naked object I saw a test description disagreed with a test result. I was really puzzled. Really, because I wrote all the code and I knew re-reading it that it was wrong. There was a typo in the attr assignment. Login with the wrapped object showed that that attr did not exist. Using the real attribute did make the test work as I expected: items orders from 'hot' to 'cold' 2. While wrapping the milestone I saw factory.makeMilestone() rarely made a valid milestone. A milestone must have a product or distro series. No test with a distro milestone could render a milestone, release, or distribution view. We did not have a test that exercised these conditions with the factory made milestone, but lp engineers have tried to help users be creating milestones using SQL, and these same milestones definitely cause exceptions last year. 3. While adding tests to avoid regressions for the ones I was fixing I found 5 classes which did not correctly implement the interfaces they claimed to. I assumed it was standard practice to verifyObject() an instance of each new class you wrote, but there seems to be a hole here. The cases I found were minor, and I'm not sure how a major bug would squeeze in here, but it still seems like a hole. It certainly stole a day from me. I think the registry team have branches that fixed or fix all the issues reported running registry tests. Soyuz also has the SoyuzTestPublisher which was creating some objects that weren't proxied. Fixing that is proving to be more time consuming than any I have fixed in the factory. I am fixing these each time I see a warning in the tests I run (when I run a subset), so taking the warning away would mean that I would stop doing that. From a complete test run earlier today we still have: test_publishing.py complaints about removeSecurityProxy (now ShouldThisBeUsingRemoveSecurityProxy). This can just be replaced with removeSecurityProxy, and I have that in an in-progress branch here, but it will be a while before that lands. Someone with a branch ready to submit could fold that in. This is by far and away the most number of lines in the test output. makeSeries makeBranchMergeProposal makeCodeReviewVoteReference makeSourcePackageRecipeBuildJob makeTranslationMessage makePOTMsgSet makeTranslationImportQueueEntry So there are not too many left to fix. An alternative to turning it off would be to only produce one warning per method, per run. That would still provide the warning but also be much less output. If we are going to ditch the proxy, or turn it off in tests, or something, then could we please do it soon? Otherwise I will waste even more hours of my time. Thanks, James ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Wed, 2010-08-04 at 16:47 -0400, James Westby wrote: 3. While adding tests to avoid regressions for the ones I was fixing I found 5 classes which did not correctly implement the interfaces they claimed to. I assumed it was standard practice to verifyObject() an instance of each new class you wrote, but there seems to be a hole here. The cases I found were minor, and I'm not sure how a major bug would squeeze in here, but it still seems like a hole. https://bugs.edge.launchpad.net/bugs/87199 The only reason the results look better is because we move most of the model that utilities/check-content-interfaces.py Soyuz also has the SoyuzTestPublisher which was creating some objects that weren't proxied. Fixing that is proving to be more time consuming than any I have fixed in the factory. I know jcsackett's branch fixes two of these. May the fasted code land first. -- __Curtis C. Hovey_ http://launchpad.net/ signature.asc Description: This is a digitally signed message part ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 28.07.2010 20:02, James Westby wrote: On Wed, 21 Jul 2010 18:54:50 +0200, Abel Deuring abel.deur...@canonical.com wrote: The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1 is at present in ec2 and will land soon. Its main change affects LaunchpadObjectFactory: This class has at present ca 20 makeWhatever() methods which return objects without a security proxy. If this happens, LaunchpadObjectFactory will now print a warning. I intend to land further branches where the affected methods will changed so that they return security proxied objects. This will in turn cause a larger number of test failures. As a simple fix/workaround, I added a function remove_security_proxy_and_shout_at_engineer(obj) which just returns removeSecurityProxy(obj) but it also prints a warning to stderr. Properly fixing all tests would take far too much time -- and after all, the test behaviour will be the same as before. The only difference is that we will clearly see where our tests work with bad objects. Hi, Just a couple of observations as I got some of this in my tests. I realise things may be changing, but I wanted to record them as I found them. PLEASE FIX: LaunchpadObjectFactory.getAnyPocket returns an unproxied object. - This returns an enum value, which I don't think can be proxied? Right. This means that I forgot to include DBItem in the tuple factory.unwrapped_types. Abel PLEASE FIX: LaunchpadObjectFactory.makeSourcePackagePublishingHistory returns an unproxied object. - This does # SPPH and SSPPH IDs are the same, since they are SPPH is an SQLVIEW # of SSPPH and other useful attributes. return SourcePackagePublishingHistory.get(sspph.id) what would be the way to get a proxied object if that is what we want? Thanks, James ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Tue, Jul 27, 2010 at 10:39 PM, Michael Hudson michael.hud...@canonical.com wrote: ... Do you have an example of a problem we've caught because we were using security proxies in our model tests? Even a contrived example of the kind of problem we catch would help. One would be a typo in the interface definition. For example: class IThing: branchs = Choice(...) The unit tests for Things, if not using proxies, could happily assign to and read from 'branches', but the 'real' code (e.g. accessing it via a secured utility in the browser) accessing branches would blow up. Is that the sort of thing you meant? It is. I guess almost every other Python project in the world gets by without that though. To me it all just seems to be an extension of the 'go in through the front door' test principle: you should use the SUT in the test in a way close to how it's actually going to be used in anger. I'm not sure. Genuinely uncertain. Yes the SUT in the test should be close to production. On the other hand, I see much of the permission / login stuff in model tests as meaningless and useless, and can't help but speculate on how it affects test run time. jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2010 05:37 AM, Abel Deuring wrote: Right. This means that I forgot to include DBItem in the tuple factory.unwrapped_types. It doesn't seem very good to have a list of types that we don't have to proxy. Updating this list is already causing conflicts. Can't we determine which objects were supposed to be proxied from the Zope machinery? Aaron -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkxRf4gACgkQ0F+nu1YWqI2NqACdHcyfbtK0uuqtAuESIZ9vPxc3 XKAAni5rZwa0Qj9JH117znArBwUrxyeq =6dos -END PGP SIGNATURE- ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Thu, 29 Jul 2010 09:18:04 -0400, Aaron Bentley aa...@canonical.com wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/29/2010 05:37 AM, Abel Deuring wrote: Right. This means that I forgot to include DBItem in the tuple factory.unwrapped_types. It doesn't seem very good to have a list of types that we don't have to proxy. Updating this list is already causing conflicts. Can't we determine which objects were supposed to be proxied from the Zope machinery? from zope.security import checker dont_wrap = checker.BasicTypes.get(type(ob)) == checker.NoProxy is a start. Cheers, mwh ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 2010-07-27 16:32, Jonathan Lange wrote: On Tue, Jul 27, 2010 at 2:05 PM, Jeroen Vermeulenj...@canonical.com wrote: ... I'm inclined to agree with Abel: it's still better for us to run into test complications and be regularly worried about security proxies than to lose the mental reinforcement of the security model. Lose the reinforcement and we'll gradually lose our inhibitions w.r.t. creating unproxied objects. Do you have an example of a problem we've caught because we were using security proxies in our model tests? Even a contrived example of the kind of problem we catch would help. I don't think that's particularly likely, because the problems we see in testing are ones from having proxies where we don't expect them. Production problems with proxies would probably be ones where we don't have proxies. What I'm saying is that that doesn't necessarily justify dropping proxies from our testing, because running into proxies during testing reinforces proxy-aware design and code, and promotes understanding of how the proxies work. My feeling is that without the occasional permissions nuisance in test setup, we'd eventually (perhaps one or two generations of engineers down the line) start to make more silent mistakes. An example of such mistakes would be to write BugSet().createBug instead of getUtility(IBugSet).createBug. Essentially we catch these all the time before we even write the code, or during review, because of enforced awareness. That said, being already somewhat aware of proxies I'd probably be quite happy to use a context manager for I've set up my data, now run this under more realistic restrictions. Jeroen ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Wed, 21 Jul 2010 18:54:50 +0200, Abel Deuring abel.deur...@canonical.com wrote: The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1 is at present in ec2 and will land soon. Its main change affects LaunchpadObjectFactory: This class has at present ca 20 makeWhatever() methods which return objects without a security proxy. If this happens, LaunchpadObjectFactory will now print a warning. I intend to land further branches where the affected methods will changed so that they return security proxied objects. This will in turn cause a larger number of test failures. As a simple fix/workaround, I added a function remove_security_proxy_and_shout_at_engineer(obj) which just returns removeSecurityProxy(obj) but it also prints a warning to stderr. Properly fixing all tests would take far too much time -- and after all, the test behaviour will be the same as before. The only difference is that we will clearly see where our tests work with bad objects. Hi, Just a couple of observations as I got some of this in my tests. I realise things may be changing, but I wanted to record them as I found them. PLEASE FIX: LaunchpadObjectFactory.getAnyPocket returns an unproxied object. - This returns an enum value, which I don't think can be proxied? PLEASE FIX: LaunchpadObjectFactory.makeSourcePackagePublishingHistory returns an unproxied object. - This does # SPPH and SSPPH IDs are the same, since they are SPPH is an SQLVIEW # of SSPPH and other useful attributes. return SourcePackagePublishingHistory.get(sspph.id) what would be the way to get a proxied object if that is what we want? Thanks, James ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Wednesday 28 July 2010 19:02:52 James Westby wrote: PLEASE FIX: LaunchpadObjectFactory.makeSourcePackagePublishingHistory returns an unproxied object. - This does # SPPH and SSPPH IDs are the same, since they are SPPH is an SQLVIEW # of SSPPH and other useful attributes. return SourcePackagePublishingHistory.get(sspph.id) what would be the way to get a proxied object if that is what we want? I removed the Secure*PublishingHistory tables and converted the views to tables a while ago now. The code can be changed to simply return the sspph (but rename the variable!). Cheers. ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Monday 26 July 2010 17:54:00 Benji York wrote: On Mon, Jul 26, 2010 at 12:13 PM, Julian Edwards julian.edwa...@canonical.com wrote: If it is *really* needed, I would *much* rather see an explicit removeSecurityProxy() with a comment explaining why you need to remove the wrapper. It should be a conscious exception, not a trap you can fall into. +1 I've fallen into that trap myself. As a result, if I have to remove a security proxy (in non-test code) I ask myself if the operation I'm about to do is one the user shouldn't be able to do of their own accord (otherwise it shouldn't be restricted by the security proxy in the first place) and I'm removing the security proxy because the system needs to perform some action that the user himself isn't allowed to do. Another rule of thumb I follow is that if I remove a security proxy I try not to bind the naked object to a name but instead perform the operation in the same expression as the call to removeSecurityProxy. That way I don't introduce any unintentional un-proxied operations later. Hi Benji One of our conventions is to do something like this, although your way is also something I've seen in our code. naked_thing = removeSecurityProxy(thing) If that's not possible I'll explicitly del the name binding as soon as I'm done with it (with copious comments to explain what's going on). Adding more comments is rarely bad :) ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Monday 26 July 2010 17:57:48 Aaron Bentley wrote: On 07/26/2010 12:42 PM, Julian Edwards wrote: On Monday 26 July 2010 17:28:01 Aaron Bentley wrote: If there's no @classmethod on new() then you need an instantiated object. At that point, it's more likely that you've DTRT regarding securedutility rather than circumventing the proxy by calling the classmethod. Okay, point taken. Though the scope where you can import Foo in order to call Foo.new is already pretty restricted due to the importfascist. So, if I understand this stuff right, you simply don't need the @classmethod if you call getUtility(IFoo).new() The @classmethod means that you can register the class Foo, not instances of the class Foo, as a utility, which means that you don't need an instance of Foo in order to call Foo.new. A, now I see why it has it. I didn't see that it was registering the class rather than the instance, so this makes much more sense now. If you don't have an @classmethod on Foo, then you need an instance of Foo in order to call Foo.new on it, and that's crazy. So no classmethod means you need to hang new() off a FooSet class instead of Foo. Personally, I've always thought it was strange that we have to instantiate these FooSet objects, since they don't have any state. If we keep on using them, I'd recommend using @classmethod/staticmethod on them :-) I don't think it's that crazy if you're using utilities. They're lazily- instantiated (or supposed to be!) singletons and we don't store any state in them either, so they're super lightweight. We could even rename them to XXXFactory utilities to make it obvious what's going on, and maybe move their existing collection querying methods to classmethods on the model object (getXXX etc). ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 5:54 PM, Benji York benji.y...@canonical.com wrote: On Mon, Jul 26, 2010 at 12:13 PM, Julian Edwards julian.edwa...@canonical.com wrote: If it is *really* needed, I would *much* rather see an explicit removeSecurityProxy() with a comment explaining why you need to remove the wrapper. It should be a conscious exception, not a trap you can fall into. +1 I've fallen into that trap myself. As a result, if I have to remove a security proxy (in non-test code) I ask myself if the operation I'm about to do is one the user shouldn't be able to do of their own accord (otherwise it shouldn't be restricted by the security proxy in the first place) and I'm removing the security proxy because the system needs to perform some action that the user himself isn't allowed to do. That sounds like a good idea. However, the question that I now have is this: what benefit do we gain by insisting on security-proxied objects in the unit tests for our model code? jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Tue, Jul 27, 2010 at 6:29 AM, Jonathan Lange j...@canonical.com wrote: However, the question that I now have is this: what benefit do we gain by insisting on security-proxied objects in the unit tests for our model code? If the model code doesn't operate on proxied objects in production, then I can't see any benefit. The hard part is being sure. -- Benji York ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Tue, 27 Jul 2010 07:50:26 -0400 Benji York benji.y...@canonical.com wrote: On Tue, Jul 27, 2010 at 6:29 AM, Jonathan Lange j...@canonical.com wrote: However, the question that I now have is this: what benefit do we gain by insisting on security-proxied objects in the unit tests for our model code? If the model code doesn't operate on proxied objects in production, then I can't see any benefit. The hard part is being sure. I just got my first FAILURE ec2 emails and I have to say that these warnings make it impossible to find the failing tests, and make the body of the email impossibly long. In my mind, that should be the only thing that shows in the body of the email, and you can put whatever other junk in the attachment. All I care about are the failing tests. Unless we have a clear complete plan for fixing these warnings, I suggest we back out this change. It's great that we get the nudging, but I also don't want to have to deal with this for months on end until the next Epic where we have some free-hack time to fix it. Cheers, Paul ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Tue, Jul 27, 2010 at 1:06 PM, Paul Hummer paul.hum...@canonical.com wrote: ... I just got my first FAILURE ec2 emails and I have to say that these warnings make it impossible to find the failing tests, and make the body of the email impossibly long. In my mind, that should be the only thing that shows in the body of the email, and you can put whatever other junk in the attachment. All I care about are the failing tests. I have a patch that greatly reduces the number of warnings and groups them all together at the end of the test run. It was rejected for silly Python compatibility reasons, but I hope to have it land later today. Perhaps this will be sufficient. Also, I have a work-in-progress branch that I've handed off to Maris that trims down the contents of the email. As a workaround for your current problems, if you load the attachment into testrepository (or use a subunit filter or whatever), it's really easy to get just the failing tests. (Either gzip -dc thing.gz | subunit --no-passthrough --no-skip | subunit-ls, or testr init; gzip -dc thing.gz | testr load; testr failing) jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Tue, 27 Jul 2010 13:21:29 +0100 Jonathan Lange j...@canonical.com wrote: As a workaround for your current problems, if you load the attachment into testrepository (or use a subunit filter or whatever), it's really easy to get just the failing tests. (Either gzip -dc thing.gz | subunit --no-passthrough --no-skip | subunit-ls, or testr init; gzip -dc thing.gz | testr load; testr failing) So I understand the value of testrepository and that I can use it trim this down in this case, but I don't think this is a habit we want to adopt every time. It's good that we're cleaning it up and fixing it, and that this long command line is merely a workaround. Cheers, Paul ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 2010-07-26 18:34, Abel Deuring wrote: On 26.07.2010 17:03, Jonathan Lange wrote: I used to agree, but now I'm not so sure. Can you give an example of the kind of permission problem we might miss, or of one that we've caught because we were using security proxies in our model tests? I can't give any good example. My reason simply is this: if we get an Unauthorized exception while iterating over the result of getUtility(IFooSet).getStuff(), we know that we should either fix getStuff() or use/write a method getStuffForUser(some_person). I'm inclined to agree with Abel: it's still better for us to run into test complications and be regularly worried about security proxies than to lose the mental reinforcement of the security model. Lose the reinforcement and we'll gradually lose our inhibitions w.r.t. creating unproxied objects. (Old-fashioned Iron Maiden education taught us this as take not thy thunder from us, but take away our pride. It may not have been about Zope originally.) Jeroen ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 28/07/10 02:32, Jonathan Lange wrote: On Tue, Jul 27, 2010 at 2:05 PM, Jeroen Vermeulenj...@canonical.com wrote: ... I'm inclined to agree with Abel: it's still better for us to run into test complications and be regularly worried about security proxies than to lose the mental reinforcement of the security model. Lose the reinforcement and we'll gradually lose our inhibitions w.r.t. creating unproxied objects. Do you have an example of a problem we've caught because we were using security proxies in our model tests? Even a contrived example of the kind of problem we catch would help. One would be a typo in the interface definition. For example: class IThing: branchs = Choice(...) The unit tests for Things, if not using proxies, could happily assign to and read from 'branches', but the 'real' code (e.g. accessing it via a secured utility in the browser) accessing branches would blow up. Is that the sort of thing you meant? To me it all just seems to be an extension of the 'go in through the front door' test principle: you should use the SUT in the test in a way close to how it's actually going to be used in anger. Cheers, mwh ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
Perhaps I missed a response, but I'm still very worried that we're simply toggling from a fail-secure bias to a fail-insecure bias. And that worries me much more than seeing removeSecurityProxy calls. Can anyone reassure me here? Please? -Rob ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 25.07.2010 15:31, Jonathan Lange wrote: On Wed, Jul 21, 2010 at 5:54 PM, Abel Deuring abel.deur...@canonical.com wrote: The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1 is at present in ec2 and will land soon. Its main change affects LaunchpadObjectFactory: This class has at present ca 20 makeWhatever() methods which return objects without a security proxy. If this happens, LaunchpadObjectFactory will now print a warning. I've got a branch up that tweaks this a little to actually emit warnings, rather than printing straight to stderr: https://code.edge.launchpad.net/~jml/launchpad/warnings-for-unproxied/+merge/30891 Sounds good. But: 1. I don't know why we raised errors for non-silenced warnings. In general, we should aim to get rid of all warnings in our code/tests. I would even consider the warnings introduced by my branch to be errors -- the point is that they occur far too often to be fixed in a single branch, so I opted instead to annoy everbody who looks at the output of affected tests ;) 2. I intend to replace the first warning (PLEASE FIX:...) by an exception once I added security wrappers to all objects returned by methods of LPObjectFactory. ... As a simple fix/workaround, I added a function remove_security_proxy_and_shout_at_engineer(obj) which just returns removeSecurityProxy(obj) but it also prints a warning to stderr. The text of the warning isn't particularly clear to me. If I start seeing this warning when running my tests, what should I do about it? It indicates that there is a bad workaround to prevent a test failure that is caused by a permission problem. So, when we work on an test that shows this warning, I think we should spend a few minutes to see how we can fix it: - consciously call removeSecurityProxy() because it is apparent that the test is not about access to the affected attribute by the current user. Might be reasonable for example in a setUp() method. (Though it is probably better to use the new with_person_logged_in) - login() as the right person - move an existing login() call a few lines up or down. - if the current user should have access to the attribute but hasn't, fix the permission. Abel ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 10:10 AM, Abel Deuring abel.deur...@canonical.com wrote: 1. I don't know why we raised errors for non-silenced warnings. In general, we should aim to get rid of all warnings in our code/tests. I would even consider the warnings introduced by my branch to be errors -- the point is that they occur far too often to be fixed in a single branch, so I opted instead to annoy everbody who looks at the output of affected tests ;) For future reference, see the patch James W put together to attach oopses to tests; a similar approach would have let you gather all the items without them showing up : we could then ratchet it down as a form of lint without having the UI exploding in our faces. Separately, I'm a little concerned we may spend a lot of time churning between users to setup things now - and that that will have performance implications for the test suite. Lastly, and here I expose my ignorance of some subtleties in zope - I thought security proxies only lived between view and model objects, not between model objects? -Rob ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 9:10 AM, Abel Deuring abel.deur...@canonical.com wrote: On 25.07.2010 15:31, Jonathan Lange wrote: On Wed, Jul 21, 2010 at 5:54 PM, Abel Deuring abel.deur...@canonical.com wrote: The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1 is at present in ec2 and will land soon. Its main change affects LaunchpadObjectFactory: This class has at present ca 20 makeWhatever() methods which return objects without a security proxy. If this happens, LaunchpadObjectFactory will now print a warning. I've got a branch up that tweaks this a little to actually emit warnings, rather than printing straight to stderr: https://code.edge.launchpad.net/~jml/launchpad/warnings-for-unproxied/+merge/30891 Sounds good. But: 1. I don't know why we raised errors for non-silenced warnings. In general, we should aim to get rid of all warnings in our code/tests. I would even consider the warnings introduced by my branch to be errors -- the point is that they occur far too often to be fixed in a single branch, so I opted instead to annoy everbody who looks at the output of affected tests ;) Don't worry, these warnings will still annoy everyone. Just only once per thing that needs fixing. 2. I intend to replace the first warning (PLEASE FIX:...) by an exception once I added security wrappers to all objects returned by methods of LPObjectFactory. That'll be even easier to do with the warning code I've added :) ... It indicates that there is a bad workaround to prevent a test failure that is caused by a permission problem. So, when we work on an test that shows this warning, I think we should spend a few minutes to see how we can fix it: - consciously call removeSecurityProxy() because it is apparent that the test is not about access to the affected attribute by the current user. Might be reasonable for example in a setUp() method. (Though it is probably better to use the new with_person_logged_in) - login() as the right person - move an existing login() call a few lines up or down. - if the current user should have access to the attribute but hasn't, fix the permission. OK, thanks. I'll try to update the docs/warning to incorporate this. jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 11:44 AM, Julian Edwards julian.edwa...@canonical.com wrote: On Monday 26 July 2010 10:29:56 Robert Collins wrote: Lastly, and here I expose my ignorance of some subtleties in zope - I thought security proxies only lived between view and model objects, not between model objects? That's right. Once the code inside a proxied object is running, it's effectively security-free and can see objects that the code outside of it would not normally be able to access. We need to be careful about this, because there's no protection against returning data to the caller that it should not see. So I don't understand this overall change then. If we're testing view code, we want something like: Proxy - model1 - model2 etc If we're testing model code, given that model code is unproxied as it interacts with other model code, we want model1 - model2 Only view code can depend on security proxies for permission checking, so making all our tests have security proxies *does not fit* our deployed object structure, and can easily fail by having a false sense of security. What about this: * Write a decorator factory that wraps *anything* it is asked for in a proxy, except one attribute 'unwrapped_factory' (which is the thing it is decorating). * Make the view tests get a decorated launchpad factory * Leave unit tests alone. This requires backing out the recent changes, but I think its the right thing todo because it will more accurately match how things work in production, which is the driving force behind this change in the first place. -Rob ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 26.07.2010 11:29, Robert Collins wrote: On Mon, Jul 26, 2010 at 10:10 AM, Abel Deuring abel.deur...@canonical.com wrote: 1. I don't know why we raised errors for non-silenced warnings. In general, we should aim to get rid of all warnings in our code/tests. I would even consider the warnings introduced by my branch to be errors -- the point is that they occur far too often to be fixed in a single branch, so I opted instead to annoy everbody who looks at the output of affected tests ;) For future reference, see the patch James W put together to attach oopses to tests; a similar approach would have let you gather all the items without them showing up : we could then ratchet it down as a form of lint without having the UI exploding in our faces. Separately, I'm a little concerned we may spend a lot of time churning between users to setup things now - and that that will have performance implications for the test suite. well, if this is a concern, simply calling removeSecurityProxy() should be fine in a setUp() method -- provided that the need to do that does not indicate a permission problem in the real world. Actually, I used removeSecurityProxy(obj) in LPObjectFactory.makeSomething(), if this method failed because obj is returned by LPObjectFactory.makeOther() and makeOther() now returns a proxied object. Lastly, and here I expose my ignorance of some subtleties in zope - I thought security proxies only lived between view and model objects, not between model objects? right. But many tests work like views (in the sense that login() calls are necessary), so LPObjectFactory should provide them with proxied objects. (Or did I miss your point?) Abel ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 11:06 AM, Robert Collins robert.coll...@canonical.com wrote: On Mon, Jul 26, 2010 at 11:44 AM, Julian Edwards julian.edwa...@canonical.com wrote: On Monday 26 July 2010 10:29:56 Robert Collins wrote: Lastly, and here I expose my ignorance of some subtleties in zope - I thought security proxies only lived between view and model objects, not between model objects? That's right. Once the code inside a proxied object is running, it's effectively security-free and can see objects that the code outside of it would not normally be able to access. This is true, but it's a little more complex than that. Security proxies live anywhere that someone has explicitly made a security proxy. Calls to ProxyFactory usually do it. In Launchpad, we have deliberately registered special ZCML handlers to make sure that our utilities are wrapped in security proxies (the securedutility directive). Thus, getUtility(IFooSet) returns a security-proxied IFooSet provider. Methods called on that IFooSet provider also return security-proxied objects. We might also have other things that wrap objects up in security proxies. A little bit of poking around with grep will tell you for sure. Now, while many of our model objects continue to access other model objects using attribute access or direct Storm query, others use utilities internally. This makes our internal security model Very Confused. (Poking around will also find model code that checks for permissions or even duplicates the logic found in canonical/launchpad/security.py). jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 26.07.2010 12:06, Robert Collins wrote: On Mon, Jul 26, 2010 at 11:44 AM, Julian Edwards julian.edwa...@canonical.com wrote: On Monday 26 July 2010 10:29:56 Robert Collins wrote: Lastly, and here I expose my ignorance of some subtleties in zope - I thought security proxies only lived between view and model objects, not between model objects? That's right. Once the code inside a proxied object is running, it's effectively security-free and can see objects that the code outside of it would not normally be able to access. We need to be careful about this, because there's no protection against returning data to the caller that it should not see. So I don't understand this overall change then. If we're testing view code, we want something like: Proxy - model1 - model2 etc If we're testing model code, given that model code is unproxied as it interacts with other model code, we want model1 - model2 Only view code can depend on security proxies for permission checking, so making all our tests have security proxies *does not fit* our deployed object structure, and can easily fail by having a false sense of security. What about this: * Write a decorator factory that wraps *anything* it is asked for in a proxy, except one attribute 'unwrapped_factory' (which is the thing it is decorating). * Make the view tests get a decorated launchpad factory * Leave unit tests alone. If we don't work with proxied objects in the unit tests, we may miss permission problems, unless the view tests cover each code path... This requires backing out the recent changes, but I think its the right thing todo because it will more accurately match how things work in production, which is the driving force behind this change in the first place. -Rob ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
I'm going to be travelling very shortly and offline for ~ 36 hours. I think we should improve our testing in this area. I don't think that having the core factory proxy the objects it returns is very faithful to how things are wired up in prod - jml's mail reinforces this for me. We can fail on security problems in two directions: 1 - miss something that will block a user inappropriately 2 - miss something that fails to block a user Having more proxies around objects that talk to each other in tests than there are around those objects in production will bias our test failures to (2). IMO This is a risk with more severe consequences than having a test suite biased to (1). Discuss. If you guys agree that the bias would shift to (2), and that that is a greater risk, please consider backing out the change and rethinking it - perhaps along the lines I suggested, perhaps along some other lines. -Rob ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Monday 26 July 2010 11:15:01 Jonathan Lange wrote: In Launchpad, we have deliberately registered special ZCML handlers to make sure that our utilities are wrapped in security proxies (the securedutility directive). Thus, getUtility(IFooSet) returns a security-proxied IFooSet provider. Methods called on that IFooSet provider also return security-proxied objects. I've seen a proliferation recently of people writing code like: class FlangeGrobbler: @classmethod def new(cls, ...) which completely bypasses the security adapter when returning new objects. I think this should stop and the code be converted to IFlangeGrobblerSet.new() style, or at the very least audited for security concerns. ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 11:46 AM, Julian Edwards julian.edwa...@canonical.com wrote: ... I've seen a proliferation recently of people writing code like: class FlangeGrobbler: �...@classmethod def new(cls, ...) which completely bypasses the security adapter when returning new objects. It depends on how you do it. You can declare a class as providing an interface and then register the class as secured utility for that interface. Grep for classProvides in the code to see examples. The problem isn't how you write the class, it's how you invoke new(). You can still write new() as a classmethod and invoke it with getUtility(IFooSet).new(). jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Monday 26 July 2010 11:55:00 Jonathan Lange wrote: On Mon, Jul 26, 2010 at 11:46 AM, Julian Edwards julian.edwa...@canonical.com wrote: ... I've seen a proliferation recently of people writing code like: class FlangeGrobbler: @classmethod def new(cls, ...) which completely bypasses the security adapter when returning new objects. It depends on how you do it. You can declare a class as providing an interface and then register the class as secured utility for that interface. Grep for classProvides in the code to see examples. The problem isn't how you write the class, it's how you invoke new(). You can still write new() as a classmethod and invoke it with getUtility(IFooSet).new(). I guess I'm struggling to see how @classmethod is useful then. In fact, it's dangerous. FWIW I don't think this way of doing things is any better (nor worse, though) than a utility class, is there a reason to avoid those? ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 12:48 PM, Julian Edwards julian.edwa...@canonical.com wrote: On Monday 26 July 2010 11:55:00 Jonathan Lange wrote: On Mon, Jul 26, 2010 at 11:46 AM, Julian Edwards julian.edwa...@canonical.com wrote: ... I've seen a proliferation recently of people writing code like: class FlangeGrobbler: �...@classmethod def new(cls, ...) which completely bypasses the security adapter when returning new objects. It depends on how you do it. You can declare a class as providing an interface and then register the class as secured utility for that interface. Grep for classProvides in the code to see examples. The problem isn't how you write the class, it's how you invoke new(). You can still write new() as a classmethod and invoke it with getUtility(IFooSet).new(). I guess I'm struggling to see how @classmethod is useful then. In fact, it's dangerous. Why do you think it's dangerous? FWIW I don't think this way of doing things is any better (nor worse, though) than a utility class, is there a reason to avoid those? There are a few: * An IFooSet is a poorly defined object, that doesn't do a very good job of representing any actual thing in the system * It is actually more work and more boilerplate to have a second class implement IFooSet (type it out!) * This extra work does not buy any meaningful separation of concerns. It's very normal to have code that creates an object together with the code that defines the behaviour of the object. jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 11:15 AM, Abel Deuring abel.deur...@canonical.com wrote: On 26.07.2010 12:06, Robert Collins wrote: On Mon, Jul 26, 2010 at 11:44 AM, Julian Edwards julian.edwa...@canonical.com wrote: On Monday 26 July 2010 10:29:56 Robert Collins wrote: Lastly, and here I expose my ignorance of some subtleties in zope - I thought security proxies only lived between view and model objects, not between model objects? That's right. Once the code inside a proxied object is running, it's effectively security-free and can see objects that the code outside of it would not normally be able to access. We need to be careful about this, because there's no protection against returning data to the caller that it should not see. So I don't understand this overall change then. If we're testing view code, we want something like: Proxy - model1 - model2 etc If we're testing model code, given that model code is unproxied as it interacts with other model code, we want model1 - model2 Only view code can depend on security proxies for permission checking, so making all our tests have security proxies *does not fit* our deployed object structure, and can easily fail by having a false sense of security. What about this: * Write a decorator factory that wraps *anything* it is asked for in a proxy, except one attribute 'unwrapped_factory' (which is the thing it is decorating). * Make the view tests get a decorated launchpad factory * Leave unit tests alone. If we don't work with proxied objects in the unit tests, we may miss permission problems, unless the view tests cover each code path... I used to agree, but now I'm not so sure. Can you give an example of the kind of permission problem we might miss, or of one that we've caught because we were using security proxies in our model tests? jml ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Monday 26 July 2010 16:01:31 Jonathan Lange wrote: I guess I'm struggling to see how @classmethod is useful then. In fact, it's dangerous. Why do you think it's dangerous? Because it allows someone to get hold of a new object that is not security wrapped. I don't know why you'd ever want to do that. FWIW I don't think this way of doing things is any better (nor worse, though) than a utility class, is there a reason to avoid those? There are a few: * An IFooSet is a poorly defined object, that doesn't do a very good job of representing any actual thing in the system That's not entirely correct - it's used to represent top-level collections in the API. * It is actually more work and more boilerplate to have a second class implement IFooSet (type it out!) From what I've seen it's marginal. * This extra work does not buy any meaningful separation of concerns. It's very normal to have code that creates an object together with the code that defines the behaviour of the object. That I agree with :) ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Monday 26 July 2010 16:24:33 Aaron Bentley wrote: On 07/26/2010 11:10 AM, Julian Edwards wrote: On Monday 26 July 2010 16:01:31 Jonathan Lange wrote: I guess I'm struggling to see how @classmethod is useful then. In fact, it's dangerous. Why do you think it's dangerous? Because it allows someone to get hold of a new object that is not security wrapped. So does FooSet.new. What's the difference? None - that's exactly what I was referring to. Either one can be registered as a utility, and calling it as a utility looks exactly the same. I don't know why you'd ever want to do that. It's very convenient in test code. Why? Why do you need to ignore security? If it is *really* needed, I would *much* rather see an explicit removeSecurityProxy() with a comment explaining why you need to remove the wrapper. It should be a conscious exception, not a trap you can fall into. * This extra work does not buy any meaningful separation of concerns. It's very normal to have code that creates an object together with the code that defines the behaviour of the object. That I agree with :) Another point is that having an IFooSet encourages people to hang a deleteFoo method from it, and this is an antipattern, because it prevents normal zope security checks from being available. (You can only restrict permission on self, not other parameters to methods.) def Foo.destroySelf(self) - We can check whether the user has permission to delete this object at the zope level, because self is the object def FooSet.destroyFoo(self, foo) - We can't check whether the user has permission to delete this object at the zope level, because it's foo, not self. I agree with all of this, except that I can't say I've ever been encouraged to have a delete method on a utility :) But still, we can do store.remove() anywhere and that's not checked. J ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/26/2010 12:13 PM, Julian Edwards wrote: On Monday 26 July 2010 16:24:33 Aaron Bentley wrote: On 07/26/2010 11:10 AM, Julian Edwards wrote: On Monday 26 July 2010 16:01:31 Jonathan Lange wrote: I guess I'm struggling to see how @classmethod is useful then. In fact, it's dangerous. Why do you think it's dangerous? Because it allows someone to get hold of a new object that is not security wrapped. So does FooSet.new. What's the difference? None - that's exactly what I was referring to. So this means that FooSet.new is also dangerous. So why do you say that a @classmethod is dangerous, if it's no more dangerous than a normal method? I don't know why you'd ever want to do that. It's very convenient in test code. Why? Why do you need to ignore security? Because I want to set up preconditions for my test that are much easier to do by poking at internal state than by using the approved Interface. Because it's sometimes easier or more precise to verify postconditions by looking at internal state than by using the approved Interface. If it is *really* needed, I would *much* rather see an explicit removeSecurityProxy() with a comment explaining why you need to remove the wrapper. Sure, explicitly removing the security proxy is clearer. I don't agree about the need for a comment. def FooSet.destroyFoo(self, foo) - We can't check whether the user has permission to delete this object at the zope level, because it's foo, not self. I agree with all of this, except that I can't say I've ever been encouraged to have a delete method on a utility :) Having seen a delete method on a utility, I can only assume some people have been. But still, we can do store.remove() anywhere and that's not checked. Indeed. It's not about making evil impossible, it's about helping ourselves to avoid mistakes. Aaron -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkxNt5EACgkQ0F+nu1YWqI2xnQCaAw1RWqkqrV6TIFQvHSKaA6UB vYsAoInMN+PF5PfFSCocH8TIF6yaDUys =d31n -END PGP SIGNATURE- ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On 26.07.2010 17:03, Jonathan Lange wrote: On Mon, Jul 26, 2010 at 11:15 AM, Abel Deuring abel.deur...@canonical.com wrote: On 26.07.2010 12:06, Robert Collins wrote: On Mon, Jul 26, 2010 at 11:44 AM, Julian Edwards julian.edwa...@canonical.com wrote: On Monday 26 July 2010 10:29:56 Robert Collins wrote: Lastly, and here I expose my ignorance of some subtleties in zope - I thought security proxies only lived between view and model objects, not between model objects? That's right. Once the code inside a proxied object is running, it's effectively security-free and can see objects that the code outside of it would not normally be able to access. We need to be careful about this, because there's no protection against returning data to the caller that it should not see. So I don't understand this overall change then. If we're testing view code, we want something like: Proxy - model1 - model2 etc If we're testing model code, given that model code is unproxied as it interacts with other model code, we want model1 - model2 Only view code can depend on security proxies for permission checking, so making all our tests have security proxies *does not fit* our deployed object structure, and can easily fail by having a false sense of security. What about this: * Write a decorator factory that wraps *anything* it is asked for in a proxy, except one attribute 'unwrapped_factory' (which is the thing it is decorating). * Make the view tests get a decorated launchpad factory * Leave unit tests alone. If we don't work with proxied objects in the unit tests, we may miss permission problems, unless the view tests cover each code path... I used to agree, but now I'm not so sure. Can you give an example of the kind of permission problem we might miss, or of one that we've caught because we were using security proxies in our model tests? I can't give any good example. My reason simply is this: if we get an Unauthorized exception while iterating over the result of getUtility(IFooSet).getStuff(), we know that we should either fix getStuff() or use/write a method getStuffForUser(some_person). ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Monday 26 July 2010 17:28:01 Aaron Bentley wrote: On 07/26/2010 12:13 PM, Julian Edwards wrote: On Monday 26 July 2010 16:24:33 Aaron Bentley wrote: On 07/26/2010 11:10 AM, Julian Edwards wrote: On Monday 26 July 2010 16:01:31 Jonathan Lange wrote: I guess I'm struggling to see how @classmethod is useful then. In fact, it's dangerous. Why do you think it's dangerous? Because it allows someone to get hold of a new object that is not security wrapped. So does FooSet.new. What's the difference? None - that's exactly what I was referring to. So this means that FooSet.new is also dangerous. So why do you say that a @classmethod is dangerous, if it's no more dangerous than a normal method? Argh, sorry I misread your last email. If there's no @classmethod on new() then you need an instantiated object. At that point, it's more likely that you've DTRT regarding securedutility rather than circumventing the proxy by calling the classmethod. So, if I understand this stuff right, you simply don't need the @classmethod if you call getUtility(IFoo).new() I don't know why you'd ever want to do that. It's very convenient in test code. Why? Why do you need to ignore security? Because I want to set up preconditions for my test that are much easier to do by poking at internal state than by using the approved Interface. Because it's sometimes easier or more precise to verify postconditions by looking at internal state than by using the approved Interface. If it is *really* needed, I would *much* rather see an explicit removeSecurityProxy() with a comment explaining why you need to remove the wrapper. Sure, explicitly removing the security proxy is clearer. I don't agree about the need for a comment. Maybe I am jaded by bad Soyuz code, but I've seen removeSecurityProxy used all over the place because someone didn't understand the security. I therefore feel aggrieved when I see one, and demand a comment to explain it. :) def FooSet.destroyFoo(self, foo) - We can't check whether the user has permission to delete this object at the zope level, because it's foo, not self. I agree with all of this, except that I can't say I've ever been encouraged to have a delete method on a utility :) Having seen a delete method on a utility, I can only assume some people have been. That's, umm, interesting. In a bad way. :( But still, we can do store.remove() anywhere and that's not checked. Indeed. It's not about making evil impossible, it's about helping ourselves to avoid mistakes. Totally. That's why I'm advocating getting rid of @classmethod on that new() :) ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
On Mon, Jul 26, 2010 at 12:13 PM, Julian Edwards julian.edwa...@canonical.com wrote: If it is *really* needed, I would *much* rather see an explicit removeSecurityProxy() with a comment explaining why you need to remove the wrapper. It should be a conscious exception, not a trap you can fall into. +1 I've fallen into that trap myself. As a result, if I have to remove a security proxy (in non-test code) I ask myself if the operation I'm about to do is one the user shouldn't be able to do of their own accord (otherwise it shouldn't be restricted by the security proxy in the first place) and I'm removing the security proxy because the system needs to perform some action that the user himself isn't allowed to do. Another rule of thumb I follow is that if I remove a security proxy I try not to bind the naked object to a name but instead perform the operation in the same expression as the call to removeSecurityProxy. That way I don't introduce any unintentional un-proxied operations later. If that's not possible I'll explicitly del the name binding as soon as I'm done with it (with copious comments to explain what's going on). -- Benji York ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-dev] warning: we will soon have much noise in the test results...
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/26/2010 12:42 PM, Julian Edwards wrote: On Monday 26 July 2010 17:28:01 Aaron Bentley wrote: If there's no @classmethod on new() then you need an instantiated object. At that point, it's more likely that you've DTRT regarding securedutility rather than circumventing the proxy by calling the classmethod. Okay, point taken. Though the scope where you can import Foo in order to call Foo.new is already pretty restricted due to the importfascist. So, if I understand this stuff right, you simply don't need the @classmethod if you call getUtility(IFoo).new() The @classmethod means that you can register the class Foo, not instances of the class Foo, as a utility, which means that you don't need an instance of Foo in order to call Foo.new. If you don't have an @classmethod on Foo, then you need an instance of Foo in order to call Foo.new on it, and that's crazy. So no classmethod means you need to hang new() off a FooSet class instead of Foo. Personally, I've always thought it was strange that we have to instantiate these FooSet objects, since they don't have any state. If we keep on using them, I'd recommend using @classmethod/staticmethod on them :-) Aaron -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkxNvowACgkQ0F+nu1YWqI2idgCfc0999kOuJyzqSPk9/FKqhHJL /O0An1HZI0xcSPIawQHdd1HhdnQPexS+ =GhRF -END PGP SIGNATURE- ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
[Launchpad-dev] warning: we will soon have much noise in the test results...
The branch lp:~adeuring/launchpad/security-guarded-test-object-factory-1 is at present in ec2 and will land soon. Its main change affects LaunchpadObjectFactory: This class has at present ca 20 makeWhatever() methods which return objects without a security proxy. If this happens, LaunchpadObjectFactory will now print a warning. I intend to land further branches where the affected methods will changed so that they return security proxied objects. This will in turn cause a larger number of test failures. As a simple fix/workaround, I added a function remove_security_proxy_and_shout_at_engineer(obj) which just returns removeSecurityProxy(obj) but it also prints a warning to stderr. Properly fixing all tests would take far too much time -- and after all, the test behaviour will be the same as before. The only difference is that we will clearly see where our tests work with bad objects. sorry for any inconvenience... Abel ___ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
