| === mwhudson_ is now known as mwhudson | ||
| === rockstar` is now known as rockstar | ||
| === bac|away is now known as bac | ||
| === bac is now known as bac|away | ||
| === bac|away is now known as bac | ||
| === bac is now known as bac|away | ||
| === bac is now known as bac|away | ||
| === bac|away is now known as bac | ||
| === EdwinGrub is now known as EdwinGrubbs | ||
| === bigjools_ is now known as bigjools | ||
| intellectronica | me | 15:00 |
|---|---|---|
| barry | #startmeeting | 15:00 |
| MootBot | Meeting started at 09:02. The chair is barry. | 15:00 |
| MootBot | Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] | 15:00 |
| barry | welcome everyone to this week's ameu reviewer's meeting. who's here today? | 15:01 |
| bigjools | me | 15:01 |
| sinzui | me | 15:01 |
| BjornT | me | 15:01 |
| gmb | me | 15:01 |
| allenap | me | 15:01 |
| bac | me | 15:01 |
| EdwinGrubbs | me | 15:01 |
| schwuk | me | 15:01 |
| flacoste | me | 15:02 |
| flacoste | salgado is on holiday | 15:02 |
| barry | cprov: ping | 15:02 |
| cprov | me | 15:03 |
| barry | [TOPIC] agenda | 15:04 |
| MootBot | New Topic: agenda | 15:04 |
| barry | == Agenda == | 15:04 |
| barry | * Roll call | 15:04 |
| barry | * Next meeting | 15:04 |
| barry | * Action items | 15:04 |
| barry | * Queue status | 15:04 |
| barry | * Mentoring update | 15:04 |
| barry | * Review process | 15:04 |
| barry | * Ensure in database code that non-DB state is reset at transaction | 15:04 |
| barry | boundaries (flacoste) | 15:04 |
| barry | * Security checks (should usually be avoided) in view code (intellectronica) | 15:04 |
| barry | [TOPIC] * Next meeting | 15:04 |
| MootBot | New Topic: * Next meeting | 15:04 |
| barry | same time and place next week? anybody know they will not be here? | 15:04 |
| bigjools | cprov and I are sprinting, so may or may not make it | 15:05 |
| intellectronica | i may be busy with the ubuntu sprint. don't know yet, but will update when i know the schedule for sure | 15:05 |
| abentley | That would be fine with me. | 15:05 |
| barry | cool. if you're going to be away, please update the wiki page w/ apologies | 15:05 |
| cprov | bigjools: we will have 30 minutes for the meeting, I'm sure. | 15:06 |
| barry | [TOPIC] * Action items | 15:06 |
| MootBot | New Topic: * Action items | 15:06 |
| bigjools | cprov: almost certainly, but I am warning just in case! | 15:06 |
| barry | * barry to ask lifeless to summarize what he knows about the PQM Mysteries (e.g. autopacking bug losing branches) | 15:06 |
| barry | done | 15:06 |
| barry | * barry to ask losas and devs to update LaunchpadProductionDocumentation on operation details of machines for devs | 15:07 |
| barry | done | 15:07 |
| barry | the other action items are for asiapacsters | 15:07 |
| barry | [TOPIC] * Queue status | 15:07 |
| MootBot | New Topic: * Queue status | 15:07 |
| barry | everything looks fine to me. any comments from y'all? | 15:08 |
| sinzui | schwuk: intellectronica: bugtarget-hasbugs-1 is mege-conditional isn't it? | 15:08 |
| schwuk | sinzui: that's how it was left. | 15:08 |
| barry | leonardr: i'll note that pending-reviews has 55 conflicts (!) on your integration branch | 15:08 |
| intellectronica | sinzui: it is, but i'm only going to land it when another branch is prepared. that's why i haven't removed it yet | 15:08 |
| sinzui | someone, (schwuk) should update PR | 15:09 |
| schwuk | sinzui: done | 15:10 |
| barry | thanks | 15:10 |
| bac | during my on-call shift i've started reminding people by name to clean up PendingReviews. seems effective and well received. perhaps others can do the same. | 15:10 |
| barry | bac: +1 | 15:10 |
| barry | [TOPIC] * Mentoring update | 15:11 |
| MootBot | New Topic: * Mentoring update | 15:11 |
| barry | nothing from me here. any comments? | 15:11 |
| barry | [TOPIC] * Review process | 15:12 |
| MootBot | New Topic: * Review process | 15:12 |
| barry | * Ensure in database code that non-DB state is reset at transaction | 15:13 |
| barry | boundaries (flacoste) | 15:13 |
| barry | flacoste: the floor is yours | 15:13 |
| flacoste | ok | 15:13 |
| flacoste | so our move to storm changed a subtle semantic in the ORM | 15:13 |
| flacoste | Storm does reuse instances across transactions | 15:13 |
| flacoste | so a .get() after a transaction.commit() or abort() will return the same instance than previously | 15:14 |
| flacoste | that wasn't the case with SQLObject | 15:14 |
| flacoste | the instance is marked in a special way though so that all DB attributes will be fetched again | 15:14 |
| flacoste | but non-DB attributes will hold the values from the last transaction | 15:14 |
| flacoste | so be mindful of any non-DB attributes used on database objects | 15:15 |
| flacoste | cachedproperty is an example of that | 15:15 |
| flacoste | but there are other examples | 15:15 |
| flacoste | objects that use non-DB attributes should provide a __storm_invalidate__ method | 15:15 |
| flacoste | to reset these attributes | 15:16 |
| flacoste | (it's called by storm on transaction boundaries) | 15:16 |
| barry | flacoste: has cachedproperty been "fixed" to clear its cache on txn boundaries? | 15:16 |
| flacoste | barry: no | 15:16 |
| flacoste | i think i filed a bug about that, but i have to check | 15:16 |
| flacoste | but iirc | 15:16 |
| flacoste | we never used cachedproperty on DB object | 15:16 |
| allenap | Does Storm do this as a performance optimisation? | 15:16 |
| flacoste | yes | 15:17 |
| barry | flacoste: thanks | 15:17 |
| flacoste | and also as a convenience to the developer | 15:17 |
| BjornT | flacoste: we do use cachedproperty on db objects | 15:17 |
| flacoste | we do | 15:17 |
| flacoste | ok | 15:17 |
| barry | find lib/canonical/launchpad/database -name \*.py | xargs grep cachedproperty | wc -l | 15:17 |
| barry | 57 | 15:17 |
| flacoste | wow | 15:17 |
| flacoste | ok, i'll make sure to report and fix cachedproperty then | 15:17 |
| flacoste | and maybe offer a wrapper for storm to easily create transaction-aware attribuets | 15:18 |
| barry | [ACTION] flacoste to submit bug report on cachedproperty needing to __storm_invalidate__ | 15:18 |
| MootBot | ACTION received: flacoste to submit bug report on cachedproperty needing to __storm_invalidate__ | 15:18 |
| flacoste | i'll also update the checklist with that info | 15:18 |
| barry | flacoste: excellent, thanks | 15:18 |
| flacoste | any questions? comments? | 15:18 |
| barry | * Security checks (should usually be avoided) in view code (intellectronica) | 15:19 |
| barry | intellectronica: the floor is yours | 15:20 |
| intellectronica | when we spot security checks in view code, it's a good idea to ask whether they really belong there | 15:20 |
| flacoste | intellectronica: what kind of security checks are you talking about? | 15:21 |
| flacoste | check_permission call? | 15:21 |
| intellectronica | in most cases, it would be code to have them condified in the content object, and only called from the view | 15:21 |
| flacoste | or security logic? | 15:21 |
| intellectronica | flacoste: yes - stuff that relies on check_permission, and especially stuff the does more logic than that, for example by combining several calls to check_permission | 15:21 |
| intellectronica | or a call to check_permission with some other stuff | 15:22 |
| intellectronica | there are several reasons why i think this is important: | 15:22 |
| flacoste | i can see several good reasons to call check_permission() in view code (to inform users or other forms of conditional action/display) | 15:22 |
| abentley | An example of where I think it makes sense to check permission in view code is on the branch deletion page, where we show users what needs to be changed, and whether they can do it. | 15:22 |
| bigjools | check_permission is already done in lots of browser code | 15:22 |
| intellectronica | flacoste: true, but if the security check gets complicated, it would be good to have it in the content object and call it from there | 15:23 |
| intellectronica | it's easier to test and easier to maintain when the predicate changes | 15:23 |
| barry | intellectronica: maybe instead of adding these to content objects, we should register securitycheck adapters for them, and use that wherever it's necessary (e.g. browser code)? | 15:24 |
| intellectronica | and most importantly, if we need to expose it from a different layer (the api, or a script, or whatever), we can reuse it | 15:24 |
| intellectronica | barry: in some cases that's appropriate, in others less | 15:24 |
| barry | i like keeping database code free from security checks because in some of my cases, what's appropriate in the browser isn't appropriate (or different) in the xmlrpc layer or web services layer | 15:25 |
| intellectronica | i don't think that there's a rule that can be formulated. just that since i had to deal with this recently it opened my eyes and all of a sudden i see lots of places where this could become a problem | 15:25 |
| intellectronica | barry: can you give an example? | 15:25 |
| intellectronica | i'm asking, because i think that this it's quite rare for this to happen | 15:26 |
| barry | intellectronica: i'd have to think of a specific example, sorry | 15:27 |
| * barry could be misremembering too | 15:27 | |
| barry | intellectronica: could you outline the specific case where this came up? | 15:27 |
| bigjools | don't the bug scripts already cope with lack of a user in zopeless mode? | 15:28 |
| intellectronica | the case i've been dealing with is a check on setting various attributes of IBugTask (importance, for example), which was restricted using the browser | 15:28 |
| allenap | So all the restrictions were worthless when you exposed IBugTask.importance in the API | 15:29 |
| intellectronica | after moving this check to the content object (so that i can expose this object using the API safely) i discovered lots of places where we relied on ignoring the security check | 15:29 |
| intellectronica | i'm sure that there are other cases, where restrictions hold in the browser code, but are being ignored elsewhere, and we just don't realise | 15:30 |
| intellectronica | that's the main reason i think this is important | 15:30 |
| bigjools | maybe we need another layer of code? I'm not convinced that polluting content classes is right. | 15:31 |
| BjornT | intellectronica: iirc, that was a bit different, though. the check was only done in browser code, and you needed to do the same check in the security checker, and from within the content class. | 15:31 |
| intellectronica | in addition, it's much easier to check a security constraint or a method on a content object independently, than check through the view code | 15:31 |
| barry | bigjools: hence my suggestion of adapters | 15:31 |
| intellectronica | BjornT: indeed, but there are many cases like this, where we compound several predicates for the security check | 15:31 |
| flacoste | ok, this is very particular | 15:32 |
| bigjools | intellectronica: what places rely on ignoring the security check? | 15:32 |
| intellectronica | BjornT: and i think this should be encapsulated somehow. in a security checker or if necessary, in the content class | 15:32 |
| flacoste | and i agree with intellectronica that it is misplaced | 15:32 |
| BjornT | right. but if you don't need to re-use the check from within the content class, you don't win much by adding an extra method. | 15:32 |
| flacoste | but it's because the view is handling the mutation logic | 15:32 |
| flacoste | that's not the place for that | 15:32 |
| intellectronica | bigjools: i only know about the once pertaining to this change, and there are heaps of them. many scripts, for example | 15:33 |
| bigjools | are the scripts able to log in as the right user? | 15:33 |
| barry | do we usually have "a view" for whatever channel we're accessing/mutating the object through? (i.e. xmlrpc, api, browser)? if so, i wonder if the publishers could be hooked to provide that security checker adapter automatically | 15:34 |
| cprov | bigjools: login(xxx) | 15:34 |
| bigjools | cprov: exactly | 15:34 |
| bigjools | so if we define the zope security adapter properly, is this moot? | 15:35 |
| cprov | bigjools: as a predefined celebrity, though, not the right user in most of the cases | 15:35 |
| flacoste | another difference | 15:35 |
| BjornT | barry: well, we have the security policies | 15:35 |
| flacoste | is that scripts usually use the PermissiveSecurityPolicy | 15:35 |
| flacoste | which bypass all security checks | 15:35 |
| bigjools | ew! | 15:36 |
| barry | so. intellectronica, what's the plan? how do we resolve this? | 15:37 |
| intellectronica | as i said, i don't think there's a rule hiding here, just something to pay attention to | 15:38 |
| intellectronica | if you see check_permission(this) and check_permission(that) in browser code, ask whether this belongs elsewhere, for example | 15:38 |
| barry | intellectronica: okay, but i think we need guidelines in the wiki so devs and reviewers can answer this question | 15:39 |
| intellectronica | i can try to come up with something, but obviously it will need to be reviewed by more people | 15:40 |
| intellectronica | i can add that and ask for comments | 15:40 |
| barry | intellectronica: definitely... and thanks! | 15:40 |
| barry | [ACTION] intellectronica to write up guidelines on check_permission in the wiki and email the ml for additional input | 15:41 |
| MootBot | ACTION received: intellectronica to write up guidelines on check_permission in the wiki and email the ml for additional input | 15:41 |
| barry | anything else on this, or any other topic? we have 4 minutes left | 15:41 |
| barry | okay, thanks everybody | 15:42 |
| barry | #endmeeting | 15:42 |
| MootBot | Meeting finished at 09:43. | 15:42 |
| bigjools | cheers all | 15:42 |
| abentley | tx | 15:42 |
| === bac is now known as bac|away | ||
| === bac|away is now known as bac | ||
| === Moot2 is now known as MootBot | ||
| === bac is now known as bac|away | ||
| === bac|away is now known as bac | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!