Some admins setting up Zulip's LDAP auth against Active Directory see
a rather baffling error message: "In order to perform this operation a
successful bind must be completed on the connection". This happens
despite AUTH_LDAP_BIND_DN and auth_ldap_bind_password being set
perfectly correctly, and on a query that the `ldapsearch` command-line
tool performs quite happily.
Empirically, adding a setting like this to /etc/zulip/settings.py
resolves the issue:
AUTH_LDAP_CONNECTION_OPTIONS = {
ldap.OPT_REFERRALS: 0
}
Some useful, concise background on the LDAP "referral" concept is here:
https://docs.oracle.com/javase/jndi/tutorial/ldap/referral/overview.html
and a pertinent bit of docs for the underlying Python `ldap` client:
https://www.python-ldap.org/en/latest/faq.html
and some very helpful documentation for Active Directory:
https://docs.microsoft.com/en-us/windows/desktop/ad/referrals
Based on the docs above, the story appears to be something like this:
* This server has the information for part of the scope of our query
-- in particular it happens to have the information we actually want.
* But there are other areas ("subordinate domains") that our query is
in principle asking about, and this server doesn't know if there are
matches there, so it gives us a referral.
* And by default, python-ldap lets `libldap` run ahead and attempt to
bind to those referrals and do those queries too -- which raises an
error because, unlike Microsoft's "LDAP API", it doesn't reuse the
credentials.
So if we simply skip trying to follow the referrals, there's no
error... and we already have, from the original response, the answer
we actually need. That's what the `ldap.OPT_REFERRALS` option does.
There may be more complex situations where the referral really is
relevant, because the desired user info is split across servers. Even
then, unless an anonymous query will be acceptable, there's no point
in letting `libldap` follow the referral and setting this option is
still the right thing. When someone eventually comes to this bridge,
some code will be required to cross it, by following the referrals.
That code might look a bit like this (unfinished) example:
https://bugs.launchpad.net/nav/+bug/1209178
Manually tested by tabbott.
Fixes#343, which was effectively a report of the need for this
OPT_REFERRALS setting.
Fixes#349, since with this change, we no longer require tricky manual
configuration to get Active Directory up and running.
Previously, we were having issues installing on Debian Stretch with
non-English locales, because `locale-gen` actually doesn't take a
locale as an argument (and thus `locale-gen en_US.UTF-8` did nothing).
We should instead be calling localedef directly.
Thanks to Tom Daff for debugging this.
Fixes#10629.
Previously, Zulip did not correctly handle the case of a mobile device
being registered with a push device token being registered for
multiple accounts on the same server (which is a common case on
zulipchat.com). This was because our database `unique` and
`unique_together` indexes incorrectly enforced the token being unique
on a given server, rather than unique for a given user_id.
We fix this gap, and at the same time remove unnecessary (and
incorrectly racey) logic deleting and recreating the tokens in the
appropriate tables.
There's still an open mobile app bug causing repeated re-registrations
in a loop, but this should fix the fact that the relevant mobile bug
causes the server to 500.
Follow-up work that may be of value includes:
* Removing `ios_app_id`, which may not have much purpose.
* Renaming `last_updated` to `data_created`, since that's what it is now.
But none of those are critical to solving the actual bug here.
Fixes#8841.
Changes -
a) Updated the border-radius to 4px for all the buttons.
b) Increased the margins between the labels and inputs.
These changes affect the login and register page's styling.
We now allow outgoing webhooks to provide us a
"content" field, which is probably a more guessable
name than "response_string", particularly for folks
that use our other bot-related APIs. And we don't
modify content as we do response_string, i.e. no
"Success!" prefix.
If we're not too concerned about backward compatibility,
we can do a subsequent commit that makes "content"
and "response_string" true synonyms and get rid of
the "Success!" prefix, which was probably accidental
to begin with.
This commit starts by changing the third
argument of send_response_message to be a Dict
instead of a string, so that the data can be more
structured going forward.
That change makes the 2nd/3rd parameters both be
dicts, so to be defensive, I now have all the callers
pass in explicit keyword names. And then I rename
message to message_info, so that the callers have
more clear code.
And that changes the implementation inside of
send_response_message() a bit.
Sorry this commit is a bit coarse, but the intermediate
commits would have been kind of ugly, too.
At the end of the day, it's pretty simple:
bot_id: never changed
message_info: just renamed from message
response_data: is a Dict with the key of "content"
And the innards of send_response_message() are basically
simply dictionary lookups and function calls.
There's no reason to return a failure message in
process_success(), since it's implied to be part of
the success codepath. I didn't look at the full history
of how the strange API evolved, but the second element
of the tuple was clearly noise by the time I got here.
Neither of the subclasses ever set it, and none of the
consumers used it.
This two-line function wasn't really carrying its
weight, and it just made it harder to refactor the
overall codepath.
Eliminating the function forces us to mock at a slightly
deeper level, which is probably a good thing for what
the test intends to do. The deeper mock still verifies that
we're sending the message (good) without digging into
all the details of how we send it (good).
Note that we will still keep around the similarly named
`fail_with_message` helper, which is a lot more useful.
(The succeed/fail scenarios aren't really symmetric here.
For success, there are fewer codepaths that do more complex
things, whereas we have lots and lots of failure codepaths
that all do the same simple thing of replying with a canned
message.)
Before this change subclasses of OutgoingWebhookServiceInterface
would return a raw string as the first element of its return
tuple in process_success(). This is not a very flexible
design, as it prevents the bot from passing extra data like
`widget_content`.
It's also possible in the future that we'll want to let outgoing
bots reply directly to senders who mention them on streams, and
again the original design was overly constrained for that.
This commit does not actually change any functionality yet.
Tweaked by tabbott to use a declared constant rather than just use
5000 in multiple places; this also means we can change the count
without updating translations.
Fixes#10446.
The major changes are:
* Remove the --destroy-rebuild-database option
* Merge the new and existing self-hosted server sections
* Change the wording of the Gitter document to match the Slack one
IFTTT allows custom templating for their payloads, so the onus is
on the user to ensure that their custom templates conform to the
expectations outlined in our IFTTT webhook docs. For that reason,
these payloads weren't generated, but were manually edited.
After discovering a couple of bugs, I decided to thoroughly test
and rewrite this integration from scratch. The older code wasn't
generating coherent messages.
This also commit gets this integration up to 100% test coverage.
Test coverage was improved by removing an unused function and
removing some code (written by me) that was actually handling
Test Hook event types incorrectly.
It was a painful amount of work to generate the actual payload.
Since the only difference was a small build URL, I manually
edited the payload and used that for testing.
This commit gets our GitHub webhook up to 100% test coverage.
Some of the page build message code had insufficient test coverage.
I looked at generating the payloads that would allow me to test
the lines of code in question, but it was too much work to
generate the payloads and this seemed like a vague event anyway.
So I just rewrote the logic so that the lines missing
coverage are implicitly covered.
This is a part of our efforts to get this webhook's coverage
up to 100%.
Note that apart from just testing an uncovered line of code, this
commit also fixes a minor bug in the code for messages about issue
comment deletion and editing.
Note that Freshdesk allows custom templating for outgoing payloads
in their webhook UI. Therefore, the payloads added in this commit
did not have to be official payloads from Freshdesk.
Instead of just referring to the commit with the raw URL, we
should use the commit ID as the text of the hyperlink.
Note that in commit_status_changed type messages, the name of the
commit isn't available.
The function that generates the body of the commit_status_changed
event messages generated an invalid commit URL.
Most likely, we missed this because this event type is fairly
vague and it is possible it was never tested by users much,
if at all.
The lack of coverage was due to:
* An unused function that was never used anywhere.
* get_commit_status_changed_body was using a regex where it didn't
really need to use one. And there was an if statement that
assumed that the payload might NOT contain the URL to the commit.
However, I checked the payload and there shouldn't be any instances
where a commit event is generated but there is no URL to the commit.
* get_push_tag_body had an `else` condition that really can't happen
in any payload. I verified this by checking the BitBucket webhook
docs.
We shouldn't just ignore exceptions when encoding the incoming
auth credentials. Even if the incoming credentials are properly
encoded, it is better to know when that is the case or if
something else fails.
Greg Price
Tim Abbott
Jack Zhang
Akash Nimare
Rishi Gupta
Steve Howell
Callum Fraser
Lyla Fischer
Pragati Agrawal
Lenny Jagielski
Vishnu Ks
Cynthia Lin
Eeshan Garg