Skip to content
This repository

check for writable dirs, not just existence, in utils.path #670

Merged
merged 4 commits into from over 2 years ago

2 participants

Min RK Fernando Perez
Min RK
Owner

replaces various calls to os.path.isdir and os.path.exists with _writable_dir, which checks existence and write-permission.

should close #669

Min RK check for writable dirs, not just existence, in utils.path
replaces various calls to os.path.isdir and os.path.exists with _writable_dir.

closes gh-669
b1ec990
IPython/utils/path.py
@@ -166,7 +170,7 @@ def get_home_dir():
166 170
     raised for all other OSes.
167 171
     """
168 172
 
169  
-    isdir = os.path.isdir
  173
+    isdir = _writable_dir
1
Fernando Perez Owner
fperez added a note August 03, 2011

I'd use the name is_valid_dir to avoid getting confused later down in the code by thinking that isdir is just an alias to os.path.isdir when in reality it's a wrapper with additional specific functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Fernando Perez
Owner

Could you add a test for this? I think simply creating a directory without write permissions and then trying to use it as IPYTHON_DIR should be very easy to do. But it will help ensure that our error checks remain valid accross OSes. That kind of thing sometimes has funny platform variability, so I'd feel better with an actual test that buttresses this code.

Thanks!

added some commits August 03, 2011
Min RK don't use isdir=_writable_dir alias 1321ca2
Min RK use tempdir if no usable ipython_dir is found
add test for gh-669, and ignore nonexistent toy paths in other path tests.
2a8de3c
Min RK
Owner

test added, and tempdir behavior implemented. The tempdir behavior meant I had to tweak many other tests, since they use toy non-existent directories.

Due to the test changes, wait for a Windows test to merge.

Fernando Perez
Owner

Sorry Min, what do you mean by "wait for a Windows test to merge"? That you are going to do some Win testing yourself and don't want the merge to be done before that? Or that you need some help with Windows-specific testing?

Min RK
Owner

Yes, I just mean wait until I run the test suite on a Windows machine. Since I had to make some changes to existing tests, it's possible that one or more of the Windows-only tests will also require a change.

Min RK Skip writable_dir test on Windows
It doesn't seem possible to create a dir that the owner can't write to
on Windows, so the test doesn't run properly.  If the test can be
updated to reliably create/find a user read-only directory (probably via
NTFS ACLs and win32security), then it can be unskipped.
28c77fe
Min RK
Owner

I've had to skip the new test on Windows, because I can't figure out how to create a read-only dir on Windows. Otherwise, it still passes.

Fernando Perez
Owner

Looks good, thanks! Merging now.

Fernando Perez fperez merged commit 28c77fe into from August 11, 2011
Fernando Perez fperez closed this August 11, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 4 unique commits by 1 author.

Aug 03, 2011
Min RK check for writable dirs, not just existence, in utils.path
replaces various calls to os.path.isdir and os.path.exists with _writable_dir.

closes gh-669
b1ec990
Min RK don't use isdir=_writable_dir alias 1321ca2
Min RK use tempdir if no usable ipython_dir is found
add test for gh-669, and ignore nonexistent toy paths in other path tests.
2a8de3c
Aug 04, 2011
Min RK Skip writable_dir test on Windows
It doesn't seem possible to create a dir that the owner can't write to
on Windows, so the test doesn't run properly.  If the test can be
updated to reliably create/find a user read-only directory (probably via
NTFS ACLs and win32security), then it can be unskipped.
28c77fe
This page is out of date. Refresh to see the latest.
38  IPython/utils/path.py
@@ -16,6 +16,7 @@
16 16
 
17 17
 import os
18 18
 import sys
  19
+import tempfile
19 20
 from hashlib import md5
20 21
 
21 22
 import IPython
@@ -41,6 +42,10 @@ def _get_long_path_name(path):
41 42
     """Dummy no-op."""
42 43
     return path
43 44
 
  45
+def _writable_dir(path):
  46
+    """Whether `path` is a directory, to which the user has write access."""
  47
+    return os.path.isdir(path) and os.access(path, os.W_OK)
  48
+
44 49
 if sys.platform == 'win32':
45 50
     def _get_long_path_name(path):
46 51
         """Get a long path name (expand ~) on Windows using ctypes.
@@ -166,7 +171,6 @@ def get_home_dir():
166 171
     raised for all other OSes.
167 172
     """
168 173
 
169  
-    isdir = os.path.isdir
170 174
     env = os.environ
171 175
 
172 176
     # first, check py2exe distribution root directory for _ipython.
@@ -178,7 +182,7 @@ def get_home_dir():
178 182
         else: 
179 183
             root=os.path.join(os.path.split(IPython.__file__)[0],"../../")
180 184
         root=os.path.abspath(root).rstrip('\\')
181  
-        if isdir(os.path.join(root, '_ipython')):
  185
+        if _writable_dir(os.path.join(root, '_ipython')):
182 186
             os.environ["IPYKITROOT"] = root
183 187
         return _cast_unicode(root, fs_encoding)
184 188
 
@@ -212,7 +216,7 @@ def get_home_dir():
212 216
         except KeyError:
213 217
             pass
214 218
         else:
215  
-            if isdir(homedir):
  219
+            if _writable_dir(homedir):
216 220
                 return _cast_unicode(homedir, fs_encoding)
217 221
 
218 222
         # Now look for a local home directory
@@ -221,7 +225,7 @@ def get_home_dir():
221 225
         except KeyError:
222 226
             pass
223 227
         else:
224  
-            if isdir(homedir):
  228
+            if _writable_dir(homedir):
225 229
                 return _cast_unicode(homedir, fs_encoding)
226 230
 
227 231
         # Now the users profile directory
@@ -230,7 +234,7 @@ def get_home_dir():
230 234
         except KeyError:
231 235
             pass
232 236
         else:
233  
-            if isdir(homedir):
  237
+            if _writable_dir(homedir):
234 238
                 return _cast_unicode(homedir, fs_encoding)
235 239
 
236 240
         # Use the registry to get the 'My Documents' folder.
@@ -245,7 +249,7 @@ def get_home_dir():
245 249
         except:
246 250
             pass
247 251
         else:
248  
-            if isdir(homedir):
  252
+            if _writable_dir(homedir):
249 253
                 return _cast_unicode(homedir, fs_encoding)
250 254
 
251 255
         # A user with a lot of unix tools in win32 may have defined $HOME.
@@ -255,7 +259,7 @@ def get_home_dir():
255 259
         except KeyError:
256 260
             pass
257 261
         else:
258  
-            if isdir(homedir):
  262
+            if _writable_dir(homedir):
259 263
                 return _cast_unicode(homedir, fs_encoding)
260 264
 
261 265
         # If all else fails, raise HomeDirError
@@ -272,14 +276,13 @@ def get_xdg_dir():
272 276
     This is only for posix (Linux,Unix,OS X, etc) systems.
273 277
     """
274 278
 
275  
-    isdir = os.path.isdir
276 279
     env = os.environ
277 280
     
278 281
     if os.name == 'posix':
279 282
         # Linux, Unix, AIX, OS X
280 283
         # use ~/.config if not set OR empty
281 284
         xdg = env.get("XDG_CONFIG_HOME", None) or os.path.join(get_home_dir(), '.config')
282  
-        if xdg and isdir(xdg):
  285
+        if xdg and _writable_dir(xdg):
283 286
             return _cast_unicode(xdg, fs_encoding)
284 287
     
285 288
     return None
@@ -294,7 +297,7 @@ def get_ipython_dir():
294 297
     
295 298
     env = os.environ
296 299
     pjoin = os.path.join
297  
-    exists = os.path.exists
  300
+    
298 301
     
299 302
     ipdir_def = '.ipython'
300 303
     xdg_def = 'ipython'
@@ -312,7 +315,7 @@ def get_ipython_dir():
312 315
             
313 316
             xdg_ipdir = pjoin(xdg_dir, xdg_def)
314 317
         
315  
-            if exists(xdg_ipdir) or not exists(home_ipdir):
  318
+            if _writable_dir(xdg_ipdir) or not _writable_dir(home_ipdir):
316 319
                 ipdir = xdg_ipdir
317 320
         
318 321
         if ipdir is None:
@@ -320,6 +323,19 @@ def get_ipython_dir():
320 323
             ipdir = home_ipdir
321 324
 
322 325
     ipdir = os.path.normpath(os.path.expanduser(ipdir))
  326
+    
  327
+    if os.path.exists(ipdir) and not _writable_dir(ipdir):
  328
+        # ipdir exists, but is not writable
  329
+        warn.warn("IPython dir '%s' is not a writable location,"
  330
+                        " using a temp directory."%ipdir)
  331
+        ipdir = tempfile.mkdtemp()
  332
+    elif not os.path.exists(ipdir):
  333
+        parent = ipdir.rsplit(os.path.sep, 1)[0]
  334
+        if not _writable_dir(parent):
  335
+            # ipdir does not exist and parent isn't writable
  336
+            warn.warn("IPython parent '%s' is not a writable location,"
  337
+                        " using a temp directory."%parent)
  338
+            ipdir = tempfile.mkdtemp()
323 339
 
324 340
     return _cast_unicode(ipdir, fs_encoding)
325 341
 
43  IPython/utils/tests/test_path.py
@@ -16,6 +16,7 @@
16 16
 import shutil
17 17
 import sys
18 18
 import tempfile
  19
+import StringIO
19 20
 
20 21
 from os.path import join, abspath, split
21 22
 
@@ -26,7 +27,7 @@
26 27
 import IPython
27 28
 from IPython.testing import decorators as dec
28 29
 from IPython.testing.decorators import skip_if_not_win32, skip_win32
29  
-from IPython.utils import path
  30
+from IPython.utils import path, io
30 31
 
31 32
 # Platform-dependent imports
32 33
 try:
@@ -92,7 +93,8 @@ def teardown_environment():
92 93
     """Restore things that were remebered by the setup_environment function
93 94
     """
94 95
     (oldenv, os.name, path.get_home_dir, IPython.__file__,) = oldstuff
95  
-        
  96
+    reload(path)
  97
+    
96 98
     for key in env.keys():
97 99
         if key not in oldenv:
98 100
             del env[key]
@@ -229,6 +231,7 @@ def QueryValueEx(x, y):
229 231
 def test_get_ipython_dir_1():
230 232
     """test_get_ipython_dir_1, Testcase to see if we can call get_ipython_dir without Exceptions."""
231 233
     env_ipdir = os.path.join("someplace", ".ipython")
  234
+    path._writable_dir = lambda path: True
232 235
     env['IPYTHON_DIR'] = env_ipdir
233 236
     ipdir = path.get_ipython_dir()
234 237
     nt.assert_equal(ipdir, env_ipdir)
@@ -238,6 +241,8 @@ def test_get_ipython_dir_1():
238 241
 def test_get_ipython_dir_2():
239 242
     """test_get_ipython_dir_2, Testcase to see if we can call get_ipython_dir without Exceptions."""
240 243
     path.get_home_dir = lambda : "someplace"
  244
+    path.get_xdg_dir = lambda : None
  245
+    path._writable_dir = lambda path: True
241 246
     os.name = "posix"
242 247
     env.pop('IPYTHON_DIR', None)
243 248
     env.pop('IPYTHONDIR', None)
@@ -249,6 +254,7 @@ def test_get_ipython_dir_2():
249 254
 def test_get_ipython_dir_3():
250 255
     """test_get_ipython_dir_3, use XDG if defined, and .ipython doesn't exist."""
251 256
     path.get_home_dir = lambda : "someplace"
  257
+    path._writable_dir = lambda path: True
252 258
     os.name = "posix"
253 259
     env.pop('IPYTHON_DIR', None)
254 260
     env.pop('IPYTHONDIR', None)
@@ -283,18 +289,23 @@ def test_get_ipython_dir_5():
283 289
 @with_environment
284 290
 def test_get_ipython_dir_6():
285 291
     """test_get_ipython_dir_6, use XDG if defined and neither exist."""
286  
-    path.get_home_dir = lambda : 'somehome'
287  
-    path.get_xdg_dir = lambda : 'somexdg'
  292
+    xdg = os.path.join(HOME_TEST_DIR, 'somexdg')
  293
+    os.mkdir(xdg)
  294
+    shutil.rmtree(os.path.join(HOME_TEST_DIR, '.ipython'))
  295
+    path.get_home_dir = lambda : HOME_TEST_DIR
  296
+    path.get_xdg_dir = lambda : xdg
288 297
     os.name = "posix"
289 298
     env.pop('IPYTHON_DIR', None)
290 299
     env.pop('IPYTHONDIR', None)
291  
-    xdg_ipdir = os.path.join("somexdg", "ipython")
  300
+    env.pop('XDG_CONFIG_HOME', None)
  301
+    xdg_ipdir = os.path.join(xdg, "ipython")
292 302
     ipdir = path.get_ipython_dir()
293 303
     nt.assert_equal(ipdir, xdg_ipdir)
294 304
 
295 305
 @with_environment
296 306
 def test_get_ipython_dir_7():
297 307
     """test_get_ipython_dir_7, test home directory expansion on IPYTHON_DIR"""
  308
+    path._writable_dir = lambda path: True
298 309
     home_dir = os.path.expanduser('~')
299 310
     env['IPYTHON_DIR'] = os.path.join('~', 'somewhere')
300 311
     ipdir = path.get_ipython_dir()
@@ -305,6 +316,7 @@ def test_get_ipython_dir_7():
305 316
 def test_get_xdg_dir_1():
306 317
     """test_get_xdg_dir_1, check xdg_dir"""
307 318
     reload(path)
  319
+    path._writable_dir = lambda path: True
308 320
     path.get_home_dir = lambda : 'somewhere'
309 321
     os.name = "posix"
310 322
     env.pop('IPYTHON_DIR', None)
@@ -369,3 +381,24 @@ def test_get_long_path_name():
369 381
     p = path.get_long_path_name('/usr/local')
370 382
     nt.assert_equals(p,'/usr/local')
371 383
 
  384
+@dec.skip_win32 # can't create not-user-writable dir on win
  385
+@with_environment
  386
+def test_not_writable_ipdir():
  387
+    tmpdir = tempfile.mkdtemp()
  388
+    os.name = "posix"
  389
+    env.pop('IPYTHON_DIR', None)
  390
+    env.pop('IPYTHONDIR', None)
  391
+    env.pop('XDG_CONFIG_HOME', None)
  392
+    env['HOME'] = tmpdir
  393
+    ipdir = os.path.join(tmpdir, '.ipython')
  394
+    os.mkdir(ipdir)
  395
+    os.chmod(ipdir, 600)
  396
+    stderr = io.stderr
  397
+    pipe = StringIO.StringIO()
  398
+    io.stderr = pipe
  399
+    ipdir = path.get_ipython_dir()
  400
+    io.stderr.flush()
  401
+    io.stderr = stderr
  402
+    nt.assert_true('WARNING' in pipe.getvalue())
  403
+    env.pop('IPYTHON_DIR', None)
  404
+    
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.