Skip to content

Commit

Permalink
streams: Centralize logic for computing stream permissions.
Browse files Browse the repository at this point in the history 
I found the previous model for computing what settings to use for
streams increasingly difficult to understand, which is generally a
recipe for future bugs.

Refactor to have a clear computation of what complete permissions
state the client is requesting, validate that state, and then pass
that state to the do_change_stream_permission.
  • Loading branch information
@timabbott
timabbott committed Aug 10, 2022
1 parent 3e64638 commit 66b29fb
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 72 deletions.
37 changes: 6 additions & 31 deletions zerver/actions/streams.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
from zerver.lib.stream_traffic import get_average_weekly_stream_traffic, get_streams_traffic
from zerver.lib.streams import (
can_access_stream_user_ids,
get_default_value_for_history_public_to_subscribers,
get_occupied_streams,
get_stream_permission_policy_name,
render_stream_description,
Expand Down Expand Up @@ -833,42 +832,18 @@ def send_change_stream_permission_notification(
def do_change_stream_permission(
stream: Stream,
*,
invite_only: Optional[bool] = None,
history_public_to_subscribers: Optional[bool] = None,
is_web_public: Optional[bool] = None,
invite_only: bool,
history_public_to_subscribers: bool,
is_web_public: bool,
acting_user: UserProfile,
) -> None:
old_invite_only_value = stream.invite_only
old_history_public_to_subscribers_value = stream.history_public_to_subscribers
old_is_web_public_value = stream.is_web_public

# A note on these assertions: It's possible we'd be better off
# making all callers of this function pass the full set of
# parameters, rather than having default values. Doing so would
# allow us to remove the messy logic below, where we sometimes
# ignore the passed parameters.
#
# But absent such a refactoring, it's important to assert that
# we're not requesting an unsupported configurations.
if is_web_public:
stream.is_web_public = True
stream.invite_only = False
stream.history_public_to_subscribers = True
else:
# is_web_public is falsey
if invite_only is None:
# This is necessary to get correct default value for
# history_public_to_subscribers when invite_only is
# None.
invite_only = stream.invite_only
history_public_to_subscribers = get_default_value_for_history_public_to_subscribers(
stream.realm,
invite_only,
history_public_to_subscribers,
)
stream.invite_only = invite_only
stream.history_public_to_subscribers = history_public_to_subscribers
stream.is_web_public = False
stream.is_web_public = is_web_public
stream.invite_only = invite_only
stream.history_public_to_subscribers = history_public_to_subscribers

with transaction.atomic():
stream.save(update_fields=["invite_only", "history_public_to_subscribers", "is_web_public"])
Expand Down
114 changes: 73 additions & 41 deletions zerver/views/streams.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,79 @@ def update_stream_backend(
# description even for private streams.
(stream, sub) = access_stream_for_delete_or_update(user_profile, stream_id)

# Validate that the proposed state for permissions settings is permitted.
if is_private is not None:
proposed_is_private = is_private
else:
proposed_is_private = stream.invite_only

if is_web_public is not None:
proposed_is_web_public = is_web_public
else:
proposed_is_web_public = stream.is_web_public

if stream.realm.is_zephyr_mirror_realm:
# In the Zephyr mirroring model, history is unconditionally
# not public to subscribers, even for public streams.
proposed_history_public_to_subscribers = False
elif history_public_to_subscribers is not None:
proposed_history_public_to_subscribers = history_public_to_subscribers
elif is_private is not None:
# By default, private streams have protected history while for
# public streams history is public by default.
proposed_history_public_to_subscribers = not is_private
else:
proposed_history_public_to_subscribers = stream.history_public_to_subscribers

# Web-public streams must have subscriber-public history.
if proposed_is_web_public and not proposed_history_public_to_subscribers:
raise JsonableError(_("Invalid parameters"))

# Web-public streams must not be private.
if proposed_is_web_public and proposed_is_private:
raise JsonableError(_("Invalid parameters"))

# Public streams must be public to subscribers.
if not proposed_is_private and not proposed_history_public_to_subscribers:
if stream.realm.is_zephyr_mirror_realm:
# All Zephyr realm streams violate this rule.
pass
else:
raise JsonableError(_("Invalid parameters"))

if is_private is not None:
# Default streams cannot be made private.
default_stream_ids = {s.id for s in get_default_streams_for_realm(stream.realm_id)}
if is_private and stream.id in default_stream_ids:
raise JsonableError(_("Default streams cannot be made private."))

# We require even realm administrators to be actually
# subscribed to make a private stream public, via this
# stricted access_stream check.
access_stream_by_id(user_profile, stream_id)

# Enforce restrictions on creating web-public streams. Since these
# checks are only required when changing a stream to be
# web-public, we don't use an "is not None" check.
if is_web_public:
if not user_profile.realm.web_public_streams_enabled():
raise JsonableError(_("Web-public streams are not enabled."))
if not user_profile.can_create_web_public_streams():
raise JsonableError(_("Insufficient permission"))

if (
is_private is not None
or is_web_public is not None
or history_public_to_subscribers is not None
):
do_change_stream_permission(
stream,
invite_only=proposed_is_private,
history_public_to_subscribers=proposed_history_public_to_subscribers,
is_web_public=proposed_is_web_public,
acting_user=user_profile,
)

if message_retention_days is not None:
if not user_profile.is_realm_owner:
raise OrganizationOwnerRequired()
Expand Down Expand Up @@ -310,47 +383,6 @@ def update_stream_backend(
if stream_post_policy is not None:
do_change_stream_post_policy(stream, stream_post_policy, acting_user=user_profile)

# But we require even realm administrators to be actually
# subscribed to make a private stream public.
if is_private is not None:
default_stream_ids = {s.id for s in get_default_streams_for_realm(stream.realm_id)}
(stream, sub) = access_stream_by_id(user_profile, stream_id)
if is_private and stream.id in default_stream_ids:
raise JsonableError(_("Default streams cannot be made private."))

if (
not is_private
and history_public_to_subscribers is False
and not stream.realm.is_zephyr_mirror_realm
):
raise JsonableError(_("Invalid parameters"))

if is_web_public:
# Enforce restrictions on creating web-public streams.
if not user_profile.realm.web_public_streams_enabled():
raise JsonableError(_("Web-public streams are not enabled."))
if not user_profile.can_create_web_public_streams():
raise JsonableError(_("Insufficient permission"))
# Forbid parameter combinations that are inconsistent
if is_private or history_public_to_subscribers is False:
raise JsonableError(_("Invalid parameters"))

if history_public_to_subscribers is False and not stream.realm.is_zephyr_mirror_realm:
if is_private is None and not stream.invite_only:
raise JsonableError(_("Invalid parameters"))

if (
is_private is not None
or is_web_public is not None
or history_public_to_subscribers is not None
):
do_change_stream_permission(
stream,
invite_only=is_private,
history_public_to_subscribers=history_public_to_subscribers,
is_web_public=is_web_public,
acting_user=user_profile,
)
return json_success(request)


Expand Down

0 comments on commit 66b29fb

Please sign in to comment.