This extracts the core general group-syncing logic, which is independent
of any LDAP-specific concerns, to a separate function. This allows the
use of this core logic for group sync in other authentication backends.
The code also gets a bit cleaned up in the process to make it more
readable (with some readability tweaks to the log strings as well).
This was broken, due the mechanism simply using our
is_guest/is_realm_admin/etc. role setters, but failing to adjust system
group memberships - resulting in corrupted database state.
We need to ensure that change_user_role is called for setting user role.
There are two relevant codepaths that run the sync based on
AUTH_LDAP_USER_FLAGS_BY_GROUP and thus need to get this right:
1. manage.py sync_ldap_user_data
2. Just-in-time user creation when a user without a Zulip account logs
in for the first using their ldap credentials. After
get_or_build_user returns, django-auth-ldap sees that the user
account has just been created, and proceeds to run ._populate_user().
Now that both user.save() and do_change_user_realm will be getting
called together, we need to ensure this always happens atomically.
This imposes the need to override _get_or_create_user to put it in a
transaction. The troublesome consequence is that this new
`atomic(savepoint=False)` causes the usual type of issue, where tests
testing error get their transaction rolled back and cannot continue
executing.
To get around that, we add a test helper
`artificial_transaction_savepoint` which allows these tests to wrap
their problematic blocks in an artificial transaction which provides a
savepoint, thus preventing the full test transaction rollback derailing
the rest of the test.
Without these overrides, we cannot test the functionality in DEVELOPMENT
and TESTING.
There are two codepaths that we're covering here:
1. The sync which happens via `sync_ldap_user_data`.
2. The sync which happens during just-in-time user creation upon first
login via ldap.
Both codepaths end up triggering ldap_user._get_or_create_user().
Apparently, while we set our own maximum password length of 100
characters, zxcvbn had a hardcoded maximum length of 72 characters,
and threw an exception if that was exceeded.
The fact that we're discovering this now would suggest that nobody has
previously attempted a password between 72 and 100 characters in
length.
As detailed in the comment in the code:
The upstream implementation uses the outdated /oauth2/authorize
API (instead of the v2.0 API), which doesn't allow us to authenticate
users with just a personal Microsoft account. v2.0 API is required.
This requires us to override the default URLs to use it as well
as adjust the requested scopes, to match this new API.
The backend in its previous state was only able to authenticate users
that were tied to an organizational directory, even if the application
settings in Azure were set up to also allow personal accounts. Users
trying to use a personal account would face an error from Microsoft:
AADSTS500200: User account 'xxxx@example.com' is a
personal Microsoft account. Personal Microsoft accounts are not
supported for this application unless explicitly invited to an
organization
https://github.com/python-social-auth/social-core/issues/723 is a
related upstream issue.
There was a bug here that would trigger an exception inside
`sync_user_profile_custom_fields`, causing it to get logged with
logging.warning, when an attribute configured for SAML custom profile
field sync was missing from a SAMLResponse or had an empty value.
`sync_user_profile_custom_fields` expects valid values, and None is not
valid.
We could consider a slightly different behavior here instead - when an
attribute is sent with no value in the SAMLResponse, that means the attr
has no value in the IdP's user directory - so perhaps a better behavior
would be to also remove the custom profile field value in Zulip. However
there are two issues with that:
1. It's not necessarily the best behavior, because an organization might
want the "user doesn't have this attribute set at the IdP level" state
to just mean that the user should be free to set the value manually in
Zulip if they wish. And having that value get reset on every login would
then be an issue. The implementation in this commit is consistent with
this philosophy.
2. There's some implementation difficulty - upstream
`self.get_attr(...)`, which we use for reading the attr value from the
SAMLResponse, doesn't distinguish between an attribute being sent with
no value and the attribute not being sent at all - in both cases it
returns None. So we'd need some extra work here with parsing the
SAMLResponse properly, to be able to know when the custom profile field
should get cleared.
Replace the SOCIAL_AUTH_SYNC_CUSTOM_ATTRS_DICT with
SOCIAL_AUTH_SYNC_ATTRS_DICT, designed to support also regular user attrs
like role or full name (in the future).
Custom attributes can stay configured as they were and will get merged
into SOCIAL_AUTH_SYNC_ATTRS_DICT in computed_settings, or can be
specified in SOCIAL_AUTH_SYNC_ATTRS_DICT directly with "custom__"
prefix.
The role sync is plumbed through to user creation, so users can
immediately be created with their intended role as provided by the IdP
when they're creating their account, even when doing this flow without
an invitiation.
In #23380, we are changing all occurrences of uri with url in order to
follow the latest URL standard. Previous PRs #25038 and #25045 has
replaced the occurences of uri that has no direct relation with realm.
This commit changes just the model property, which has no API
compatibility concerns.
This commit updates code to access name from named_user_group
field which points to the "NamedUserGroup" instead of directly
accessing name from "UserGroup", since name field will only
be present on NamedUserGroup objects in further commits.
Previously, users were allowed to signup or change their names to
those which already existed in the realm.
This commit adds an Organization Permission, that shall enforce
users to use unique names while signing up or changing their
names. If a same or normalized full name is found in realm,
then a validation error is thrown.
Fixes#7830.
Only affects zulipchat, by being based on the BILLING_ENABLED setting.
The restricted backends in this commit are
- AzureAD - restricted to Standard plan
- SAML - restricted to Plus plan, although it was already practically
restricted due to requiring server-side configuration to be done by us
This restriction is placed upon **enabling** a backend - so
organizations that already have a backend enabled, will continue to be
able to use it. This allows us to make exceptions and enable a backend
for an org manually via the shell, and to grandfather organizations into
keeping the backend they have been relying on.
By convention, all our backends inherit form ZulipAuthMixin. This is
helpful to have a single place if we want to add some class attributes
that all the backends should have.
The original behavior of this setting was to disable LDAP
authentication for any realms not configured to use it. This was an
arbitrary choice, and its only value was to potentially help catch
typos for users who are lazy about testing their configuration.
Since it makes it a very inconvenient to potentially host multiple
organizations with different LDAP configurations, remove that
behavior.
This is an exception that we should be generally catching like the
others, which will give our standard /login/ redirect and proper logging
- as opposed to a 500 if we don't catch.
Addresses directly a bug we occurred in the wild, where a SAMLResponse
was submitted without issuers specified in a valid way, causing this
exception. The added test tests this specific type of scenario.
Failing to remove all of the rules which were added causes action at a
distance with other tests. The two methods were also only used by
test code, making their existence in zerver.lib.rate_limiter clearly
misplaced.
This fixes one instance of a mis-balanced add/remove, which caused
tests to start failing if run non-parallel and one more anonymous
request was added within a rate-limit-enabled block.
This doesn't have any obvious security implications right now, but
nonetheless such information is not meant to stick around in the session
if authentication didn't succeed and not cleaning up would be a bug.
This is a useful improvement in general for making correct
LogoutRequests to Idps and a necessary one to make SP-initiated logout
fully work properly in the desktop application. During desktop auth
flow, the user goes through the browser, where they log in through their
IdP. This gives them a logged in browser session at the IdP. However,
SAML SP-initiated logout is fully conducted within the desktop
application. This means that proper information needs to be given to the
the IdP in the LogoutRequest to let it associate the LogoutRequest with
that logged in session that was established in the browser. SessionIndex
is exactly the tool for that in the SAML spec.
This gives more flexibility on a server with multiple organizations and
SAML IdPs. Such a server can have some organizations handled by IdPs
with SLO set up, and some without it set up. In such a scenario, having
a generic True/False server-wide setting is insufficient and instead
being able to specify the IdPs/orgs for SLO is needed.
Closes#20084
This is the flow that this implements:
1. A logged-in user clicks "Logout".
2. If they didn't auth via SAML, just do normal logout. Otherwise:
3. Form a LogoutRequest and redirect the user to
https://idp.example.com/slo-endpoint?SAMLRequest=<LogoutRequest here>
4. The IdP validates the LogoutRequest, terminates its own user session
and redirects the user to
https://thezuliporg.example.com/complete/saml/?SAMLRequest=<LogoutResponse>
with the appropriate LogoutResponse. In case of failure, the
LogoutResponse is expected to express that.
5. Zulip validates the LogoutResponse and if the response is a success
response, it executes the regular Zulip logout and the full flow is
finished.
We now set tos_version to "-1" for imported users and the ones
created using API or using other methods like LDAP, SCIM and
management commands. This value will help us to allow users to
change email address visibility setting during first login.
The immediate application of this will be for SAML SP-initiated logout,
where information about which IdP was used for authenticating the
session needs to be accessed. Aside of that, this seems like generally
valuable session information to keep that other features may benefit
from in the future.
