Merge lp:~mandel/ubuntuone-client/illegal_windows_chars into lp:ubuntuone-client

[Manuel de la Peña]

Fixes: lp:799707 and lp:799722

This branches fixes two different bugs:

1. The correct credentials management tool is used in the windows IPC.
2. Allowed to use illegal chars to be used in the os_helper so that they can be used in the local file system.

This branches fixes two different bugs:

1. The correct credentials management tool is used in the windows IPC.
2. Allowed to use illegal chars to be used in the os_helper so that they can be used in the local file system. In order to be able to get this branch working pywin32 has to be patch (https://sourceforge.net/tracker/index.php?func=detail&aid=3323058&group_id=78018&atid=551954#) with the following:

diff -r 7dce71d174a9 win32/src/win32security.i
--- a/win32/src/win32security.i Sat Jun 18 10:16:06 2011 -0400
+++ b/win32/src/win32security.i Mon Jun 20 14:15:27 2011 +0200
@@ -2108,7 +2108,7 @@
  if (!PyWinObject_AsTCHAR(obFname, &fname))
   goto done;

- if (GetFileSecurity(fname, info, psd, dwSize, &dwSize)) {
+ if (GetFileSecurityW(fname, info, psd, dwSize, &dwSize)) {
   PyErr_SetString(PyExc_RuntimeError, "Can't query for SECURITY_DESCRIPTOR size info?");
   goto done;
  }
@@ -2117,7 +2117,7 @@
   PyErr_SetString(PyExc_MemoryError, "allocating SECURITY_DESCRIPTOR");
   goto done;
  }
- if (!GetFileSecurity(fname, info, psd, dwSize, &dwSize)) {
+ if (!GetFileSecurityW(fname, info, psd, dwSize, &dwSize)) {
   PyWin_SetAPIError("GetFileSecurity");
   goto done;
  }
@@ -2153,7 +2153,7 @@
  PSECURITY_DESCRIPTOR psd;
  if (!PyWinObject_AsSECURITY_DESCRIPTOR(obsd, &psd))
   goto done;
- if (!SetFileSecurity(fname, info, psd)) {
+ if (!SetFileSecurityW(fname, info, psd)) {
   PyWin_SetAPIError("SetFileSecurity");
   goto done;
  }

Once pywin32 has been patched in your system please run the following test on windows:

python C:\Python27\Scripts u1trial tests\platform\test_os_helper.py
python C:\Python27\Scripts u1trial tests\platform\windows\test_os_helper.py
python C:\Python27\Scripts u1trial tests\syncdaemon\test_vm.py

The changes in the code should have not added too much overhead to the IO operations meaning that the test_vm tests should not timeout.

[Natalia Bidart (nataliabidart)]

* Can you please replace:
        for char in WINDOWS_ILLEGAL_CHARS_MAP:
            self.testfile = self.testfile + char
with:
    self.testfile = ''.join(WINDOWS_ILLEGAL_CHARS_MAP)

* typos: "illechal", "illegla"

* can you please use os.path.splitdrive(path) (http://docs.python.org/library/os.path.html#os.path.splitdrive) instead of the custom logic in diff line 392 (and everywhere else where you split the volume letter)?

* can you replace:

    for illegal_char in WINDOWS_ILLEGAL_CHARS_MAP:
        if illegal_char in path:
            return True

with:

    any(c in WINDOWS_ILLEGAL_CHARS_MAP for c in path)

* can you please replace:

        for illegal_char in WINDOWS_ILLEGAL_CHARS_MAP:
            partial = partial.replace(illegal_char,
                                      WINDOWS_ILLEGAL_CHARS_MAP[illegal_char])

with:

       for illegal_char, replacement in WINDOWS_ILLEGAL_CHARS_MAP.iteritems():
            partial = partial.replace(illegal_char, replacement)

* can you please add a logger.exception statement inside the except OSError, e: within set_no_rights? it will be useful to diagnose potential issues from log reports from our users. Same for remove_file, remove_dir, make_dir, open_file, rename, stat_path.

* why are you doing this in abspath? path = path.replace('/', '\\') Seems unnecessary and may hide bugs in our code regarding multiplatform path management.

review: Needs Fixing

[Alejandro J. Cura (alecu)]

"if e.winerror and e.winerror == 123" should be formulated as """if getattr(e, "winerror", None) == 123""", in case the exception has no winerror attribute. And it should perhaps be moved into a function called let's say "checkWinError123(e)", since it appears at least 8 times through the branch.

calling set_no_rights from inside set_no_rights (in the case of errors) risks the possibility of entering an infinite recursion loop; I suggest moving the three lines inside the try to a small new function, and calling it twice, once from the try, and once from the "if e.winerror...".

typos:
 * "illechal" -> "illegal"
 * "there reason" -> "the reason"
 * spurious spaces have appeared at the end of lines 487 and 496 of the diff
 * "illegla" -> "illegal"

review: Needs Fixing

[Manuel de la Peña (mandel)]

> * Can you please replace:
> for char in WINDOWS_ILLEGAL_CHARS_MAP:
> self.testfile = self.testfile + char
> with:
> self.testfile = ''.join(WINDOWS_ILLEGAL_CHARS_MAP)
>
> * typos: "illechal", "illegla"
>
> * can you please use os.path.splitdrive(path)
> (http://docs.python.org/library/os.path.html#os.path.splitdrive) instead of
> the custom logic in diff line 392 (and everywhere else where you split the
> volume letter)?
>
> * can you replace:
>
> for illegal_char in WINDOWS_ILLEGAL_CHARS_MAP:
> if illegal_char in path:
> return True
>
> with:
>
> any(c in WINDOWS_ILLEGAL_CHARS_MAP for c in path)
>
> * can you please replace:
>
> for illegal_char in WINDOWS_ILLEGAL_CHARS_MAP:
> partial = partial.replace(illegal_char,
> WINDOWS_ILLEGAL_CHARS_MAP[illegal_char])
>
> with:
>
> for illegal_char, replacement in WINDOWS_ILLEGAL_CHARS_MAP.iteritems():
> partial = partial.replace(illegal_char, replacement)
>
> * can you please add a logger.exception statement inside the except OSError,
> e: within set_no_rights? it will be useful to diagnose potential issues from
> log reports from our users. Same for remove_file, remove_dir, make_dir,
> open_file, rename, stat_path.
>

All the above are fixed.

> * why are you doing this in abspath? path = path.replace('/', '\\') Seems
> unnecessary and may hide bugs in our code regarding multiplatform path
> management.

I have left it because if not there is a vm test that fails... the test uses / for the paths, and while we return the correct path, the assertion fails because on windows we use \\. Since verterok is not here I have left it there with a #TODO asking if it is needed.

[Alejandro J. Cura (alecu)]

there are still some instances of the old == 123 check, lines 459 and 638 in the diff

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

* Unless I'm missing something, seems like the custom logic to separate the drive letter has not been replaced by os.path.splitdrive.

* All the logger.exception calls should not add the exception itself to the message string since is automatically added by the logger. So, a sentence like this:

        logger.exception('Goet exception %s when setting path %s to no rights',
                         e, path)

should be:

        logger.exception('Got exception when setting path %s to no rights:', path)

* As Alejandro mentioned, there are some new trailing spaces added by this branch, please remove that.

review: Needs Fixing

[Alejandro J. Cura (alecu)]

Just a few typos:

"Set the rigths" -> "Set the rights"
            ^

"Get exception when triying" -> "Got exception when trying"
  ^ ^

'Got exception when trun to remove path %s'
                      ^ ???

'Got exception when reyting to remove dir %s'
                      ^ retrying?

"TODO: o we really" -> "TODO: do we really"
       ^

review: Approve

[Natalia Bidart (nataliabidart)]

Looks good now!

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~mandel/ubuntuone-client/illegal_windows_chars into lp:ubuntuone-client failed. Below is the output from the failed tests.

/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
  testing autoconf2.50... not found.
  testing autoconf... found 2.67
checking for automake >= 1.10...
  testing automake-1.11... found 1.11.1
checking for libtool >= 1.5...
  testing libtoolize... found 2.2.6b
checking for intltool >= 0.30...
  testing intltoolize... found 0.41.1
checking for pkg-config >= 0.14.0...
  testing pkg-config... found 0.25
checking for gtk-doc >= 1.0...
  testing gtkdocize... found 1.17
Checking for required M4 macros...
Checking for forbidden M4 macros...
Processing ./configure.ac
Running libtoolize...
libtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
Running gtkdocize...
Running aclocal-1.11...
Running autoconf...
Running autoheader...
Running automake-1.11...
Running ./configure --enable-gtk-doc --enable-debug ...
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... i686-pc-linux-gnu
checking host system type... i686-pc-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking whether the shell understands some XSI constructs... yes
checking whether the shell understands "+="... yes
checking for /usr/bin/ld option to reload object files... -r
checking for objdump... objdump
checking how to recognize dependent...

[Ubuntu One Auto Pilot (otto-pilot)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'tests/platform/windows/test_os_helper.py'
--- tests/platform/windows/test_os_helper.py	2011-02-07 15:34:55 +0000
+++ tests/platform/windows/test_os_helper.py	2011-06-21 00:52:37 +0000
@@ -17,115 +17,204 @@
 """Specific tests for the os_helper on Windows."""
 
 import os
-
-from win32api import GetUserName
-
-from ntsecuritycon import (
-    FILE_ALL_ACCESS,
-    FILE_GENERIC_READ,
-    FILE_GENERIC_WRITE,
-)
+import errno
+
 from ubuntuone.platform.windows.os_helper import (
-    _set_file_attributes,
+    _remove_windows_illegal_chars,
     access,
-    set_no_rights
+    listdir,
+    make_dir,
+    open_file,
+    path_exists,
+    remove_dir,
+    remove_file,
+    rename,
+    set_no_rights,
+    set_file_readwrite,
+    stat_path,
+    WINDOWS_ILLEGAL_CHARS_MAP
 )
 from contrib.testing.testcase import BaseTwistedTestCase
 
-class OSWrapperTests(BaseTwistedTestCase):
-    """Test specific windows implementation details."""
+
+class TestIllegalPaths(BaseTwistedTestCase):
+    """Test all the operations using illegal paths."""
     
     def setUp(self):
         """Setup for the tests."""
-        super(OSWrapperTests, self).setUp()
+        super(TestIllegalPaths, self).setUp()
         self.basedir = self.mktemp('test_root')
         self.testfile = os.path.join(self.basedir, "test_file")
-        self.fd = open(self.testfile, 'w')
-
-    def tearDown(self):
-        """Clean the env for the next test."""
-        self.fd.close();
-        self.rmtree(self.basedir)
-        super(OSWrapperTests, self).tearDown()
-
-    def test_access_no_rights(self):
-        """Test when the sid is not present."""
-        # remove all the rights from the test file so that
-        # we cannot read or write
+        self.testfile += ''.join(WINDOWS_ILLEGAL_CHARS_MAP)
+        self.fd = open(_remove_windows_illegal_chars(self.testfile), 'w')
+
+    def test_path_file_exist_yes(self):
+        """Test that the file exists."""
+        self.assertTrue(path_exists(self.testfile))
+
+    def test_path_file_exist_no(self):
+        """Test that the file doesn't exist."""
+        self.fd.close()
+        os.remove(_remove_windows_illegal_chars(self.testfile))
+        self.assertFalse(path_exists(self.testfile))
+
+    def test_path_dir_exist_yes(self):
+        """Test that the dir exists."""
+        self.assertTrue(path_exists(self.basedir))
+
+    def test_path_dir_exist_no(self):
+        """Test that the dir doesn't exist."""
+        self.fd.close()
+        os.remove(_remove_windows_illegal_chars(self.testfile))
+        self.assertFalse(path_exists(os.path.join(self.basedir, 'nodir')))
+
+    def test_remove_file(self):
+        """Test the remove file."""
+        self.fd.close()
+        remove_file(self.testfile)
+        self.assertFalse(os.path.exists(
+                         _remove_windows_illegal_chars(self.testfile)))
+
+    def test_remove_dir(self):
+        """Test the remove dir."""
+        testdir = os.path.join(self.basedir, 'foodir')
+        os.mkdir(testdir)
+        assert os.path.exists(testdir)
+        remove_dir(testdir)
+        self.assertFalse(path_exists(testdir))
+
+    def test_make_dir_one(self):
+        """Test the make dir with one dir."""
+        testdir = os.path.join(self.basedir, 'foodir')
+        assert not os.path.exists(testdir)
+        make_dir(testdir)
+        self.assertTrue(os.path.exists(testdir))
+
+    def test_make_dir_already_there(self):
+        """Test the make dir with one dir that exists."""
+        self.assertRaises(OSError, make_dir, self.basedir)
+
+    def test_make_dir_recursive_no(self):
+        """Test the make dir with some dirs, not recursive explicit."""
+        testdir = os.path.join(self.basedir, 'foo', 'bar')
+        assert not os.path.exists(testdir)
+        self.assertRaises(OSError, make_dir, testdir)
+
+    def test_make_dir_recursive_yes(self):
+        """Test the make dir with some dirs, recursive."""
+        testdir = os.path.join(self.basedir, 'foo', 'bar')
+        assert not os.path.exists(testdir)
+        make_dir(testdir, recursive=True)
+        self.assertTrue(os.path.exists(testdir))
+
+    def test_open_file_not_there(self):
+        """Open a file that does not exist."""
+        self.assertRaises(IOError, open_file, os.path.join(self.basedir, 'no'))
+
+    def test_open_file_gets_a_fileobject(self):
+        """Open a file, and get a file object."""
+        testfile = os.path.join(self.basedir, 'testfile')
+        open(testfile, 'w').close()
+        f = open_file(testfile)
+        self.assertTrue(isinstance(f, file))
+
+    def test_open_file_read(self):
+        """Open a file, and read."""
+        testfile = os.path.join(self.basedir, 'testfile')
+        with open(testfile, 'w') as fh:
+            fh.write("foo")
+        f = open_file(testfile, 'r')
+        self.assertTrue(f.read(), "foo")
+
+    def test_open_file_write(self):
+        """Open a file, and write."""
+        testfile = os.path.join(self.basedir, 'testfile')
+        with open_file(testfile, 'w') as fh:
+            fh.write("foo")
+        f = open(testfile)
+        self.assertTrue(f.read(), "foo")
+
+    def test_rename_not_there(self):
+        """Rename something that does not exist."""
+        self.assertRaises(OSError, rename,
+                          os.path.join(self.basedir, 'no>'), 'foo')
+
+    def test_rename_file(self):
+        """Rename a file."""
+        testfile1 = os.path.join(self.basedir, 'testfile1>')
+        testfile2 = os.path.join(self.basedir, 'testfile2<')
+        open(_remove_windows_illegal_chars(testfile1), 'w').close()
+        rename(testfile1, testfile2)
+        self.assertFalse(os.path.exists(
+                         _remove_windows_illegal_chars(testfile1)))
+        self.assertTrue(os.path.exists(
+                         _remove_windows_illegal_chars(testfile2)))
+
+    def test_rename_dir(self):
+        """Rename a dir."""
+        testdir1 = os.path.join(self.basedir, 'testdir1>')
+        testdir2 = os.path.join(self.basedir, 'testdir2<')
+        os.mkdir(_remove_windows_illegal_chars(testdir1))
+        rename(testdir1, testdir2)
+        self.assertFalse(os.path.exists(
+                         _remove_windows_illegal_chars(testdir1)))
+        self.assertTrue(os.path.exists(
+                         _remove_windows_illegal_chars(testdir2)))
+
+    def test_listdir(self):
+        """Return a list of the files in a dir."""
+        open(os.path.join(self.basedir, 'foo'), 'w').close()
+        open(os.path.join(self.basedir, 'bar'), 'w').close()
+        l = listdir(self.basedir)
+        test_file = u'test_file\u200b\u2033\u200b\u200b\u203d\u200b\u200b' +\
+                    u'\u26b9\u200b\u200b\u2223\u200b\u200b\u2236\u200b\u200b' +\
+                    u'\u2039\u200b\u200b\u2044\u200b\u200b\u203a\u200b'
+        self.assertEqual(sorted(l), ['bar', 'foo', test_file])
+
+    def test_access_rw(self):
+        """Test access on a file with full permission."""
+        self.assertTrue(access(self.testfile))
+
+    def test_access_ro(self):
+        """Test access on a file with read only permission."""
+        fixed_path = _remove_windows_illegal_chars(self.testfile)
+        os.chmod(fixed_path, 0o444)
+        self.assertTrue(access(self.testfile))
+        os.chmod(fixed_path, 0o664)
+
+    def test_access_nothing(self):
+        """Test access on a file with no permission at all."""
         set_no_rights(self.testfile)
         self.assertFalse(access(self.testfile))
-
-    def test_access_read_write_user(self):
-        """Test when the user sid has rw rights."""
-        # set the file to be read and write just by the user
-        groups = {}
-        groups[GetUserName()] = FILE_GENERIC_READ | FILE_GENERIC_WRITE
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-
-    def test_access_read_write_everyone(self):
-        """Test when the everyone sid has rw rights."""
-        groups = {}
-        groups['Everyone'] = FILE_GENERIC_READ | FILE_GENERIC_WRITE
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-        
-    def test_access_write_user_everyone_read(self):
-        """Test when the user sid has w rights."""
-        groups = {}
-        groups[GetUserName()] = FILE_GENERIC_WRITE
-        groups['Everyone'] = FILE_GENERIC_READ
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-
-    def test_access_write_everyone_user_read(self):
-        """Test when the everyone sid has w rights"""
-        groups = {}
-        groups[GetUserName()] = FILE_GENERIC_READ 
-        groups['Everyone'] = FILE_GENERIC_WRITE
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-
-    def test_access_write_user_everyone(self):
-        """Test when everyone and user have w rights."""
-        groups = {}
-        groups[GetUserName()] = FILE_GENERIC_WRITE
-        groups['Everyone'] = FILE_GENERIC_WRITE
-        _set_file_attributes(self.testfile, groups)
-        self.assertFalse(access(self.testfile))
-
-    def test_access_read_user(self):
-        """Test when the sid has r rights."""
-        groups = {}
-        groups[GetUserName()] = FILE_GENERIC_READ
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-
-    def test_access_read_everyone(self):
-        """Test when everyone has r rights."""
-        groups = {}
-        groups['Everyone'] = FILE_GENERIC_READ
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-    
-    def test_access_read_user_everyone(self):
-        """Test when user and everyone have r rights."""
-        groups = {}
-        groups[GetUserName()] = FILE_GENERIC_READ
-        groups['Everyone'] = FILE_GENERIC_READ
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-
-    def test_access_full_user(self):
-        """Test when the sid has full control."""
-        groups = {}
-        groups[GetUserName()] = FILE_ALL_ACCESS
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
-
-    def test_access_full_everyone(self):
-        """test when everyone has full control."""
-        groups = {}
-        groups['Everyone'] = FILE_ALL_ACCESS
-        _set_file_attributes(self.testfile, groups)
-        self.assertTrue(access(self.testfile))
+        set_file_readwrite(self.testfile)
+
+    def test_stat_normal(self):
+        """Test on a normal file."""
+        self.assertEqual(os.stat(_remove_windows_illegal_chars(self.testfile)),
+                         stat_path(self.testfile))
+
+    def test_stat_no_path(self):
+        """Test that it raises proper error when no file is there."""
+        try:
+            return stat_path(os.path.join(self.basedir, 'nofile'))
+        except OSError, e:
+            self.assertEqual(e.errno, errno.ENOENT)
+
+    def test_path_exists_file_yes(self):
+        """The file is there."""
+        self.assertTrue(path_exists(self.testfile))
+
+    def test_path_exists_file_no(self):
+        """The file is not there."""
+        self.fd.close()
+        os.remove(_remove_windows_illegal_chars(self.testfile))
+        self.assertFalse(path_exists(self.testfile))
+
+    def test_path_exists_dir_yes(self):
+        """The dir is there."""
+        self.assertTrue(path_exists(self.basedir))
+
+    def test_path_exists_dir_no(self):
+        """The dir is not there."""
+        self.fd.close()
+        self.assertFalse(path_exists(os.path.join(self.basedir, 'subdir')))
 \ No newline at end of file
 
=== modified file 'ubuntuone/platform/windows/ipc.py'
--- ubuntuone/platform/windows/ipc.py	2011-06-15 14:46:04 +0000
+++ ubuntuone/platform/windows/ipc.py	2011-06-21 00:52:37 +0000
@@ -58,8 +58,7 @@
 from twisted.spread.pb import Referenceable, Root, PBServerFactory
 from twisted.python.failure import Failure
 from ubuntuone.syncdaemon.interfaces import IMarker
-from ubuntuone.credentials import CredentialsManagementTool
-from ubuntuone.platform import get_creds_proxy
+from ubuntuone.platform.windows import CredentialsManagementTool
 from ubuntuone.platform.windows.network_manager import NetworkManager
 from ubuntuone.syncdaemon.interaction_interfaces import (
     bool_str,
@@ -1243,7 +1242,7 @@
 
     def _register_to_signals(self):
         """Register to the creds signals."""
-        self.creds = get_creds_proxy()
+        self.creds = CredentialsManagementTool()
         self.creds.register_to_credentials_found(self.on_credentials_found_cb)
         self.creds.register_to_credentials_not_found(
                                                  self.on_credentials_error_cb)
 
=== modified file 'ubuntuone/platform/windows/os_helper.py'
--- ubuntuone/platform/windows/os_helper.py	2011-06-17 09:31:15 +0000
+++ ubuntuone/platform/windows/os_helper.py	2011-06-21 00:52:37 +0000
@@ -26,8 +26,8 @@
 from functools import wraps
 from contextlib import contextmanager
 from pywintypes import error as PyWinError
-from win32api import MoveFileEx, GetUserName, GetFileAttributes
-from win32file import FindFilesW
+from win32api import GetUserName, GetFileAttributes
+from win32file import FindFilesW, MoveFileExW
 from win32com.client import Dispatch
 from pywintypes import error
 from win32con import FILE_ATTRIBUTE_READONLY
@@ -58,11 +58,36 @@
 if sys.platform != 'win32':
     WindowsError = None
 
+
+logger = logging.getLogger('ubuntuone.SyncDaemon.VM')
 platform = 'win32'
 
 LONG_PATH_PREFIX = '\\\\?\\'
 EVERYONE_GROUP = 'Everyone'
 ADMINISTRATORS_GROUP = 'Administrators'
+# a map that contains the illegal chars and their 'utf8' representation on
+# windows so that we can show something to the user
+WINDOWS_ILLEGAL_CHARS_MAP = {
+    '<':u'\N{ZERO WIDTH SPACE}\N{SINGLE LEFT-POINTING ANGLE QUOTATION MARK}\N{ZERO WIDTH SPACE}',
+    '>':u'\N{ZERO WIDTH SPACE}\N{SINGLE RIGHT-POINTING ANGLE QUOTATION MARK}\N{ZERO WIDTH SPACE}',
+    ':':u'\N{ZERO WIDTH SPACE}\N{RATIO}\N{ZERO WIDTH SPACE}',
+    '"':u'\N{ZERO WIDTH SPACE}\N{DOUBLE PRIME}\N{ZERO WIDTH SPACE}',
+    '/':u'\N{ZERO WIDTH SPACE}\N{FRACTION SLASH}\N{ZERO WIDTH SPACE}',
+    '|':u'\N{ZERO WIDTH SPACE}\N{DIVIDES}\N{ZERO WIDTH SPACE}',
+    '?':u'\N{ZERO WIDTH SPACE}\N{INTERROBANG}\N{ZERO WIDTH SPACE}',
+    '*':u'\N{ZERO WIDTH SPACE}\N{SEXTILE}\N{ZERO WIDTH SPACE}'
+}
+LINUX_ILLEGAL_CHARS_MAP = {
+    u'\N{ZERO WIDTH SPACE}\N{SINGLE LEFT-POINTING ANGLE QUOTATION MARK}\N{ZERO WIDTH SPACE}':'<',
+    u'\N{ZERO WIDTH SPACE}\N{SINGLE RIGHT-POINTING ANGLE QUOTATION MARK}\N{ZERO WIDTH SPACE}':'>',
+    u'\N{ZERO WIDTH SPACE}\N{RATIO}\N{ZERO WIDTH SPACE}':':',
+    u'\N{ZERO WIDTH SPACE}\N{DOUBLE PRIME}\N{ZERO WIDTH SPACE}':'"',
+    u'\N{ZERO WIDTH SPACE}\N{FRACTION SLASH}\N{ZERO WIDTH SPACE}':'/',
+    u'\N{ZERO WIDTH SPACE}\N{SET MINUS}\N{ZERO WIDTH SPACE}':'\\',
+    u'\N{ZERO WIDTH SPACE}\N{DIVIDES}\N{ZERO WIDTH SPACE}':'|',
+    u'\N{ZERO WIDTH SPACE}\N{INTERROBANG}\N{ZERO WIDTH SPACE}':'?',
+    u'\N{ZERO WIDTH SPACE}\N{SEXTILE}\N{ZERO WIDTH SPACE}':'*'
+}
 
 
 def _int_to_bin(n):
@@ -83,9 +108,79 @@
     return LookupAccountName('', group_name)[0]
 
 
+def _has_read_mask(number):
+    """Return if the read flag is present."""
+    # get the bin representation of the mask
+    binary = _int_to_bin(number)
+    # there is actual no documentation of this in MSDN but if bt 28 is set,
+    # the mask has full access, more info can be found here:
+    # http://www.iu.hio.no/cfengine/docs/cfengine-NT/node47.html
+    if binary[28] == '1':
+        return True
+    # there is no documentation in MSDN about this, but if bit 0 and 3 are true
+    # we have the read flag, more info can be found here:
+    # http://www.iu.hio.no/cfengine/docs/cfengine-NT/node47.html
+    return binary[0] == '1' and binary[3] == '1'
+
+
+def _check_winerror_123(exception, cb, path, *args, **kwargs):
+    """Checks if an exception is a 123 error and executes the cb."""
+    if getattr(exception, "winerror", None) == 123 :
+        path = _remove_windows_illegal_chars(path)
+        return cb(path, *args, **kwargs)
+    else:
+        raise exception
+
+
+def _is_illegal_path(path):
+    """Return if the path is illegal in the system."""
+    if LONG_PATH_PREFIX in path:
+        path = path.replace(LONG_PATH_PREFIX, '')
+    return any(c in WINDOWS_ILLEGAL_CHARS_MAP
+               for c in os.path.splitdrive(path)[1])
+
+
+def _remove_windows_illegal_chars(path):
+    """Remove illegal chars and return a valid windows path."""
+    if not _is_illegal_path(path):
+        return path
+    prefix = False
+    # do not split the path if you have a long path prefix since the
+    # partial paths will be wrong.
+    if LONG_PATH_PREFIX in path:
+        # remove it and remember to add it
+        path = path.replace(LONG_PATH_PREFIX, '')
+        prefix = True
+    volume, path = os.path.splitdrive(path)
+    partial_paths = path.split('\\')
+    result = []
+    for partial in partial_paths:
+        # ensure that the illegal chars are not there
+        for illegal_char, replacement in WINDOWS_ILLEGAL_CHARS_MAP.iteritems():
+            partial = partial.replace(illegal_char, replacement)
+        result.append(partial)
+    result = os.path.join(*result)
+    if volume:
+        result = volume + '\\' + result
+    if prefix:
+        result = LONG_PATH_PREFIX + result
+    return unicode(result)
+
 def _set_file_attributes(path, groups):
     """Set file attributes using the wind32api."""
-    security_descriptor = GetFileSecurity(path, DACL_SECURITY_INFORMATION)
+    security_descriptor = None
+    try:
+        security_descriptor = GetFileSecurity(path, DACL_SECURITY_INFORMATION)
+    except PyWinError, e:
+        logger.exception('Got exception when setting file attributes %s ' +
+                         'for path %s', path, groups)
+        #check if the issue was due to an illegal path
+        if getattr(e, "winerror", None) == 123:
+            path = _remove_windows_illegal_chars(path)
+            return _set_file_attributes(path, groups)
+        else:
+            raise
+
     dacl = ACL()
     for group_name in groups:
         # set the attributes of the group only if not null
@@ -94,30 +189,15 @@
             dacl.AddAccessAllowedAceEx(ACL_REVISION,
                 CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, groups[group_name],
                 group_sid)
-    # the dacl has all the info of the dff groups passed in the parameters
+    # the dacl has all the info of the diff groups passed in the parameters
     security_descriptor.SetSecurityDescriptorDacl(1, dacl, 0)
     SetFileSecurity(path, DACL_SECURITY_INFORMATION, security_descriptor)
 
 
-def _has_read_mask(number):
-    """Return if the read flag is present."""
-    # get the bin representation of the mask
-    binary = _int_to_bin(number)
-    # there is actual no documentation of this in MSDN but if bt 28 is set,
-    # the mask has full access, more info can be found here:
-    # http://www.iu.hio.no/cfengine/docs/cfengine-NT/node47.html
-    if binary[28] == '1':
-        return True
-    # there is no documentation in MSDN about this, but if bit 0 and 3 are true
-    # we have the read flag, more info can be found here:
-    # http://www.iu.hio.no/cfengine/docs/cfengine-NT/node47.html
-    return binary[0] == '1' and binary[3] == '1'
-
-
 def absparam(paths_indexes=[], paths_names=[]):
     """Ensure that the paths are absolute."""
     def decorator(function):
-        """Decorate the funtion to make sure the paths are absolute."""
+        """Decorate the function to make sure the paths are absolute."""
         @wraps(function)
         def abspath_wrapper(*args, **kwargs):
             """Set the paths to be absolute."""
@@ -131,7 +211,7 @@
                     kwargs[current_key] = \
                         os.path.abspath(kwargs[current_key])
                 except KeyError:
-                    logging.warn('Patameter %s could not be made a long path',
+                    logger.warn('Parameter %s could not be made a long path',
                         current_key)
             return function(*fixed_args, **kwargs)
         return abspath_wrapper
@@ -155,26 +235,36 @@
                     kwargs[current_key] = \
                         _set_as_long_path(kwargs[current_key])
                 except KeyError:
-                    logging.warn('Patameter %s could not be made a long path',
+                    logger.warn('Parameter %s could not be made a long path',
                         current_key)
             return function(*fixed_args, **kwargs)
         return long_path_wrapper
     return decorator
 
 
+def _set_no_rights_helper(path):
+    """Set the rights to be none."""
+    os.chmod(path, 0o000)
+    groups = {}
+    _set_file_attributes(path, groups)
+
+
 @longpath(paths_indexes=[0])
 def set_no_rights(path):
     # set the groups to be empty which will remove all the rights of the file
-    os.chmod(path, 0o000)
-    groups = {}
-    _set_file_attributes(path, groups)
+    try:
+        _set_no_rights_helper(path)
+    except OSError, e:
+        logger.exception('Got exception when setting path %s to no rights',
+                         path)
+        return _check_winerror_123(e, _set_no_rights_helper, path)
 
 
 @longpath(paths_indexes=[0])
 def set_file_readonly(path):
     """Change path permissions to readonly in a file."""
     # we use the win32 api because chmod just sets the readonly flag and
-    # we want to have imore control over the permissions
+    # we want to have more control over the permissions
     groups = {}
     groups[ADMINISTRATORS_GROUP] = FILE_GENERIC_READ | FILE_GENERIC_WRITE
     groups[GetUserName()] = FILE_GENERIC_READ
@@ -191,7 +281,12 @@
     groups[GetUserName()] = FILE_ALL_ACCESS
     # the above equals more or less to 0774
     _set_file_attributes(path, groups)
-    os.chmod(path, stat.S_IWRITE)
+    try:
+        os.chmod(path, stat.S_IWRITE)
+    except OSError, e:
+        logger.exception('Get exception when trying to set readwrite ' +
+                         'path %s', path)
+        return _check_winerror_123(e, os.chmod, path, stat.S_IWRITE)
 
 
 @longpath(paths_indexes=[0])
@@ -229,7 +324,7 @@
 @contextmanager
 @longpath(paths_indexes=[0])
 def allow_writes(path):
-    """A very simple context manager to allow writting in RO dirs."""
+    """A very simple context manager to allow witting in RO dirs."""
     # get the old dacl of the file so that we can reset it back when done
     security_descriptor = GetFileSecurity(path, DACL_SECURITY_INFORMATION)
     old_dacl = security_descriptor.GetSecurityDescriptorDacl()
@@ -243,18 +338,28 @@
 @longpath(paths_indexes=[0])
 def remove_file(path):
     """Remove a file."""
-    os.remove(path)
+    try:
+        os.remove(path)
+    except OSError, e:
+        logger.exception('Got exception when trying to remove path %s', path)
+        return _check_winerror_123(e, os.remove, path)
 
 
 @longpath(paths_indexes=[0])
 def remove_dir(path):
     """Remove a dir."""
-    os.rmdir(path)
+    try:
+        os.rmdir(path)
+    except OSError, e:
+        logger.exception('Got exception when trying to remove dir %s', path)
+        return _check_winerror_123(e, os.rmdir, path)
 
 
 @longpath(paths_indexes=[0])
 def path_exists(path):
     """Return if the path exists."""
+    if _is_illegal_path(path):
+        path = _remove_windows_illegal_chars(path)
     return os.path.exists(path)
 
 
@@ -264,37 +369,60 @@
     if recursive:
         try:
             os.makedirs(path)
-        except WindowsError, e:
-            raise OSError(str(e))
+        except OSError, e:
+            logger.exception('Got exception when trying to make dirs %s', path)
+            return _check_winerror_123(e, os.makedirs, path)
     else:
         try:
             os.mkdir(path)
-        except WindowsError, e:
-            raise OSError(str(e))
+        except OSError, e:
+            logger.exception('Got exception when trying to make dirs %s', path)
+            return _check_winerror_123(e, os.mkdir, path)
 
 
 @longpath(paths_indexes=[0])
 def open_file(path, mode='r'):
     """Open a file."""
-    return open(path, mode)
+    try:
+        return open(path, mode)
+    except OSError, e:
+        logger.exception('Got exception when trying to open path %s', path)
+        return _check_winerror_123(e, open, path, mode)
 
 
 @absparam(paths_indexes=[0, 1])
 @longpath(paths_indexes=[0, 1])
 def rename(path_from, path_to):
     """Rename a file or directory."""
-    # to ensure the same behavious as on linux, use the MoveEx function from
-    # win32 which will allow to replace the destionation path if exists and
+    # to ensure the same behaviors as on linux, use the MoveEx function from
+    # win32 which will allow to replace the destination path if exists and
     # the user has the rights see:
     # http://msdn.microsoft.com/en-us/library/aa365240(v=vs.85).aspx
     try:
-        MoveFileEx(
+        MoveFileExW(
             path_from, path_to,
             MOVEFILE_COPY_ALLOWED |
             MOVEFILE_REPLACE_EXISTING |
             MOVEFILE_WRITE_THROUGH)
     except PyWinError, e:
-        raise OSError(str(e))
+        logger.exception('Got exception when trying to rename from ' +
+                         '%s to %s', path_from, path_to)
+        #check if the issue was due to an illegal path
+        if getattr(e, "winerror", None) == 123:
+            path_from = _remove_windows_illegal_chars(path_from)
+            path_to = _remove_windows_illegal_chars(path_to)
+            try:
+                MoveFileExW(
+                    path_from, path_to,
+                    MOVEFILE_COPY_ALLOWED |
+                    MOVEFILE_REPLACE_EXISTING |
+                    MOVEFILE_WRITE_THROUGH)
+            except PyWinError, e:
+                logger.exception('Got exception when trying to rename' +
+                                 ' from %s to %s', path_from, path_to)
+                raise OSError(str(e))
+        else:
+            raise OSError(str(e))
 
 
 @absparam(paths_indexes=[0, 1])
@@ -343,7 +471,7 @@
 @longpath(paths_indexes=[0])
 def listdir(directory):
     """List a directory."""
-    # os.listdir combines / and \ in its search which breacks things on
+    # os.listdir combines / and \ in its search which breaks things on
     # windows, we use the win32 api instead.
     result = []
     for file_info in FindFilesW(os.path.join(directory, '*.*')):
@@ -357,8 +485,12 @@
 @longpath(paths_indexes=[0])
 def access(path):
     """Return if the path is at least readable."""
+    # lets consider the access on an illegal path to be a special case
+    # since that will only occur in the case where the user created the path
     # for a file to be readable it has to be readable either by the user or
     # by the everyone group
+    if _is_illegal_path(path):
+        path = _remove_windows_illegal_chars(path)
     if not os.path.exists(path):
         return False
     security_descriptor = GetFileSecurity(path, DACL_SECURITY_INFORMATION)
@@ -380,13 +512,14 @@
             # in our case we can ignore this since we are looking for visible
             # users
             continue
-    return (GetUserName() in accounts or EVERYONE_GROUP in accounts)
+    return (GetUserName() in accounts or EVERYONE_GROUP in accounts) and\
+            os.access(path, os.R_OK)
 
 
 @longpath(paths_indexes=[0])
 def can_write(path):
     """Return if the path can be written to."""
-    file_att = GetFileAttributes(path)  
+    file_att = GetFileAttributes(path)
     if file_att and FILE_ATTRIBUTE_READONLY:
         return False
     return True
@@ -395,14 +528,18 @@
 def stat_path(path, look_for_link=True):
     """Return stat info about a path."""
     # if the path end with .lnk, that means we are dealing with a link
-    # and we should return the stat of th target path
+    # and we should return the stat of the target path
     if path.endswith('.lnk'):
         shell = Dispatch('WScript.Shell')
         shortcut = shell.CreateShortCut(path)
         path = shortcut.Targetpath
     if look_for_link and os.path.exists(path + '.lnk'):
         return stat_path(path + '.lnk')
-    return os.lstat(path)
+    try:
+        return os.lstat(path)
+    except OSError, e:
+        logger.exception('Got exception when trying to stat path %s', path)
+        return _check_winerror_123(e, stat_path, path, look_for_link)
 
 
 def move_to_trash(path):
@@ -415,7 +552,7 @@
 def set_application_name(app_name):
     """Set the app name."""
     # there is not way to do this on windows. The name will be correct when
-    # executed from a bundle .exe otherwhise it will be python
+    # executed from a bundle .exe otherwise it will be python
 
 
 def is_root():
@@ -432,7 +569,7 @@
 
 
 def validate_path_from_unix(path):
-    """Get a unix path and return it so that it cna be used."""
+    """Get a unix path and return it so that it can be used."""
     path = path.replace('/', '\\')
     return path
 
@@ -449,7 +586,9 @@
 
 def abspath(path):
     """Return an abspath."""
-    abs = os.path.abspath(path)
+    # TODO: do we really need to do this, are the tests broken?
+    path = path.replace('/', '\\')
+    abs = os.path.abspath(_remove_windows_illegal_chars(path))
     if not LONG_PATH_PREFIX in abs:
         abs = LONG_PATH_PREFIX + abs
     return abs
 \ No newline at end of file