[00:55] <thumper> lifeless: sure
[01:14] <lifeless> thanks
[01:34] <thumper> lifeless: done
[01:38] <lifeless> thanks
[02:47] <jtv> thumper: while you're at it, can you review this one for me?  https://code.launchpad.net/~jtv/launchpad/recife-policy-mixin/+merge/40300
[02:48] <jtv> (that was deliberately phrased for a flippant rebuttal, so feel free if it's not convenient)
[02:48] <thumper> jtv: if it was small I'd say yes, but I'm in the middle of something.  Sorry.
[02:49] <jtv> no worries
[02:49]  * jtv scans for victims
[02:49] <jtv> bac!
[02:49] <jtv> Are you up for a non-small review?
[02:49] <bac> jtv i can be.  can it wait a little bit as i just started something?
[02:50] <jtv> bac: yes it can
[02:51] <lifeless> ahhh the sweet smell of real urls in the morning
[02:51] <lifeless> it smells like victory
[02:58]  * jtv finds this obvious crack in lifeless' sanity a good reason to ask:
[02:59] <jtv> you're suggesting a standalone class to implement translation policy.  Were you thinking of a schema change?
[02:59] <lifeless> jtv: I wasn't, but perhaps one is needed.
[03:00] <lifeless> s/needed/possibly beneficial.
[03:00] <jtv> I see it making a lot of sense at this point.
[03:00] <lifeless> we have -way- to many mixins
[03:00] <lifeless> when you've got 1 mixin its a problem. 20+ mixings is arggggh zomg mode
[03:01] <jtv> Yes, we do have too many of them.
[03:02] <jtv> And in this case, there's actual state that's being replicated into multiple classes—basically a denormalized object design.
[03:02] <lifeless> is there a good reason for that ?
[03:05] <jtv> Best I can come up with is "progressing insight."
[03:05] <jtv> In other words, "no but that may not have been so obvious at the time."
[03:05] <jtv> There's 2 identical fields in Product, ProjectGroup, and Distribution.
[03:06] <jtv> I'm trying to expose and unify the common patterns.
[03:08] <jtv> Extracting it into a separate database table could be a lot of work, of course.
[03:08] <jtv> Some joins will change shape.
[03:09] <jtv> The new objects' lifetimes have to be managed etc.
[03:10] <jtv> Too much work for now.  I think I'll go with a standalone non-db class first.
[03:24] <jtv> lifeless: I'm looking at implementing this as an adapter.
[03:25] <jtv> lifeless: so you'd say "ITranslationPolicy(pillar).makeSomeDecision()"
[03:25] <jtv> but you can also adapt certain other objects to get the ITranslationPolicy that applies to them.
[03:26] <jtv> Does that make sense to you?
[03:28] <lifeless> that seems a decent start
[03:29] <lifeless> as for adapter vs pillar.translation_policy - shrug, its largely just spelling (except those pesky lifetimes that you mentioned)
[03:36] <jtv> Yes, and I don't just want to keep adding to the interfaces.
[03:50] <jtv> bac, lifeless: reworking the policy object so that it's not a mixin becomes too much work for this branch; I'll have leave that for a separate branch.
[03:55]  * jtv goes out for a bite
[08:26] <stub> lifeless: Should https://code.launchpad.net/~stub/launchpad/update-storm/+merge/40264 go through without review? It consists entirely of unicode casts, updates to modern Storm syntax and delints at 2.4k lines of diff.
[09:27] <bac> jtv: return the favor?
[09:28] <jtv> bac: honour compels me.
[09:28] <bac> jtv: lucky for you i'm not as prolific.  a measly 88 line diff.  https://code.launchpad.net/~bac/launchpad/bug-671539/+merge/40309
[09:28] <jtv> bac: prolific?  I have more changes waiting that I carefully cut out of the diff to keep its size down.  :-)
[09:31] <jtv> bac: I find the "if not person.is_team: enabled = True else: enabled = False" a bit hard to read.  Have you considered this alternative?
[09:31] <jtv> owner_is_individual = (not person.is_team)
[09:31] <jtv> …(…, enabled=owner_is_invidual)
[09:32] <bac> jtv: yeah, i'll make that change.
[09:32] <jtv> Thanks.
[09:32] <bac> brain fart
[10:21] <bac> any other comments, jtv?
[10:21] <jtv> bac: whoops, got sidetracked!  Getting back to it now.
[10:22] <jtv> bac: Shouldn't the section heading in the story test be "---"-underlined instead of "[10:22] <jtv> (I do not like rest-style headings)
[10:27] <jtv> bac: approved.
[10:36] <bac> jtv: i'll look at the formatting.  i thought i had followed the style used in the file but perhaps i messed up
[10:36] <jtv> bac: or the previous author of the file got it wrong, or we just have a harmless inconsistency.
[12:50] <jtv> Hey, who's up for a review that I actually tried to make fun?  https://code.launchpad.net/~jtv/launchpad/independence-for-ihastranslationtemplates/+merge/40321
[12:51] <jtv> matsubara maybe?
[12:53] <jtv> gmb then?  you're always in for a laugh, no?
[12:54] <matsubara> jtv, I'm not an official reviewer
[12:54] <jtv> ah!
[12:54] <jtv> well good morning, anyway.  :)
[12:57] <matsubara> good morning :-)
[13:00] <jtv> Still holding that T-shirt.  :)
[13:03] <jtv> henninge, do you want to review it then?
[13:04] <henninge> jtv: oh, yes I should ...
[13:04] <jtv> Since it's your day and all :)
[13:04] <jtv> Thanks.
[13:04] <henninge> Yes, I will have to find a replacement for next week.
[13:04] <jtv> We're not usually that picky about missing a review day.
[13:04] <henninge> I don't think I should be doing normal reviews *and* r-c reviews...
[13:05] <jtv> At least not on this side of the world, where usually there aren't any OCRs anyway.  :)
[13:05] <jtv> henninge: why do you say normal reviews and r-c reviews?  has there been a change that I'm unaware of?
[13:06] <henninge> jtv: I am release manager so I will be the one doing r-c reviews next Monday.
[13:06] <jtv> ah
[13:06] <jtv> I forgot that you were rc
[13:06] <henninge> rm
[13:06] <jtv> sorry, rm
[13:06] <jtv> rm -rf
[13:07] <henninge> In my world: Returned Missionary ;-)
[13:07] <jtv> (Seriously, I once saw Siemens advertise their OS called, if I remember the spelling correctly, RM/UNIX)
[13:07] <henninge> :-D
[13:13] <henninge> Those Germans...
[14:11] <allenap> henninge: Fancy taking on another branch?
[14:15] <allenap> henninge: I've put myself in the queue, but feel free to eject me.
[14:19] <henninge> jtv, allenap: Sorry, lunch got in the way. I will relocate now and look at your branches after that.
[15:51] <henninge> jtv: r=me Thank you.
[16:03] <allenap> henninge: Thanks!
[16:03] <henninge> allenap: I have not yet used ARRAY's
[16:03] <henninge> s/'//
[16:06] <allenap> henninge: Dang, I misread and thought that r=me was for me :) Thank you anyway!
[16:06] <henninge> allenap: oh, I am sorry.
[16:06] <jtv> thanks henninge
[16:07] <allenap> henninge: Hehe, don't be sorry, it was my fault.
[16:34] <abentley> henninge: is [jtv] already reviewed?
[16:34] <jtv> abentley: yes I am
[16:34] <henninge> ups, chansubjupd error
[16:35] <henninge> ups=oops(de_DE)
[16:35] <abentley> henninge: We should have a web page we can use to set the channel topic :-)
[17:04] <jelmer> abentley: Hi
[17:04] <jelmer> abentley: Can I add a MP to your queue?
[17:04] <abentley> jelmer: sure.
[17:05] <jelmer> abentley: https://code.launchpad.net/~jelmer/launchpad/672476/+merge/40349
[17:31] <allenap> henninge: I have to go soon, so ask soon if you have any questions about my branch :)
[17:32] <henninge> allenap: sorry, I was about to write the review. I actually understand what's going on now ... ;)
[17:32] <henninge> allenap: I don't have questions, though. You can look at it in the morning. Nothing serious just some suggestions.
[17:35] <allenap> henninge: Awesome, thank you.
[18:20] <abentley> jelmer: 59: s/iterabel/iterable
[18:21] <abentley> jelmer: Personally, I find using "with" with files more elegant than try/finally.
[18:23] <abentley> jelmer: Why do you test whether htpassword exists (and is a file) and delete it, instead of just overwriting it?
[18:25] <abentley> jelmer: is it secure to use token.person.name[:2] as a salt?
[18:25] <jelmer> abentley: Thanks, fixed.
[18:26] <jelmer> abentley: I usually don't bother with "with" out of fear of missing a "from __future__ import with_statment" somewhere
[18:26] <abentley> jelmer: we're all-2.6 now.  You shouldn't import with_statement anymore.
[18:27] <jelmer> abentley: Overwriting it would work too I guess. That, and the token.person.name[:2] are simply the same as they were in the generate_ppa_htaccess.py script.
[18:28] <abentley> jelmer: yeah, I realize this is mostly just reorganizing old code.
[18:29] <abentley> jelmer: But using token.person.name[:2] as a salt is security-by-obscurity, and now that we're open-source, it's no longer obscure.
[18:32] <jelmer> abentley: it'd break a few existing tests though, so I wonder if I should leave that for a separate branch and file a high-priority bug about it for now.
[18:33] <jelmer> abentley: What do you think?
[18:33] <james_w> salts can be known
[18:34] <abentley> james_w: Oh?  I would have thought that would make it trivial to forge the password.
[18:34] <james_w> abentley, no. Assuming you are talking about a real salt, and not something wrongly called that, they are not keys
[18:34] <james_w> if they were then they wouldn't be separate
[18:35] <abentley> james_w: we're talking about salts as used by the crypt C function.
[18:35] <james_w> what they do is prevent exploiting all passwords at once. If there were no salts you could generate one rainbow table and then decrypt everyone's ciphertext at once
[18:36] <james_w> adding a salt means that you have to attack each ciphertext in turn
[18:36] <james_w> so they do no need to be secret, but they do benefit from being well-distributed
[18:36] <abentley> james_w: okay, thanks for that clarification.
[18:37]  * jelmer didn't know that either
[18:37] <james_w> (if num salts << num ciphertexts then you can decrypt a lot of ciphertexts with on rainbow table)
[18:37] <james_w> maybe a comment in the source?
[18:37] <abentley> james_w: and happy belated birthday.
[18:37] <james_w> thanks :-)
[18:38] <james_w> http://en.wikipedia.org/wiki/Salt_(cryptography)
[18:39] <james_w> it does say "For best security, the salt value is kept secret.", but if your system relies on the salt being secret, then the keys are probably too short anyway
[18:41] <abentley> james_w: it seems like in our context, all the passwords are equivalent, so only one cyphertext is needed.  Therefore using distinct salts doesn't help?
[18:42] <james_w> abentley, where can I look to see the use of this in the source?
[18:42] <james_w> as I'm not sure what you mean by "all the passwords are equivalent"
[18:42] <abentley> james_w: https://code.launchpad.net/~jelmer/launchpad/672476/+merge/40349
[18:43] <james_w> EACCESS
[18:43] <abentley> james_w: by "all the passwords are equivalent", I mean they all grant equivalent access AIUI
[18:44] <james_w> ah
[18:44] <james_w> so impersonation isn't really a problem
[18:44] <james_w> except for users who aren't supposed to have access at all
[18:44] <abentley> james_w: jelmer is refactoring this code: lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py
[18:46] <james_w> abentley, yeah, I'd agree, in this case salts aren't particularly useful for their normal use like in unix passwd
[18:47] <james_w> it adds a little benefit in that you wouldn't have to revoke every access if one token was compromised, but you may want to do that anyway to be sure
[18:48] <james_w> in this case though the main requirement is that crypt requires a salt, and so I think that the value used is as good as any other
[18:49] <abentley> james_w: Right.  Hard to imagine only one token being compromised in this scenario.
[18:49] <abentley> james_w: okay.
[19:13] <abentley> jelmer: should _setupHtaccess consider the case where htaccess is different from what it would write?
[19:13] <jelmer> abentley: No, it sufficient that there is *some* htaccess there.
[19:14] <jelmer> abentley: generate_ppa_htaccess should take care of updating it
[19:15] <abentley> jelmer: ISTM that until generate_ppa_htaccess runs, the contents could be exposed.
[19:16] <abentley> jelmer: because the .htaccess might not impose any content restrictions.
[19:16] <jelmer> abentley: We only write out one kind of htaccess file at the moment
[19:17] <jelmer> abentley: I guess it might make sense take into account that we might want something different in that file in the future.
[19:18] <abentley> jelmer: so _setupHtaccess assumes that htaccess will not be different from what it would write.
[19:21] <jelmer> abentley: Sorry, yes. It assumes the .htaccess file would be the same and that there is some .htpasswd
[19:23] <abentley> jelmer: If an earlier process had died causing a partial write, that would not be true, but I think that's rare enough not to care about.
[19:26] <abentley> jelmer: I guess I'm wondering whether skipping writing htaccess and htpasswd is worthwhile.
[19:27] <jelmer> abentley: That's a good question, I've wondered about that as well.
[19:27] <abentley> jelmer: it seems like it doesn't save an appreciable amount of IO, and it does introduce a corner case.
[19:27] <jelmer> abentley: It saves us a single database query and a certain amount of IO.
[19:28] <jelmer> abentley: I don't know what the maximum number of subscriptions for a PPA is.
[19:29] <abentley> jelmer: At 243, I believe temp_filename is supposed to be on a new line.
[19:31] <abentley> jelmer: similarly at 351.
[19:34] <jelmer> abentley: Fixed
[19:35] <abentley> jelmer: Your tests would be a bit simpler if you returned the contents of .htaccess and .htpasswd as strings or if your functions took a file parameter.
[19:36] <abentley> jelmer: You could also just use useTempDir in your tests.
[19:39] <abentley> jelmer: You can use dedent to write your multi-line strings more neatly at 451.
[19:43] <jelmer> abentley: I haven't used dedent before, what is it?
[19:44] <jelmer> abentley: is this textwrap.dedent?
[19:44] <abentley> jelmer: Yes.  It strips leading indents.
[19:48] <abentley> jelmer: Anyhow, those are just some ideas.  r=me.
[19:50] <jelmer> abentley: Thanks, that was quite educational.
[19:51] <abentley> jelmer: np.
[20:12] <jelmer> abentley: I just realized that if we always write out the htaccess file we need to honor the blacklist that currently exists in generate-ppa-htaccess.py
[20:13] <abentley> jelmer: I see.  It's your call.