Beefy Boxes and Bandwidth Generously Provided by pair Networks
good chemistry is complicated,
and a little bit messy -LW

discussion of Tailors group proposal

by jdporter (Chancellor)
on Dec 22, 2010 at 04:15 UTC ( #878423=scratchpad: print w/replies, xml ) Need Help??

Quoth Petruchio:

Temporary appointments of trustworthy people to lead pmdev seems like a good idea.

Here's an idea. Say we put a button on each group's node. You click the button, you renew your membership. After, say, a year with no clicking, you're removed (with certain exceptions; e.g. vroom, NodeReaper). Combined with a policy of re-instating pretty much anyone who asks to be re-instated, it seems like a 90% solution with almost no probability of strife.

I have been added gods for one month. It is an appointment not for policy decisions, but development work. My marching orders / rules of engagement:

  • document each thing (in the gw) before I do it
  • avoid the SQL prompt
  • work within the patch system

Among my tasks are:

  • to create a Tailors group, to allow select individuals to apply patches
  • to implement the expiration mechanism (described above) for usergroups.

I raised the subject in a thread not too long ago: RFC: Create a PatchAppliers group
An earlier thread on the same general topic: pmdev: patches to consider (feature idea)

Quoth tye:

As for Tailors, I'd propose that gods and Tailors have the ability to approve patches and pmdevs be allowed to apply and back out approved patches. I covered this in more detail in Re^2: RFC: Create a PatchAppliers group (crub)

Then it'd be cool if Tailors could approve patches that they didn't write so that the coup would require a pmdev member to write a patch and a tailor to approve it, to just add that bit of friction to the process. Yes, Tailors would also be pmdev members but you'd need two logins (or one god login) to be able to write and apply new code.

relevant cb convo

jdporter: in P's original proposal, the new group, Tailors, would have god-like authority to apply patches.
jdporter: in your counter-proposal, it's the devil who applies. I think this makes sense, since, conventionally, it is the applier who is responsible for verification of the patch once it has been applied.
tye: yeah, I'd rather spread the work of applying, testing, reverting wider than Tailors

jdporter: just to be clear - any other changes - outside the patch system - would still require the action of a god. e.g. creating a new nodetype.
jdporter: I ask, because in one possible interpretation of P's proposal, Tailors would have god-like authority to make changes; they'd be gods-minus (minus the policy related role)
jdporter: not that I think that's what P had in mind

tye: well, being able to apply patches means it is trivial to get gods-level access. That's why I'd like Tailors to be able to approve only patches that they didn't write.

jdporter: so - my understanding is that a patch can be applied by pmdev if it has been approved, and that a god can approve. And that a Tailor is, then, god-like in precisely one way: authorized to approve patches.
tye: It will still be a fairly easy coup but I think requiring a conspiracy helps for such a policy-only security "wall"
jdporter: ok - Tailors can not approve own patches. That's good.

tye: yeah, that's my thinking so far. Tailors might end up having the ability to create empty nodes and move patches to refer to them (phase 2)
jdporter: ah - e.g. if a new htmlcode was needed.

tye: the other aspect is that applying a patch needs to "approve" whatever patch is "current" and refuse to apply if there is no such patch (or just create such a patch)
jdporter: oh, I see. that allows for revert.

jdporter: and how about this: a pmdevil can only apply own patches.
tye: why in particular are you thinking pmdevs shouldn't be able to apply other approved patches?
jdporter: I dunno. Some kind of stupid intuition, I suppose.
jdporter: and obviously it would get in the way of revert, if I don't own the previously current patch.
jdporter: though that could be overcome through code
tye: I'm not opposed to "can only apply own patches" at least as a first stance. It is more conservative and surely reduces some forms of abuse.
tye: perhaps you could apply any approved patch if the "current" patch is yours. note also that it is possible to "give" your patch to pmdev so any member can edit it. So take that into account re applying.

tye: there should also be a retirement feature so that an evil pmdev can't wreak havoc by applying a bunch of out-dated but approved patches
jdporter: yeah! approval expires within a reasonable timeframe, e.g. one month.

ambrus: maybe approving a patch should mark the parent patch it's based on, so a pmdev can apply it only if that parent patch is active, or something similar?
tye: ambrus, I like that idea (i.e. tracking parent patch). It would also be very helpful when viewing older patches.
tye: so, slight adjustment: writing a patch notes what patch was current at the time. approval overwrites that if different.
jdporter: yes. in fact, irrespective of what we do with patch approvals, the patch system should always track what patch was current at the time of patch submissions.
jdporter: it could even automatically make an "original version capture" patch if no patches exist.
tye: also, 'edit' should update the 'parent' information
tye: I say patch application should be where the "original, unpatched state" should be created (and applied and approved), if needed
jdporter: well, 'edit' entails re-'submit', so that should happen automatically.
jdporter: hm. I don't see why, tye. I think even un-applied patches should have a referent.
ambrus: tye: no edit shouldn't automatically update parent status.
tye: oh, having patches w/ no parent would be a bit ugly. patch creation time might be a better place for that then, yes.
ambrus: (but edit should unapprove the patch of course)
jdporter: maybe, on edit, the user can select whether to update the referent. a simple checkbox.
jdporter: so it sounds like we're going to need a new patch type, with a new table, to have fields 'previous_patch' and 'approved_exp_date'.
jdporter: unless it's possible to add a dbtable to the definition of an existing nodetype.
ambrus: why new type? why not just add fields to the existing type and table?
jdporter: oh, right. adding fields to existing table is also easy.

tye: no, edit should update the parent and should unapprove the patch /before/ the edit is applied (perhaps even as a separate step for the user)
tye: (I worry of races or bugs tempting "edit at same time as applied" exploits)
tye: editing a patch to leave it with an out-dated parent makes little sense. editing of applied patches should probably be disallowed (except to gods)
tye: (the main motivation I see for editing an old patch is for the purpose of making it sane to apply, that is, make it have a new parent)

jdporter: need to make sure that, if applying a patch marks its predecessor as approved, an errant pmdev can't revert all the way back up the chain to the primordial version of a node.
jdporter: I think the way to achieve this is to not mark the predecessor as as approved, but to allow revert through some other checks. I.e. "this is predecessor of an approved patch and that approved patch is current."
tye: reverting makes the "new" patch the "old" patch (without even having to do anything special)
tye: I'd mark the 'current' as 'approved' so that it can't be edited
tye: the more exceptions, the more places for security holes
tye: please also make patch application a POST operation (and enforce that it is only done that way), if you get the time
tye: then we can control when reverting is possible by removing the approval
tye: (for a security fix, gods would unapprove the reversal patch rather quickly. a batch process could unapprove reversal patches $N days after a patch was applied)
tye: we'll need an htmlcode for "find current patch" that does the equivalent of "select patch_id from ... where node_id = $N order by patch_applied desc limit 3 having patch.code = document.doctext"... I wonder if that is possible in SQL.
tye: nope. can't put 'having' after 'limit'. sad, that.
tye: can we just "assume" that the most recently applied patch should be "current" and create a new patch if not?
tye: and we may want to avoid races w/ extra conditionals on the SQL.
jdporter: I wasn't even talking about in the face of edits. If, as I think was proposed, applying a (new) patch marks its predecessor as approved so that the pmdev can revert, then it would be possible for him to revert all the way down the stack.
jdporter: that would clearly be A Bad Thing.
tye: no, it wouldn't. since step two would make the 'new' patch approved because /it/ was current
jdporter: ("all the way down" only as far as the predecessor field has been populated, of course)
tye: the 'current' patch gets approved, not the 'parent' patch.
tye: update document as code, patch, document as p set code.doctext=p.doctext where code.document_id=$N and patch_id=$P and p.document_id=$P and p.doctext=$content and patch.is_approved;
tye: ($content would be the patch content as seen on the patch page that the approval was submitted from)
tye: update doc as code, patch, doc as new, doc as cur set code.doctext=new.doctext where code.doc_id=$N and patch_id=$new and new.doc_id=$new and cur.doc_id=$parent and p.doctext=$content and patch.is_approved and code.doctext=cur.doctext; even
tye: s/$parent/patch.parent_patch/
jdporter: wait - how does one revert, then? doesn't the predecessor patch need to be approved?
tye: (if 0 records updated, then we had a race so abort patch application)
tye: 'current' patch at time of application, not 'parent' patch
jdporter needs to study that sql. I'm sure your answer is in there.
jdporter: oh, I see. so one could "revert" back and forth between the top two patches.

tye: yes
jdporter: and, generally: 'Edit' does both (a) set patch.approval = null; (b) set patch.parent = current_patch. Correct?
tye: yes. But if approval is done "at the same time", then the really long SQL I posted become important to prevent races. And that's probably the route I'd go.
jdporter: I do think we want to tighten down editability. Obviously, if patch has never been applied, it's editable; but also...
tye: 'approval' would be user_id of who approved it. Tracking date of approval probably not required.
tye: note that there is a convention that DB fields that contain node IDs should have an '_' in their name. Otherwise, they shouldn't.
jdporter: if it has been reverted. is there an easy way to tell if it's been reverted, without adding an explicit flag for such?
jdporter: and also without doing a full table scan?
tye: only gods would be allowed to edit patches that have ever been applied (as I already said this morning)
jdporter: no, I thought we wanted to expire approvals.
jdporter: oh, ok. camelCase, then?
tye: expiring approvals have nothing to do with whether a patch has been applied, so I don't understand your response.
jdporter: (but note, my above was meant to be pseudocode.)
tye: so we'd add approving_user and parent_patch to the patch table
jdporter: ok then I don't understand the context of your comment, 'approval' would be user_id of who approved it. Tracking date of approval probably not required.
tye: actually, we should have a separate 'approved' boolean so unapproving doesn't hide who originally approved it.
jdporter: I would let the non-null-ness of approvaldate indicate the approved status.
jdporter: unless for some reason we want to remember that date forever.
tye: that's fine
jdporter: thanks. I feel like it's coming together.
tye: approval should have similar (but not as bad) SQL for preventing races.
jdporter: actually... we could leave the date, and have the "isPatchApproved" code do return $approvaldate && now() <= $approvaldate + 30; (or some such)
jdporter: that way we'll remember the date "forever", until it gets re-approved.
tye: ah! There would still be the race of editing an unapproved patch at the same time as it is getting approved. The new node cache wouldn't force the approval status to be unset. So might be worth doing that as a separate SQL update.
jdporter: that's a tiny bit beyond me, man
tye: hmm. I think I'd rather have expriing done via cron batch.
tye: 1) create a patch, 2) notice that somebody is about to approve it, 3) edit patch to put in exploit, 4) patch gets approved, 5) node cache flushes your update, changing doctext of patch but your edit didn't change the approval fields...
tye: the (new) node cache doesn't touch those field and they remain in the 'approved' state.
tye: (The current node cache wastefully (and also undesirable for other reasons) updates all fields every time.)
jdporter: really? I find cron batches unreliable. not to mention being code outside the engine.
tye: really. putting cron code into the DB would be nice. we have several cron things.
jdporter: ok, well, I don't feel strongly about that. cron is certainly "clean", conceptually.

jdporter: k, I'm having doubts about whether it's necessary to disallow Tailors from applying patches they wrote. Tailors must be trusted persons. More trusted than anyone, aside from gods.
jdporter: How about this: a Tailor can approve his own patches.... but with an enforced time delay. Say, 7 days.
tye: a time delay is fine as well, so long as you also implement a "can't ever be approved as-is" feature. I'm not sure that seems worth the extra complexity.
tye: the point is that approval becomes so easy that it stops being a roadblock. I'd prefer gods wait for somebody to approve unless it is an emergency.
jdporter: I'm actually a little surprised at how much complexity mere approval adds.
jdporter: currently, we have "some pmdev writes it, some god applies it"
jdporter: now it will be "a pmdev writes it, a tailor approves it - but only if it's not his own - and then a pmdev applies it - but only if it is his own..."
jdporter: etc etc. Wow.
tye: if "is(not) his" is a simple check requiring no extra fields. "can never be approved" requires a new data field.
jdporter: sure. I'm just saying the new scheme is considerably more complex than the "one more man in the loop" would seem to imply at first glance.
tye: adding an 'if' to each of several htmlcodes is not much additional complexity, to me.
jdporter: I suppose at the code level it won't be too bad. But if you compare the logical "rules" that pertain the old and new systems...
jdporter: adding the Tailor doesn't add one or two more rules, it adds like a dozen.
jdporter: as we've defined it, that is. many of the rules are for security -- checks and balances.
tye: yeah. the old system was overly simple on that level because the bar was so far on one side. pmdev could write a patch. gods had to do everything after that, much of it by hand.
tye: adds about one or two new rules at each step, adds a new step, and automates some other steps, encoding some rules there :)
jdporter: well put
tye: so I'd rather have Tailors not be able to approved their own patches, just like I can't approve nor front-page my own nodes, even from gods, unless I resort to rolling my own code / SQL :)
tye: even trusted people have blind spots when it comes to their own work. Good to have code review. Let's just make it easy.
tye: And I'd like a god applying their own unapproved to incur an extra step and some chiding. (:
tye: require them to update the patch reason to say "emergency"
tye: I'm actually looking forward to my flow changing to waiting for somebody to approve my patch before /I/ apply it.
tye: one day, maybe things will even be to the point that I can expect to get a reply to a patch before I apply it

Tailors group

Possesses precisely one power (which would also obtain to gods, of course): The power to approve a patch.

An approved patch is one which can be applied by a member of pmdev.

In our initial incarnation of this capability, the only pmdev who can apply an approved patch is the owner of the patch. This may be relaxed in the future.

Note that the author of a patch can transfer ownership to the pmdev group.

gods' ability to apply a patch directly will not be affected by this new feature.

Tailors would normally also be members of pmdev.

A Tailor cannot approve a patch he wrote. (I'm actually not sure this is necessary; Tailors must needs be very trusted individuals.)

Play all
        DepriMarvin  >  <color=#800080>Scout</color> +1 Trade (Trade:1)
        DepriMarvin  >  <color=#800080>Scout</color> +1 Trade (Trade:2)
        DepriMarvin  >  <color=#800080>Scout</color> +1 Trade (Trade:3)
Acquired <color=#FFFF00>Falcon</color>
        DepriMarvin - -3 Trade (Trade:0)
DepriMarvin ends turn 1
        Drew 5 cards.
        It is now GuyFleegman's turn 2
Play all
        GuyFleegman  >  <color=#800080>Scout</color> +1 Trade (Trade:1)

Log In?

What's my password?
Create A New User
Domain Nodelet?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others lurking in the Monastery: (3)
As of 2023-12-08 02:19 GMT
Find Nodes?
    Voting Booth?
    What's your preferred 'use VERSION' for new CPAN modules in 2023?

    Results (34 votes). Check out past polls.