[4175] in BarnOwl Developers
Re: [barnowl/barnowl] Experimental Zulip support (#184)
daemon@ATHENA.MIT.EDU (Anders Kaseorg)
Tue Oct 17 03:11:16 2017
Date: Mon, 16 Oct 2017 19:50:40 +0000 (UTC)
From: Anders Kaseorg <notifications@github.com>
Reply-To: barnowl/barnowl <reply+004448c94514ed6249d18b083737b582ec14aca24d39d46f92cf0000000115fccf9092a169ce0fd8a3d7@reply.github.com>
To: barnowl/barnowl <barnowl@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
In-Reply-To: <barnowl/barnowl/pull/184@github.com>
----==_mimepart_59e50d9070434_500a3fb0c2dbaf30489f3
Content-Type: text/plain;
charset=UTF-8
Content-Transfer-Encoding: quoted-printable
andersk requested changes on this pull request.
> @@ -0,0 +1,15 @@
+use strict;
+use warnings;
+
+use inc::Module::Install;
+
+requires('AnyEvent::HTTP');
+requires('AnyEvent::TLS');
+requires('JSON');
+requires('URI');
+requires('URI::Encode');
+requires('MIME::Base64');
These requirements should be documented in the top-level `README`.
> + }
+ return BarnOwl::quote("zulip:write", @filtered_recipients);
+}
+
+# Strip layers of "un" and ".d" from names
+sub base_name {
+ my $name =3D shift;
+
+ while($name =3D~ /(^un)/) {
+ $name =3D~ s/(^un)//;
+ }
+ while($name =3D~ /\.d$/) {
+ $name =3D~ s/(\.d$)//g;
+ }
+ return $name;
+}
Or just `my ($base) =3D $name =3D~ /^(?:un)*(.*?)(?:\.d)*$/; return $base;`
> +
+sub baseclass {
+ my $self =3D shift;
+ return base_name($self->class);
+}
+
+sub baseinstance {
+ my $self =3D shift;
+ return base_name($self->instance);
+}
+
+sub quote_for_filter {
+ my $quote_chars =3D '!+*.?\[\]\^\\${}()|';
+ my $str =3D shift;
+ return ($str =3D~ s/[$quote_chars]/\\$1/gr);
+}
Despite appearances, this function fails to quote backslashes.
> + if($self->is_private) {
+ my @recips =3D $self->private_recipient_list;
+ if(scalar(@recips) > 1) {
+ BarnOwl::message("Smartnarrow for group personals not supported yet. =
Sorry :(");
+ return "";
+ } else {
+ my $person;
+ if($self->direction eq "in") {
+ $person =3D $self->sender;
+ } else {
+ $person =3D $self->recipient;
+ }
+ $filter =3D "zulip-user-$person";
+ # $person =3D~ s/\./\\./
+ $person =3D~ quote_for_filter($person);
+ @filter =3D split / /, "( type ^Zulip\$ and filter personal and ( ( d=
irection ^in\$ and sender ^${person}\$ ) or ( direction ^out\$ and recipien=
t ^${person}\$ ) ) )";
Avoid splitting interpolated variables:
```perl
@filter =3D (qw{( type ^Zulip$ and filter personal and ( ( direction ^in$ a=
nd sender}, "^$person\$", qw{) or ( direction ^out$ and recipient}, "^$pers=
on\$", qw{) ) )})
```
> + my @filter;
+ if($self->is_private) {
+ my @recips =3D $self->private_recipient_list;
+ if(scalar(@recips) > 1) {
+ BarnOwl::message("Smartnarrow for group personals not supported yet. =
Sorry :(");
+ return "";
+ } else {
+ my $person;
+ if($self->direction eq "in") {
+ $person =3D $self->sender;
+ } else {
+ $person =3D $self->recipient;
+ }
+ $filter =3D "zulip-user-$person";
+ # $person =3D~ s/\./\\./
+ $person =3D~ quote_for_filter($person);
You meant `=3D`, not `=3D~`.
> + }
+ $filter =3D "related-";
+ } else {
+ $class =3D $self->class;
+ if($inst) {
+ $instance =3D $self->instance;
+ }
+ $filter =3D "";
+ }
+ $filter .=3D "class-$class";
+ if($inst) {
+ $filter .=3D "-instance-$instance";
+ }
+ $class =3D quote_for_filter($class);
+ # TK deal with filter already existing
+ @filter =3D (($related ? ("class", "^(un)*${class}(\\.d)*\$") : ("class",=
"^${class}\$")));
Missing `"type", "^Zulip\$"`?
> + if( exists $raw_cfg->{ssl}) {
+ # mandatory parameters
+ if (! exists $raw_cfg->{ssl}->{ca_file}) {
+ fail("SSL parameters specified, but no CA file set");
+ }
+ $cfg{'ssl_ca_file'} =3D $raw_cfg->{ssl}->{ca_file};
+ $cfg{'ssl_verify'} =3D 1;
+ # optional parameters
+ if ( (exists $raw_cfg->{ssl}->{cert_file}) && exists $raw_cfg->{ss=
l}->{key_file}) {
+ $cfg{'ssl_cert_file'} =3D $raw_cfg->{ssl}->{cert_file};
+ $cfg{'ssl_key_file'} =3D $raw_cfg->{ssl}->{key_file};
+ } else {
+ warn "SSL parameters specified, but no client credentials set.=
";
+ }
+ } else {
+ $cfg{'ssl_verify'} =3D 0;
No.
This is a really bad default in 2017, even with the warning (and even for s=
omeone who understands why they shouldn=E2=80=99t ignore the warning: the c=
onsequence of making some typo in the config file shouldn=E2=80=99t be that=
your API key gets exposed). If a user really needs to skip SSL verificatio=
n, they should be explicit about it.
> + $cfg{'apikey'} =3D $apikey;
+ $cfg{'api_url'} =3D $api_url;
+ $cfg{'realm'} =3D $default_realm;
+}
+
+sub register {
+ my $retry_count =3D 0;
+ my $callback;
+
+ if(!exists $cfg{'api_url'}) {
+ die("Zulip not configured, cannot poll for events");
+ }
+ $callback =3D sub {
+ BarnOwl::debug("In register callback");
+ my ($body, $headers) =3D @_;
+ if($headers->{Status} > 399) {
Are you expecting anything other than 200 here (and the other instances of =
`> 399` and `< 400`)? `!=3D 200` seems safer.
> + zulip($type, $to, $subject, $text);
+ });
+=20=20=20=20
+}
+
+BarnOwl::new_command('zulip:login' =3D> sub { cmd_zulip_login(@_); },
+ {
+ summary =3D> "Log in to Zulip",
+ usage =3D> "zulip:login",
+ description =3D> "Start receiving Zulip messages"
+ });
+
+BarnOwl::new_command('zulip:write' =3D> sub { cmd_zulip_write(@_); },
+ {
+ summary =3D> "Send a zulipgram",
+ usage =3D> "zulip:login [-c stream] [-i subject] =
[recipient(s)]",
s/login/write/
> @@ -330,3 +330,7 @@ sub subcontext {""}
=20
=20
1;
+
+# Local Variables:
+# indent-tabs-mode: nil
+# End:
This seems to be a whitespace-only change.
--=20
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/barnowl/barnowl/pull/184#pullrequestreview-69666001=
----==_mimepart_59e50d9070434_500a3fb0c2dbaf30489f3
Content-Type: text/html;
charset=UTF-8
Content-Transfer-Encoding: quoted-printable
<p><b>@andersk</b> requested changes on this pull request.</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4939101">perl/modules/Zulip/Makefile.PL</a>:</p>
<pre style=3D'color:#555'>> @@ -0,0 +1,15 @@
+use strict;
+use warnings;
+
+use inc::Module::Install;
+
+requires('AnyEvent::HTTP');
+requires('AnyEvent::TLS');
+requires('JSON');
+requires('URI');
+requires('URI::Encode');
+requires('MIME::Base64');
</pre>
<p>These requirements should be documented in the top-level <code>README</c=
ode>.</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4940812">perl/modules/Zulip/lib/BarnOwl/Message/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> + }
+ return BarnOwl::quote("zulip:write", @filtered_recipients);
+}
+
+# Strip layers of "un" and ".d" from names
+sub base_name {
+ my $name =3D shift;
+
+ while($name =3D~ /(^un)/) {
+ $name =3D~ s/(^un)//;
+ }
+ while($name =3D~ /\.d$/) {
+ $name =3D~ s/(\.d$)//g;
+ }
+ return $name;
+}
</pre>
<p>Or just <code>my ($base) =3D $name =3D~ /^(?:un)*(.*?)(?:\.d)*$/; return=
$base;</code></p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4942510">perl/modules/Zulip/lib/BarnOwl/Message/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> +
+sub baseclass {
+ my $self =3D shift;
+ return base_name($self->class);
+}
+
+sub baseinstance {
+ my $self =3D shift;
+ return base_name($self->instance);
+}
+
+sub quote_for_filter {
+ my $quote_chars =3D '!+*.?\[\]\^\\${}()|';
+ my $str =3D shift;
+ return ($str =3D~ s/[$quote_chars]/\\$1/gr);
+}
</pre>
<p>Despite appearances, this function fails to quote backslashes.</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4943502">perl/modules/Zulip/lib/BarnOwl/Message/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> + if($self->is_private) {
+ my @recips =3D $self->private_recipient_list;
+ if(scalar(@recips) > 1) {
+ BarnOwl::message("Smartnarrow for group personals not supported =
yet. Sorry :(");
+ return "";
+ } else {
+ my $person;
+ if($self->direction eq "in") {
+ $person =3D $self->sender;
+ } else {
+ $person =3D $self->recipient;
+ }
+ $filter =3D "zulip-user-$person";
+ # $person =3D~ s/\./\\./
+ $person =3D~ quote_for_filter($person);
+ @filter =3D split / /, "( type ^Zulip\$ and filter personal and =
( ( direction ^in\$ and sender ^${person}\$ ) or ( direction ^out\$ and rec=
ipient ^${person}\$ ) ) )";
</pre>
<p>Avoid splitting interpolated variables:</p>
<div class=3D"highlight highlight-source-perl"><pre><span class=3D"pl-smi">=
@filter</span> =3D (<span class=3D"pl-s"><span class=3D"pl-pds">qw{</span>(=
type ^Zulip$ and filter personal and ( ( direction ^in$ and sender<span cl=
ass=3D"pl-pds">}</span></span>, <span class=3D"pl-s"><span class=3D"pl-pds"=
>"</span>^<span class=3D"pl-smi">$person</span><span class=3D"pl-cce">\$</s=
pan><span class=3D"pl-pds">"</span></span>, <span class=3D"pl-s"><span clas=
s=3D"pl-pds">qw{</span>) or ( direction ^out$ and recipient<span class=3D"p=
l-pds">}</span></span>, <span class=3D"pl-s"><span class=3D"pl-pds">"</span=
>^<span class=3D"pl-smi">$person</span><span class=3D"pl-cce">\$</span><spa=
n class=3D"pl-pds">"</span></span>, <span class=3D"pl-s"><span class=3D"pl-=
pds">qw{</span>) ) )<span class=3D"pl-pds">}</span></span>)</pre></div>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4943717">perl/modules/Zulip/lib/BarnOwl/Message/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> + my @filter;
+ if($self->is_private) {
+ my @recips =3D $self->private_recipient_list;
+ if(scalar(@recips) > 1) {
+ BarnOwl::message("Smartnarrow for group personals not supported =
yet. Sorry :(");
+ return "";
+ } else {
+ my $person;
+ if($self->direction eq "in") {
+ $person =3D $self->sender;
+ } else {
+ $person =3D $self->recipient;
+ }
+ $filter =3D "zulip-user-$person";
+ # $person =3D~ s/\./\\./
+ $person =3D~ quote_for_filter($person);
</pre>
<p>You meant <code>=3D</code>, not <code>=3D~</code>.</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4944444">perl/modules/Zulip/lib/BarnOwl/Message/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> + }
+ $filter =3D "related-";
+ } else {
+ $class =3D $self->class;
+ if($inst) {
+ $instance =3D $self->instance;
+ }
+ $filter =3D "";
+ }
+ $filter .=3D "class-$class";
+ if($inst) {
+ $filter .=3D "-instance-$instance";
+ }
+ $class =3D quote_for_filter($class);
+ # TK deal with filter already existing
+ @filter =3D (($related ? ("class", "^(un)*${class}(\\.d)*\=
$") : ("class", "^${class}\$")));
</pre>
<p>Missing <code>"type", "^Zulip\$"</code>?</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4945527">perl/modules/Zulip/lib/BarnOwl/Module/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> + if( exists $raw_cfg->{ssl}) {
+ # mandatory parameters
+ if (! exists $raw_cfg->{ssl}->{ca_file}) {
+ fail("SSL parameters specified, but no CA file set");
+ }
+ $cfg{'ssl_ca_file'} =3D $raw_cfg->{ssl}->{ca_file};
+ $cfg{'ssl_verify'} =3D 1;
+ # optional parameters
+ if ( (exists $raw_cfg->{ssl}->{cert_file}) && exists=
$raw_cfg->{ssl}->{key_file}) {
+ $cfg{'ssl_cert_file'} =3D $raw_cfg->{ssl}->{cert=
_file};
+ $cfg{'ssl_key_file'} =3D $raw_cfg->{ssl}->{key_f=
ile};
+ } else {
+ warn "SSL parameters specified, but no client credentials=
set.";
+ }
+ } else {
+ $cfg{'ssl_verify'} =3D 0;
</pre>
<p>No.</p>
<p>This is a really bad default in 2017, even with the warning (and even fo=
r someone who understands why they shouldn=E2=80=99t ignore the warning: th=
e consequence of making some typo in the config file shouldn=E2=80=99t be t=
hat your API key gets exposed). If a user really needs to skip SSL verifica=
tion, they should be explicit about it.</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4946314">perl/modules/Zulip/lib/BarnOwl/Module/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> + $cfg{'apikey'} =3D $apikey;
+ $cfg{'api_url'} =3D $api_url;
+ $cfg{'realm'} =3D $default_realm;
+}
+
+sub register {
+ my $retry_count =3D 0;
+ my $callback;
+
+ if(!exists $cfg{'api_url'}) {
+ die("Zulip not configured, cannot poll for events");
+ }
+ $callback =3D sub {
+ BarnOwl::debug("In register callback");
+ my ($body, $headers) =3D @_;
+ if($headers->{Status} > 399) {
</pre>
<p>Are you expecting anything other than 200 here (and the other instances =
of <code>> 399</code> and <code>< 400</code>)? <code>!=3D 200</code> =
seems safer.</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4948801">perl/modules/Zulip/lib/BarnOwl/Module/Zulip.pm</a>:</p>
<pre style=3D'color:#555'>> + zulip($type, $t=
o, $subject, $text);
+ });
+=20=20=20=20
+}
+
+BarnOwl::new_command('zulip:login' =3D> sub { cmd_zulip_login(@=
_); },
+ {
+ summary =3D> "Log in to Zulip",
+ usage =3D> "zulip:login",
+ description =3D> "Start receiving Zulip m=
essages"
+ });
+
+BarnOwl::new_command('zulip:write' =3D> sub { cmd_zulip_write(@=
_); },
+ {
+ summary =3D> "Send a zulipgram",
+ usage =3D> "zulip:login [-c stream] [-i s=
ubject] [recipient(s)]",
</pre>
<p>s/login/write/</p>
<hr>
<p>In <a href=3D"https://github.com/barnowl/barnowl/pull/184#discussion_r14=
4949473">perl/lib/BarnOwl/Message.pm</a>:</p>
<pre style=3D'color:#555'>> @@ -330,3 +330,7 @@ sub subcontext {"&q=
uot;}
=20
=20
1;
+
+# Local Variables:
+# indent-tabs-mode: nil
+# End:
</pre>
<p>This seems to be a whitespace-only change.</p>
<p style=3D"font-size:small;-webkit-text-size-adjust:none;color:#666;">&mda=
sh;<br />You are receiving this because you are subscribed to this thread.<=
br />Reply to this email directly, <a href=3D"https://github.com/barnowl/ba=
rnowl/pull/184#pullrequestreview-69666001">view it on GitHub</a>, or <a hre=
f=3D"https://github.com/notifications/unsubscribe-auth/AERIyW_Jfr0VIFkAdrHO=
BL4wbKpqJjZAks5ss7OQgaJpZM4P6_o2">mute the thread</a>.<img alt=3D"" height=
=3D"1" src=3D"https://github.com/notifications/beacon/AERIyUCVzrf8xRlMGjarp=
NyRvneMAnGXks5ss7OQgaJpZM4P6_o2.gif" width=3D"1" /></p>
<div itemscope itemtype=3D"http://schema.org/EmailMessage">
<div itemprop=3D"action" itemscope itemtype=3D"http://schema.org/ViewAction=
">
<link itemprop=3D"url" href=3D"https://github.com/barnowl/barnowl/pull/18=
4#pullrequestreview-69666001"></link>
<meta itemprop=3D"name" content=3D"View Pull Request"></meta>
</div>
<meta itemprop=3D"description" content=3D"View this Pull Request on GitHub"=
></meta>
</div>
<script type=3D"application/json" data-scope=3D"inboxmarkup">{"api_version"=
:"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"Gi=
tHub"},"entity":{"external_key":"github/barnowl/barnowl","title":"barnowl/b=
arnowl","subtitle":"GitHub repository","main_image_url":"https://cloud.gith=
ubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7b=
b5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/1434=
18/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Ope=
n in GitHub","url":"https://github.com/barnowl/barnowl"}},"updates":{"snipp=
ets":[{"icon":"PERSON","message":"@andersk requested changes on #184"}],"ac=
tion":{"name":"View Pull Request","url":"https://github.com/barnowl/barnowl=
/pull/184#pullrequestreview-69666001"}}}</script>=
----==_mimepart_59e50d9070434_500a3fb0c2dbaf30489f3--
