sjs/vdirsyncer
1
0
Fork 0
mirror of https://github.com/samsonjs/vdirsyncer.git synced 2026-03-25 08:55:50 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Fix all known URL-quoting problems

Browse source 
- Fix #49 -- The old fix caused problems with other servers. The new
  behavior only decodes ``@`` characters.

- ``@`` is now not used when generating a new href, as some servers seem
  to have problems with it (http://sabre.io/dav/character-encoding/).
  This behavior is configurable via the ``unsafe_href_chars`` parameters
  for DAV storages, and is disabled in the testsuite for Radicale and
  ownCloud.

- Decoding of hrefs is also done twice for CarddavStorage.list because
  of owncloud/contacts#581. Vdirsyncer has behaved like that before, but
  not intentionally.

- Storages now don't share their ``_get_href`` methods anymore.
This commit is contained in:
Markus Unterwaditzer 2014-08-04 10:49:58 +02:00
parent 73a540261c
commit eb1431e5db
11 changed files with 88 additions and 48 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
tests/storage/__init__.py
View file

@ -39,7 +39,8 @@ class BaseStorageTests(object):
return storage()
def _create_bogus_item(self, item_template=None):
r = random.random()
# assert that special chars are handled correctly.
r = '{}@vdirsyncer'.format(random.random())
item_template = item_template or self.item_template
return Item(item_template.format(r=r))
@ -84,8 +85,6 @@ class BaseStorageTests(object):
def test_update_nonexisting(self, s):
item = self._create_bogus_item()
with pytest.raises(exceptions.PreconditionFailed):
s.update(s._get_href(item), item, '"123"')
with pytest.raises(exceptions.PreconditionFailed):
s.update('huehue', item, '"123"')
@ -108,8 +107,8 @@ class BaseStorageTests(object):
def test_list(self, s):
assert not list(s.list())
s.upload(self._create_bogus_item())
assert list(s.list())
href, etag = s.upload(self._create_bogus_item())
assert list(s.list()) == [(href, etag)]
def test_has(self, s):
assert not s.has('asd')

2
tests/storage/dav/servers/radicale/__init__.py
View file

@ -105,4 +105,4 @@ class ServerMixin(object):
collection += self.storage_class.fileext
return {'url': url, 'username': 'bob', 'password': 'bob',
'collection': collection}
'collection': collection, 'unsafe_href_chars': ''}

26
tests/storage/dav/test_main.py
View file

@ -20,7 +20,8 @@ from tests import EVENT_TEMPLATE, TASK_TEMPLATE, VCARD_TEMPLATE
import vdirsyncer.exceptions as exceptions
from vdirsyncer.storage.base import Item
from vdirsyncer.storage.dav import CaldavStorage, CarddavStorage
from vdirsyncer.storage.dav import CaldavStorage, CarddavStorage, \
_normalize_href
from .. import StorageTests
@ -215,3 +216,26 @@ class TestCaldavStorage(DavStorageTests):
class TestCarddavStorage(DavStorageTests):
storage_class = CarddavStorage
item_template = VCARD_TEMPLATE
@pytest.mark.parametrize('base,path', [
('http://example.com/', ''),
('http://example.com/L%C3%98/', '/L%C3%98'),
('http://example.com/LØ/', '/L%C3%98'),
])
def test_normalize_href(base, path):
assert _normalize_href(base, 'asdf') == path + '/asdf'
assert _normalize_href(base, 'hahah') == path + '/hahah'
assert _normalize_href(base, 'whoops@vdirsyncer.vcf') == \
path + '/whoops@vdirsyncer.vcf'
assert _normalize_href(base, 'whoops%40vdirsyncer.vcf') == \
path + '/whoops@vdirsyncer.vcf'
assert _normalize_href(base, 'wh%C3%98ops@vdirsyncer.vcf') == \
path + '/wh%C3%98ops@vdirsyncer.vcf'
assert _normalize_href(base, 'whØops@vdirsyncer.vcf') == \
path + '/wh%C3%98ops@vdirsyncer.vcf'

32
tests/test_sync.py
View file

@ -23,7 +23,7 @@ def empty_storage(x):
def test_irrelevant_status():
a = MemoryStorage()
b = MemoryStorage()
status = {'1': ('1.txt', 1234, '1.ics', 2345)}
status = {'1': ('1', 1234, '1.ics', 2345)}
sync(a, b, status)
assert not status
assert empty_storage(a)
@ -55,8 +55,8 @@ def test_missing_status_and_different_items():
sync(a, b, status)
assert not status
sync(a, b, status, conflict_resolution='a wins')
assert_item_equals(item1, b.get('1.txt')[0])
assert_item_equals(item1, a.get('1.txt')[0])
assert_item_equals(item1, b.get('1')[0])
assert_item_equals(item1, a.get('1')[0])
def test_upload_and_update():
@ -67,22 +67,22 @@ def test_upload_and_update():
item = Item(u'UID:1') # new item 1 in a
a.upload(item)
sync(a, b, status)
assert_item_equals(b.get('1.txt')[0], item)
assert_item_equals(b.get('1')[0], item)
item = Item(u'UID:1\nASDF:YES') # update of item 1 in b
b.update('1.txt', item, b.get('1.txt')[1])
b.update('1', item, b.get('1')[1])
sync(a, b, status)
assert_item_equals(a.get('1.txt')[0], item)
assert_item_equals(a.get('1')[0], item)
item2 = Item(u'UID:2') # new item 2 in b
b.upload(item2)
sync(a, b, status)
assert_item_equals(a.get('2.txt')[0], item2)
assert_item_equals(a.get('2')[0], item2)
item2 = Item(u'UID:2\nASDF:YES') # update of item 2 in a
a.update('2.txt', item2, a.get('2.txt')[1])
a.update('2', item2, a.get('2')[1])
sync(a, b, status)
assert_item_equals(b.get('2.txt')[0], item2)
assert_item_equals(b.get('2')[0], item2)
def test_deletion():
@ -94,16 +94,16 @@ def test_deletion():
a.upload(item)
a.upload(Item(u'UID:2'))
sync(a, b, status)
b.delete('1.txt', b.get('1.txt')[1])
b.delete('1', b.get('1')[1])
sync(a, b, status)
assert not a.has('1.txt') and not b.has('1.txt')
assert not a.has('1') and not b.has('1')
a.upload(item)
sync(a, b, status)
assert a.has('1.txt') and b.has('1.txt')
a.delete('1.txt', a.get('1.txt')[1])
assert a.has('1') and b.has('1')
a.delete('1', a.get('1')[1])
sync(a, b, status)
assert not a.has('1.txt') and not b.has('1.txt')
assert not a.has('1') and not b.has('1')
def test_already_synced():
@ -113,7 +113,7 @@ def test_already_synced():
a.upload(item)
b.upload(item)
status = {
'1': ('1.txt', a.get('1.txt')[1], '1.txt', b.get('1.txt')[1])
'1': ('1', a.get('1')[1], '1', b.get('1')[1])
}
old_status = dict(status)
a.update = b.update = a.upload = b.upload = \
@ -122,7 +122,7 @@ def test_already_synced():
for i in (1, 2):
sync(a, b, status)
assert status == old_status
assert a.has('1.txt') and b.has('1.txt')
assert a.has('1') and b.has('1')
@pytest.mark.parametrize('winning_storage', 'ab')

3
vdirsyncer/storage/base.py
View file

@ -65,9 +65,6 @@ class Storage(object):
'''
raise NotImplementedError()
def _get_href(self, item):
return item.ident + self.fileext
def __repr__(self):
return '<{}(**{})>'.format(
self.__class__.__name__,

43
vdirsyncer/storage/dav.py
View file

@ -22,6 +22,21 @@ dav_logger = log.get(__name__)
CALDAV_DT_FORMAT = '%Y%m%dT%H%M%SZ'
def _normalize_href(base, href, decoding_rounds=1):
'''Normalize the href to be a path only relative to hostname and
schema.'''
if not href:
raise ValueError(href)
x = utils.urlparse.urljoin(base, href)
x = utils.urlparse.urlsplit(x).path
for i in range(decoding_rounds):
x = utils.compat.urlunquote(x)
x = utils.compat.urlquote(x, '/@')
return x
class Discover(object):
xml_home = None
@ -240,6 +255,8 @@ class DavStorage(Storage):
``guess``. If you know yours, consider setting it explicitly for
performance.
:param useragent: Default ``vdirsyncer``.
:param unsafe_href_chars: Replace the given characters when generating
hrefs. Defaults to ``'@'``.
'''
# the file extension of items. Useful for testing against radicale.
@ -259,7 +276,8 @@ class DavStorage(Storage):
_repr_attributes = ('username', 'url')
def __init__(self, url, username='', password='', collection=None,
verify=True, auth=None, useragent=USERAGENT, **kwargs):
verify=True, auth=None, useragent=USERAGENT,
unsafe_href_chars='@', **kwargs):
super(DavStorage, self).__init__(**kwargs)
url = url.rstrip('/') + '/'
@ -268,6 +286,7 @@ class DavStorage(Storage):
self.session = DavSession(url, username, password, verify, auth,
useragent, dav_header=self.dav_header)
self.collection = collection
self.unsafe_href_chars = unsafe_href_chars
# defined for _repr_attributes
self.username = username
@ -288,17 +307,14 @@ class DavStorage(Storage):
s.displayname = c['displayname']
yield s
def _normalize_href(self, href):
'''Normalize the href to be a path only relative to hostname and
schema.'''
if not href:
raise ValueError(href)
x = utils.urlparse.urljoin(self.session.url, href)
return utils.compat.urlunquote_plus(utils.urlparse.urlsplit(x).path)
def _normalize_href(self, *args, **kwargs):
return _normalize_href(self.session.url, *args, **kwargs)
def _get_href(self, item):
href = utils.compat.urlunquote_plus(item.ident) + self.fileext
return self._normalize_href(href)
href = item.ident
for char in self.unsafe_href_chars:
href = item.ident.replace(char, '_')
return self._normalize_href(href + self.fileext)
def get(self, href):
((actual_href, item, etag),) = self.get_multi([href])
@ -613,7 +629,10 @@ class CarddavStorage(DavStorage):
if 'vcard' not in content_type.text.lower():
continue
href = self._normalize_href(element.find('{DAV:}href').text)
# Decode twice because ownCloud encodes twice.
# See https://github.com/owncloud/contacts/issues/581
href = self._normalize_href(element.find('{DAV:}href').text,
decoding_rounds=2)
etag = prop.find('{DAV:}getetag').text
if href in hrefs:
@ -623,4 +642,4 @@ class CarddavStorage(DavStorage):
continue
hrefs.add(href)
yield self._normalize_href(href), etag
yield href, etag

3
vdirsyncer/storage/filesystem.py
View file

@ -61,6 +61,9 @@ class FilesystemStorage(Storage):
def _get_filepath(self, href):
return os.path.join(self.path, href)
def _get_href(self, item):
return item.ident + self.fileext
def list(self):
for fname in os.listdir(self.path):
fpath = os.path.join(self.path, fname)

3
vdirsyncer/storage/http.py
View file

@ -110,9 +110,8 @@ class HttpStorage(Storage):
for item in split_collection(r.text):
item = Item(item)
href = self._get_href(item)
etag = item.hash
self._items[href] = item, etag
self._items[item.ident] = item, etag
return ((href, etag) for href, (item, etag) in iteritems(self._items))

2
vdirsyncer/storage/memory.py
View file

@ -41,7 +41,7 @@ class MemoryStorage(Storage):
return href in self.items
def upload(self, item):
href = self._get_href(item)
href = item.ident
if href in self.items:
raise exceptions.AlreadyExistingError(item)
etag = _get_etag()

5
vdirsyncer/storage/singlefile.py
View file

@ -96,9 +96,8 @@ class SingleFileStorage(Storage):
for item in split_collection(text):
item = Item(item)
href = self._get_href(item)
etag = item.hash
self._items[href] = item, etag
self._items[item.ident] = item, etag
return ((href, etag) for href, (item, etag) in iteritems(self._items))
@ -112,7 +111,7 @@ class SingleFileStorage(Storage):
raise exceptions.NotFoundError(href)
def upload(self, item):
href = self._get_href(item)
href = item.ident
self.list()
if href in self._items:
raise exceptions.AlreadyExistingError(href)

8
vdirsyncer/utils/compat.py
View file

@ -15,15 +15,15 @@ PY2 = sys.version_info[0] == 2
if PY2: # pragma: no cover
import urlparse
from urllib import \
quote_plus as urlquote_plus, \
unquote_plus as urlunquote_plus
quote as urlquote, \
unquote as urlunquote
text_type = unicode # flake8: noqa
iteritems = lambda x: x.iteritems()
itervalues = lambda x: x.itervalues()
else: # pragma: no cover
import urllib.parse as urlparse
urlquote_plus = urlparse.quote_plus
urlunquote_plus = urlparse.unquote_plus
urlquote = urlparse.quote
urlunquote = urlparse.unquote
text_type = str
iteritems = lambda x: x.items()
itervalues = lambda x: x.values()