-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
append.c: only call out to annotator on email #5236
base: master
Are you sure you want to change the base?
Conversation
4962039
to
4c08c1a
Compare
4c08c1a
to
dec29ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Othe rthan my comment, LGTM
imap/append.c
Outdated
@@ -1107,7 +1107,10 @@ EXPORTED int append_fromstage_full(struct appendstate *as, struct body **body, | |||
goto out; | |||
} | |||
|
|||
if (config_getstring(IMAPOPT_ANNOTATION_CALLOUT)) { | |||
if ( | |||
config_getstring(IMAPOPT_ANNOTATION_CALLOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit. We (the core devs) typically don't put like breaks after the opening '(' and before the closing ')'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to address. (Also, eager to see what autoformatting we could agree on…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ksmurchison I believe my latest commit matches the style I've now found elsewhere in append.c. Look right to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sir. All good!
The "/*" nonsense is so that the test annotator won't choke on the comments needed to make the content valid Sieve.
When we do a SieveScript/set.create to make a script, plus a onSuccessActivateScript, we would set the authstate *only* for the onSuccessActivateScript. This worked in two parts: 1. append happens, as nobody, which works because of an "anyone p" ACL 2. flag set happens on onSuccessActivateScript, which worked because we set the auth state In writing tests for the Annotator, I found that it couldn't be instructed to add flags, because it didn't have permission. That's because the annotator runs during step 1, append, when it was acting as nobody. We don't want the annotator to run, but we also want it (and other things) to see the right auth state. So, we set the auth state up correctly always, now.
d08b8e0
to
25f21ce
Compare
The annotator exists so that installations can update email messages to have extra flags or annotations. Because "everything" is an email, it was also running on sieve scripts and other non-emails. This was probably harmless on many installations, but not intended.
This branch updates the annotator to run only on messages being appended to mailboxes of type
e
.It also fixes a bug in
sieve_db
, wherein we only set the authstate for theonSuccessActivateScript
side effect, not the rest of script creation.