Experimental Zulip support #184
Conversation
Allow validation of server certs by specifying a CA file and at least theoretically support client certs (I will test the latter on Monday)
…anks to adehnert for noticing the tabs
add USAGE section and more notes on future work
…it couldn't parse the config file). Thanks to Alex Dehnert for the bug report
Zulip requires them anyway and making them required fixes some obnoxious quoting-related behavior.
| requires('JSON'); | ||
| requires('URI'); | ||
| requires('URI::Encode'); | ||
| requires('MIME::Base64'); |
There was a problem hiding this comment.
These requirements should be documented in the top-level README.
| $name =~ s/(\.d$)//g; | ||
| } | ||
| return $name; | ||
| } |
There was a problem hiding this comment.
Or just my ($base) = $name =~ /^(?:un)*(.*?)(?:\.d)*$/; return $base;
| my $quote_chars = '!+*.?\[\]\^\\${}()|'; | ||
| my $str = shift; | ||
| return ($str =~ s/[$quote_chars]/\\$1/gr); | ||
| } |
There was a problem hiding this comment.
Despite appearances, this function fails to quote backslashes.
There was a problem hiding this comment.
What am I missing here? \ is in $quote_chars and $quote_chars gets interpolated into the character class of things to be quoted...
There was a problem hiding this comment.
In single quotes, "A backslash represents a backslash unless followed by the delimiter or another backslash, in which case the delimiter or backslash is interpolated."
The '\\' in $quote_chars becomes a '\', which escapes the '$' in the regular expression. Also, you're not capturing the character class, so $1 isn't defined.
Isn't escaping fun? '\\\\'
NB: I had to add yet another layer of escaping just so the markdown would display what I wanted.
| $filter = "zulip-user-$person"; | ||
| # $person =~ s/\./\\./ | ||
| $person =~ quote_for_filter($person); | ||
| @filter = split / /, "( type ^Zulip\$ and filter personal and ( ( direction ^in\$ and sender ^${person}\$ ) or ( direction ^out\$ and recipient ^${person}\$ ) ) )"; |
There was a problem hiding this comment.
Avoid splitting interpolated variables:
@filter = (qw{( type ^Zulip$ and filter personal and ( ( direction ^in$ and sender}, "^$person\$", qw{) or ( direction ^out$ and recipient}, "^$person\$", qw{) ) )})| } | ||
| $filter = "zulip-user-$person"; | ||
| # $person =~ s/\./\\./ | ||
| $person =~ quote_for_filter($person); |
| } | ||
| $class = quote_for_filter($class); | ||
| # TK deal with filter already existing | ||
| @filter = (($related ? ("class", "^(un)*${class}(\\.d)*\$") : ("class", "^${class}\$"))); |
| warn "SSL parameters specified, but no client credentials set."; | ||
| } | ||
| } else { | ||
| $cfg{'ssl_verify'} = 0; |
There was a problem hiding this comment.
No.
This is a really bad default in 2017, even with the warning (and even for someone who understands why they shouldn’t ignore the warning: the consequence of making some typo in the config file shouldn’t be that your API key gets exposed). If a user really needs to skip SSL verification, they should be explicit about it.
There was a problem hiding this comment.
You are, of course, correct. I'm rewriting this, especially in light of the belated discovery that with no CA file specified, AnyEvent will use a platform-dependent default.
| $callback = sub { | ||
| BarnOwl::debug("In register callback"); | ||
| my ($body, $headers) = @_; | ||
| if($headers->{Status} > 399) { |
There was a problem hiding this comment.
Are you expecting anything other than 200 here (and the other instances of > 399 and < 400)? != 200 seems safer.
| BarnOwl::new_command('zulip:write' => sub { cmd_zulip_write(@_); }, | ||
| { | ||
| summary => "Send a zulipgram", | ||
| usage => "zulip:login [-c stream] [-i subject] [recipient(s)]", |
|
|
||
| # Local Variables: | ||
| # indent-tabs-mode: nil | ||
| # End: |
There was a problem hiding this comment.
This seems to be a whitespace-only change.
| } | ||
| } else { | ||
| # we still want it for a unique id | ||
| $tls_ctx = int(rand(1024)); |
There was a problem hiding this comment.
How unique can it be if you expect a collision within ~32 calls?
There was a problem hiding this comment.
I honestly don't remember why I picked 1024 here. Do you think replacing it with "high" (the AnyEvent::TLS "validate certs" canned context) is sufficient?
There was a problem hiding this comment.
Hm, no, that makes no sense, because this is the "no https" case.
| my @filtered_recipients = grep { $_ ne BarnOwl::Module::Zulip::user() } @recipients; | ||
| if (scalar(@filtered_recipients) == 0) { | ||
| # Self must have been only recipient, so re-add it (one copy) | ||
| @filtered_recipients = @recipients[0]; |
There was a problem hiding this comment.
Scalar value @recipients[0] better written as $recipients[0] at barnowl/perl/modules/Zulip/lib/BarnOwl/Message/Zulip.pm line 92.
| read_config($fh); | ||
| close($fh); | ||
| } | ||
| if ($cfg{'api_url'} =~ /^https/) { |
There was a problem hiding this comment.
Use of uninitialized value $BarnOwl::Module::Zulip::cfg{"api_url"} in pattern match (m//) at barnowl/perl/modules/Zulip/lib/BarnOwl/Module/Zulip.pm line 46.
(for non-Zulip users)
|
On Tue, 2017-10-17 at 03:25 +0000, Adam Glasgall wrote:
@aglasgall commented on this pull request.
In perl/modules/Zulip/lib/BarnOwl/Message/Zulip.pm:
> +
+sub baseclass {
+ my $self = shift;
+ return base_name($self->class);
+}
+
+sub baseinstance {
+ my $self = shift;
+ return base_name($self->instance);
+}
+
+sub quote_for_filter {
+ my $quote_chars = '!+*.?\[\]\^\\${}()|';
+ my $str = shift;
+ return ($str =~ s/[$quote_chars]/\\$1/gr);
+}
What am I missing here? \ is in $quote_chars and $quote_chars gets
interpolated into the character class of things to be quoted...
Firstly, it doesn't quote anything -- it substitutes a backslash in
place of each character that should be quoted.
Second, in a single-quoted string, a backslash stands for itself
_unless_ it is quoting the delimiter or a backslash. So $quote_chars
does not contain a doubled backslash. Instead, it contains a single
backslash quoting a dollar sign, which, inside a bracketed character
class, isn't special anyway. So, the pattern doesn't actually match a
backslash.
This should work as intended:
sub quote_for_filter {
my $quote_chars = '!+*.?\[\]\^\\\\${}()|';
my $str = shift;
return ($str =~ s/([$quote_chars])/\\$1/gr);
}
However... Is there some reason not to just use quotemeta() ?
|
|
Whatever happened here? Would it be useful for somebody else to sit down and fix things? Or do a barnowl hackathon? |
This PR adds experimental support for the Zulip [https://zulipchat.com/] chat system. Zulip is inspired by Zephyr and so it fits pretty well into the model Barnowl expects.
As implemented, the code supports:
sending and receiving zulip stream and personal messages (including
with multiple recipients, mostly-functionally)
listing, adding, and removing subscriptions
minimal support for stream creation (if you try to sub to a stream
that doesn't exist, it'll probably create a new one with whatever
your site's default settings are)
full filter language support (message attributes are mostly the same
as zephyr) including support for "punt" and mostly-functional
smartfilter.
support for displaying message edits (they show up as new messages
with the correct stream/topic/sender with the new text and opcode
EDIT)
It is missing, among other things:
better reliability around reconnecting to the server after it's gone down and the retry logic has given up
backfilling from history (this will be hard. barnowl currently only
supports appending to the msglist from perl, and the msglist
is also an array, so that's badness). Really, the right thing here is sqlite, but that's a very big piece to bite off.
smartnarrow robustness (has been ported from C, but almost certainly
still has weird corner cass)
better handling of personals with multiple recipients
syncing the curmsg pointer with the server
sending presence more cleverly (right now it just sends a ping every
minute regardless of how active you are)
deleting messages is probably interestingly broken right now w/r/t
the hash of zid -> barnowl message id. make it work.
being able to view user presence (ala zlocate/aim buddy list/jabber
roster/etc)
being able to invite people to invite-only streams (probably just a new command; should hopefully be straightforward)
improved URL generation. We shouldn't have to warn people not to
have a trailing / in their api_url setting (this one is probably an easy fix).
editing messages
option to allow in-place update of edited messages instead of adding
a new message
being able to create a stream with options (e.g. invite-only) set
being able to edit attributes of streams (zephyr doesn't have 'em,
but zulip does)
being able to delete streams
being able to see a list of people subscribed to a given stream
It is absolutely usable as a "daily driver" - I've been using it with the Zulip instance at my job for the past few years. The main driver for making this part of barnowl instead of an out-of-tree module is that support for handling message edit notifications required a small bit of XS (so you have to patch barnowl anyway to use it, so it may as well be in mainline)