Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notifications: Fix leaking of private stream msgs sent before subscribing. #10086

Conversation

shubhamdhama
Copy link
Member

Here I've suggested some refactors, feedback about how they sound to other. I'm working on top of it to fix the issue 9834.

zerver/models.py Outdated
@@ -1558,6 +1558,12 @@ def __str__(self) -> str:
class UserMessage(AbstractUserMessage):
message = models.ForeignKey(Message, on_delete=CASCADE) # type: Message

def get_usermessage_by_msg_id(user_profile: UserProfile, message_id: int) -> Optional[UserMessage]:
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd spell it by_message_id, but otherwise looks good.

@@ -492,39 +492,48 @@ def access_message(user_profile: UserProfile, message_id: int) -> Tuple[Message,

user_message = get_usermessage_by_msg_id(user_profile, message_id)

if has_message_access(user_profile, message, user_message):
return (message, user_message)
else:
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else is unnecessary

@timabbott
Copy link
Sponsor Member

These look reasonable

@shubhamdhama shubhamdhama force-pushed the 9834-fix-email-notification-context branch from da77244 to 301d253 Compare July 27, 2018 16:47
@zulipbot zulipbot added size: XL and removed size: L labels Jul 27, 2018
@shubhamdhama shubhamdhama force-pushed the 9834-fix-email-notification-context branch from 4d237c2 to 4fc9be7 Compare July 28, 2018 11:52
@shubhamdhama
Copy link
Member Author

@timabbott I've updated the PR up to v1, so only optimization is left.

@shubhamdhama shubhamdhama force-pushed the 9834-fix-email-notification-context branch from 4fc9be7 to 8fab5b3 Compare July 28, 2018 11:57
@shubhamdhama
Copy link
Member Author

shubhamdhama commented Jul 28, 2018

build failing due to another issue, which is fixed in #10093

@shubhamdhama shubhamdhama changed the title message: Minor refactor of access_message. notifications: Fix leaking of private stream msgs sent before subscribing. Jul 28, 2018
@shubhamdhama shubhamdhama force-pushed the 9834-fix-email-notification-context branch from 8fab5b3 to 288ceab Compare July 28, 2018 15:36
@shubhamdhama shubhamdhama force-pushed the 9834-fix-email-notification-context branch from 288ceab to fb3eead Compare July 28, 2018 15:37
@@ -446,10 +446,14 @@ def handle_missedmessage_emails(user_profile_id: int,
for recipient_subject, msgs in messages_by_recipient_subject.items()
}

from zerver.lib.message import bulk_access_messages

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import to avoid a circular loop is avoidable with some refactoring. I added 6bbffe0 to do the appropriate refactor to avoid this (I think it's cleaner this way anyway).

@zulipbot
Copy link
Member

Heads up @shubhamdhama, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott
Copy link
Sponsor Member

I merged this, after adding 6bbffe0 to avoid the circular import workaround. Thanks @shubhamdhama!

For working on the optimization piece, I think we can just add a wrapper for bulk_access_messages, with a name like bulk_access_messages_in_stream, and have it pass in the Stream object (and maybe also the subscription object?), using e.g. this approach:

@@ -497,13 +497,15 @@ def access_message(user_profile: UserProfile, message_id: int) -> Tuple[Message,
     raise JsonableError(_("Invalid message(s)"))
 
 def has_message_access(user_profile: UserProfile, message: Message,
-                       user_message: Optional[UserMessage]) -> bool:
+                       user_message: Optional[UserMessage],
+                       stream: Optional[Stream]=None) -> bool:
     if user_message is None:
         if message.recipient.type != Recipient.STREAM:
             # You can't access private messages you didn't receive
             return False
 
-        stream = Stream.objects.get(id=message.recipient.type_id)
+        if stream is None:
+            stream = Stream.objects.get(id=message.recipient.type_id)
         if stream.realm != user_profile.realm:
             # You can't access public stream messages in other realms
             return False

@timabbott timabbott closed this Jul 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants