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

Support "file:///" as hyperlink #508

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions zerver/lib/bugdown/__init__.py
Expand Up @@ -651,7 +651,7 @@ def sanitize_url(url):
if not scheme:
return sanitize_url('http://' + url)

locless_schemes = ['mailto', 'news']
locless_schemes = ['mailto', 'news', 'file']
if netloc == '' and scheme not in locless_schemes:
# This fails regardless of anything else.
# Return immediately to save additional proccessing
Expand All @@ -661,7 +661,7 @@ def sanitize_url(url):
# appears to have a netloc. Additionally there are plenty of other
# schemes that do weird things like launch external programs. To be
# on the safe side, we whitelist the scheme.
if scheme not in ('http', 'https', 'ftp', 'mailto'):
if scheme not in ('http', 'https', 'ftp', 'mailto', 'file'):
return None

# Upstream code scans path, parameters, and query for colon characters
Expand Down Expand Up @@ -940,12 +940,13 @@ def extendMarkdown(self, md, md_globals):
%s # zero-to-6 sets of paired parens
)?) # Path is optional
| (?:[\w.-]+\@[\w.-]+\.[\w]+) # Email is separate, since it can't have a path
%s # File path start with file:///, enable by setting ENABLE_FILE_LINKS=True
)
(?= # URL must be followed by (not included in group)
[!:;\?\),\.\'\"\>]* # Optional punctuation characters
(?:\Z|\s) # followed by whitespace or end of string
)
""" % (tlds, nested_paren_chunk)
""" % (tlds, nested_paren_chunk, r"| (?:file://(/[^/ ]*)+/?)" if settings.ENABLE_FILE_LINKS else r"")
md.inlinePatterns.add('autolink', AutoLink(link_regex), '>link')

md.preprocessors.add('hanging_ulists',
Expand Down
2 changes: 1 addition & 1 deletion zerver/models.py
Expand Up @@ -999,7 +999,7 @@ def content_has_image(content):

@staticmethod
def content_has_link(content):
return 'http://' in content or 'https://' in content or '/user_uploads' in content
return 'http://' in content or 'https://' in content or '/user_uploads' in content or 'file:///' in content
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

hmm probably this or case should be conditional on settings.ENABLE_FILE_LINKS as well, right?


def update_calculated_fields(self):
# TODO: rendered_content could also be considered a calculated field
Expand Down
5 changes: 5 additions & 0 deletions zerver/test_bugdown.py
Expand Up @@ -175,6 +175,11 @@ def replaced(payload, url, phrase=''):
converted = bugdown_convert(inline_url)
self.assertEqual(match, converted)

def test_inline_file(self):
msg = 'Check out this file file:///Volumes/myserver/Users/Shared/pi.py'
converted = bugdown_convert(msg)
self.assertEqual(converted, '<p>Check out this file <a href="file:///Volumes/myserver/Users/Shared/pi.py" target="_blank" title="file:///Volumes/myserver/Users/Shared/pi.py">file:///Volumes/myserver/Users/Shared/pi.py</a></p>')

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 think it'd be good to have tests for correct behavior with both ENABLE_FILE_LINKS=False and ENABLE_FILE_LINKS=True

https://docs.djangoproject.com/en/dev/topics/testing/tools/#overriding-settings is probably relevant for how to write such tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the slow reply!
I have try overriding settings. However it didn't seems to work.
The function sill follow the setting that I set in zproject/test_settings.
How can I solve this problem? Thanks

def test_inline_file(self):
    # Using initial settings I setup in test_settings (ENABLE_FILE_LINKS=False)
    msg = 'Check out this file file:///Volumes/myserver/Users/Shared/pi.py'
    converted = bugdown_convert(msg)
    self.assertEqual(converted, '<p>Check out this file file:///Volumes/myserver/Users/Shared/pi.py</p>') # Success
    # Test for Enabling file links
    with self.settings(ENABLE_FILE_LINKS=True):
        converted = bugdown_convert(msg)
        self.assertEqual(converted, '<p>Check out this file <a href="file:///Volumes/myserver/Users/Shared/pi.py" target="_blank" title="file:///Volumes/myserver/Users/Shared/pi.py">file:///Volumes/myserver/Users/Shared/pi.py</a></p>') # Failed

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 think what's going on is effectively a form of caching -- the bugdown library only is initialized once per realm and the code involved is initialization code. See make_md_engine for details. I think you could fix this by first creating a new empty realm with domain e.g. "file_links_test.example.com" and then using bugdown.convert(text ,realm_domain="file_links_test.example.com") rather than the bugdown_convert helper.

def test_inline_youtube(self):
msg = 'Check out the debate: http://www.youtube.com/watch?v=hx1mjT73xYE'
converted = bugdown_convert(msg)
Expand Down
4 changes: 4 additions & 0 deletions zproject/local_settings_template.py
Expand Up @@ -112,6 +112,10 @@
# a link to an image is referenced in a message.
INLINE_IMAGE_PREVIEW = True

# Controls whether or not Zulip will parse links starting with
# "file:///" as a hyperlink.
ENABLE_FILE_LINKS = False
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe expand this comment a bit more to discuss the use case, e.g.

# Controls whether or not Zulip will parse links starting with
# "file:///" as a hyperlink (useful if you have e.g. an NFS share).


# By default, files uploaded by users and user avatars are stored
# directly on the Zulip server. If file storage in Amazon S3 is
# desired, you can configure that as follows:
Expand Down
1 change: 1 addition & 0 deletions zproject/settings.py
Expand Up @@ -159,6 +159,7 @@ def get_secret(key):
'REMOTE_POSTGRES_SSLMODE': '',
'GOOGLE_CLIENT_ID': '',
'DBX_APNS_CERT_FILE': None,
'ENABLE_FILE_LINKS' : False,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

You should also add a patch to zproject/local_settings_template.py documenting the feature and providing a clear place to set this new setting.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

cool, looks like you did this one.

}

for setting_name, setting_val in DEFAULT_SETTINGS.iteritems():
Expand Down
3 changes: 3 additions & 0 deletions zproject/test_settings.py
Expand Up @@ -51,6 +51,9 @@
}
}

# Enable file:/// hyperlink for test
ENABLE_FILE_LINKS = True

LOGGING['loggers']['zulip.requests']['level'] = 'CRITICAL'
LOGGING['loggers']['zulip.management']['level'] = 'CRITICAL'

Expand Down