460580 - (bz-securemail) Hooks for SecureMail extension

Status: RESOLVED WONTFIX

(Bugzilla :: Extensions, enhancement)

Description

[Max Kanat-Alexander]

I'm writing an extension called "securemail" for bugzilla.mozilla.org that encrypts some types of emails sent from Bugzilla. It needs lots of new hooks, so this is the tracking bug for adding all of them upstream.

Comment 1

[Gervase Markham [:gerv]]

Attachment: Changes made for Securemail in 3.6

Hi Max,

Here are the changes I've made to add hooks to get Securemail working on 3.6. The larger changes are backports from 3.8, and the smaller ones are the addition of new hooks.

I'm not asking for a full review; can you just let me know which of these changes you would in principle accept on the stable branch, under the hook admissions policy, and whether the names are right? I'll then go away and make a patch with just those ones and add documentation and example code.

Thanks,

Gerv

Comment 2

[Max Kanat-Alexander]

Comment on attachment 443591 [details] [diff] [review]
Changes made for Securemail in 3.6

I think group_end_of_set_all should just be object_end_of_set_all.

user_email_save should just be user_end_of_update, and userprefs.cgi should be modified to use Bugzilla::User (I have a bug on that even.)

The template hooks all look OK, and can even go into 3.6 if you want.

Comment 3

[Reed Loden [:reed]]

(In reply to comment #2)
> userprefs.cgi should be modified to use Bugzilla::User (I have a bug on that even.)

Bug 555829

Comment 4

[Gervase Markham [:gerv]]

Thanks for the quick review!

(In reply to comment #2)
> (From update of attachment 443591 [details] [diff] [review])
> I think group_end_of_set_all should just be object_end_of_set_all.

I assume you don't mean I should rename the hook, but that I should use the existing object_end_of_set_all instead of adding a new group_end_of_set_all.

I'm not quite sure how I could do that. It's not possible to call set_all with a field name which is not one of the built-in ones, because set_all just calls set_<paramname> functions, and such a function would not exist. And if I can't call set_all with the new data from doGroupChanges(), then I can't trigger the object_end_of_set_all hook.

So you will have to explain more what you mean :-)

> user_email_save should just be user_end_of_update, 

Unfortunately, the structure of the switch statement around line 523 (in 3.6) of userprefs.cgi makes it a bit tricky to insert a generic user_end_of_update which fires for all five tabs. I agree that this is the right thing in the end, but it may not be the right thing for a 3.6 b.m.o. patch.

> and userprefs.cgi should be
> modified to use Bugzilla::User (I have a bug on that even.)

It should, but presumably that's a 3.8 fix. So I will keep things as they are for the 3.6 patch for b.m.o. Once this is done, we can implement user_end_of_update and switch to using that.

> The template hooks all look OK, and can even go into 3.6 if you want.

Awesome. I'll split those out into a separate patch.

Are you happy to take the backports of the object_columns and object_validators hooks for 3.6?

Gerv

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> I'm not quite sure how I could do that. It's not possible to call set_all with
> a field name which is not one of the built-in ones, because set_all just calls
> set_<paramname> functions, and such a function would not exist. And if I can't
> call set_all with the new data from doGroupChanges(), then I can't trigger the
> object_end_of_set_all hook.

  If you have to set something that's not a normal set_ function, then you should override Object::set_all in Group::set_all. If that override is in fact necessary, and you then need a hook at the end of Group::set_all (as opposed to the SUPER::set_all call inside it) then we could consider adding a group_end_of_set_all hook.

> I agree that this is the right thing in the end,
> but it may not be the right thing for a 3.6 b.m.o. patch.

  Oh yeah, there's no way you should be refactoring things on 3.6 for bmo. Just do whatever's easiest. But my reviews here are for what should be done upstream, since I'm not working on bmo.

> Are you happy to take the backports of the object_columns and object_validators
> hooks for 3.6?

  No, I already decided not to backport those.

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.1

OK, here are the uncontroversial ones (template hooks and one case of passing in an extra variable), for trunk checkin and hopefully also 3.6 backport approval.

Gerv

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.2

...and now one against the trunk. Sorry!

Gerv

Comment 8

[Max Kanat-Alexander]

Comment on attachment 445791 [details] [diff] [review]
Patch v.2

>=== modified file 'Bugzilla/Token.pm'
>+    $vars->{'user'} = $user;

  Actually, we don't usually add in variables that we aren't using--there's a fair chance that somebody will come along and remove this, going "Hey, this isn't being used."

  Also, you can't call the template variable "user"--there's already a global template variable with that name that you definitely don't want to be over-writing (it represents Bugzilla->user).

  Perhaps the variable should be renamed and the email template should use it.

>=== modified file 'template/en/default/email/newchangedmail.txt.tmpl'
> [%+ threadingmarker %]
>+[%+ Hook.process("extra_headers") %]
> 

  Hmm, does that add an extra newline at the top of emails, though, when there's no hook data?

Comment 9

[Gervase Markham [:gerv]]

(In reply to comment #8)
>   Actually, we don't usually add in variables that we aren't using--there's a
> fair chance that somebody will come along and remove this, going "Hey, this
> isn't being used."

We've had this discussion before, but: if we are considering template writing to be a valid way of extending Bugzilla, then the interface presented to the templates in the form of the "vars" hash is a public interface. So people can't just go removing things from it like that, and if they are of that mind, they should be told not to.

>   Also, you can't call the template variable "user"--there's already a global
> template variable with that name that you definitely don't want to be
> over-writing (it represents Bugzilla->user).

Well, that's what I want, the current user - but it doesn't seem to be accessible in the template when it's called by Hook.process. So I've had to keep the variable, and change the name to "to_user".

>   Hmm, does that add an extra newline at the top of emails, though, when
> there's no hook data?

Yes. However, after an hour of trying, I can't find any way to make this Do The Right Thing both in the case where there's no added headers and where there is. Hook.process() seems to be stripping whitespace from the beginning and end of the included text, or something.

The workaround I am currently using is:

X-Bugzilla-Changed-Fields: [% changedfields %]
[%+ Hook.process("extra_headers") || "X-TT-Whitespace-Handling: Is-A-Pain" %]
[%+ threadingmarker %]

If you know how to do it better, please tell me!

Gerv

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch for b.m.o. 3.6

This is an updated version of the patch needed for 3.6 and b.m.o. (fixes some bitrot). This patch matches the latest version of the Securemail extension in the extensions/securemail/3.6 bzr repo (revision 8).

The two together should permit securemail to be used on current b.m.o.

Gerv

Comment 11

[Gervase Markham [:gerv]]

Dave is taking on the task of getting this on to b.m.o.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Securemail for Bugzilla 4.0 no longer requires any hooks.

Gerv