Repository: iterative/dvc
Pull request: utils/fs: checking files ownership in 'move' (#4348)
Files are checked for ownership by trying to `chmod` them before moving them around; if fail, return a verbose exception.
Previous behavior: moving files to a temp folder and then do the check; when fail, files seemed to be missing.

Should fix #4348 and maybe fix #2992.

* [x] ❗ I have followed the [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) checklist.

* [X] 📖 If this PR requires [documentation](https://dvc.org/doc) updates, I have created a separate PR (or issue, at least) in [dvc.org](https://github.com/iterative/dvc.org) and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏


Problem statement:
better message when trying to link from files that belong to another user
**Please provide information about your setup**
DVC version(i.e. `dvc --version`), Platform and method of installation (pip, homebrew, pkg Mac, exe (Windows), DEB(Linux), RPM(Linux))
DVC version: 0.77.3, Platform linux, method of installation: anaconda

Problem:
Our team has a shared data directory, all of the files and directories belong to the same group.
The members of the group have full read and write access.

I create dvc repository with `--no-scm` option successfully. However when I add any file whose owner is not the current user, dvc fails very badly.
It panics with the following error,
```
ERROR: unexpected error - [Errno 1] Operation not permitted: '/gendata/.dvc/cache/80/b1aeab28f6ddf9ba779b2681923442.ZKMhApEkd5VrDDarerdKmU'
```
 However, the file which I was adding is removed from the directory.

Maybe at the moment dvc doesn't support shared repository? 
However, I consider the error message is not clear enough, and it should never remove user's files when it fails. 



Parquet file owned by other user gets deleted upon dvc add

## Bug Report
 
On a Linux machine, in a folder `foo/` shared by various users, but not owned by me, I did:
```
git init .
dvc init .
git add .dvc
git commit -m "first commit"
```
The folder already had a file `bar.parquet` sitting in it, not owned by me either.
I did:
```
dvc add bar.parquet
```
And got the error
```
Adding...
ERROR: unexpected error - [Errno 1] Operation not permitted: '/baz/.dvc-cache/d0/3ea995971fc314c3087abbb3106e92.L6rCVNZWCFrzQRk53wdSCS'
```
 
**Most importantly**, the `foo/bar.parquet` file got deleted by the `dvc add` command above.
 
Note that I am using a shared `dvc` cache located at `/baz/.dvc-cache/`.
 
### Please provide information about your setup
 
**Output of `dvc version`:**
 
```console
$ dvc version
1.3.1
```
 
**Additional Information (if any):**
 
I could not reproduce the bug. After recreating `bar.parquet` manually, with me being the owner, I could run `dvc add bar.parquet` successfully.

Hints from issue discussion:
@qiuwei that's sounds really bad, but it's not exactly clear what's happening here - could you please give us a little bit more details?

 > Our team has a shared data directory

is it `/gendata`? do you use the same dir for the DVC cache? is DVC cache set to be shared as well?

> DVC doesn't support shared repository?

where is your repository located? the same directory as data?
Yes, it's /gendata.

I could provide a minimal working exmaple to reproduce.

step 1: create `proj1` directory
step 2: inside the `proj1` directory, initialize a dvc directory without scm.
```
dvc init --no-scm
```
step 3: create a file `test.txt`, change the owner of the file to someone else other than the current user, and change the permission bit to 664.  Note that the two users should be in the same group, so that the current user still has access to the `test.txt`.

step 4: add `test.txt` to dvc
```
dvc add test.txt
```
then you would be able to see the error mesage like:
```
ERROR: unexpected error - [Errno 1] Operation not permitted: '/gendata/.dvc/cache/80/b1aeab28f6ddf9ba779b2681923442.ZKMhApEkd5VrDDarerdKmU'

```

Hi @qiuwei ! Could you please add `-v` to the command that is failing and show us full log? 
@qiuwei Also, take a look at `dvc config cache.shared group` config option, it allows you to use your cache directory that is shared between users of the same group.
@chloe-wang , the file is still there at `gendata/.dvc/cache/80/b1aeab28f6ddf9ba779b2681923442.ZKMhApEkd5VrDDarerdKmU`

If the problem is that DVC is mysteriously disappearing files, we could wrap `remote.base.move` into a `try/except/finally` to make sure that if something failed, we bring back the file to where it was.
> Hi @qiuwei ! Could you please add `-v` to the command that is failing and show us full log?
The detailed log:
```
DEBUG: Trying to spawn '['/home/wqiu/anaconda3/bin/python', '/home/wqiu/anaconda3/bin/dvc', 'daemon', '-q', 'updater']'
DEBUG: Spawned '['/home/wqiu/anaconda3/bin/python', '/home/wqiu/anaconda3/bin/dvc', 'daemon', '-q', 'updater']'
DEBUG: CREATE TABLE IF NOT EXISTS state (inode INTEGER PRIMARY KEY, mtime TEXT NOT NULL, size TEXT NOT NULL, md5 TEXT NOT NULL, timestamp TEXT NOT NULL)
DEBUG: CREATE TABLE IF NOT EXISTS state_info (count INTEGER)
DEBUG: CREATE TABLE IF NOT EXISTS link_state (path TEXT PRIMARY KEY, inode INTEGER NOT NULL, mtime TEXT NOT NULL)
DEBUG: INSERT OR IGNORE INTO state_info (count) SELECT 0 WHERE NOT EXISTS (SELECT * FROM state_info)
DEBUG: PRAGMA user_version = 3;
DEBUG: Path test.txt inode 7080708
DEBUG: SELECT mtime, size, md5, timestamp from state WHERE inode=?
DEBUG: fetched: []
DEBUG: Path test.txt inode 7080708
DEBUG: SELECT mtime, size, md5, timestamp from state WHERE inode=?
DEBUG: fetched: []
DEBUG: INSERT INTO state(inode, mtime, size, md5, timestamp) VALUES (?, ?, ?, ?, ?)
DEBUG: {'test.txt': 'modified'}
DEBUG: Path test.txt inode 7080708
DEBUG: SELECT mtime, size, md5, timestamp from state WHERE inode=?
DEBUG: fetched: [('1577073058321199872', '16', 'a938454f3e0261239af50b945dd1041b', '1577073120559698944')]
DEBUG: UPDATE state SET timestamp = ? WHERE inode = ?
DEBUG: Computed stage 'test.txt.dvc' md5: '116fc54f419d5b3b489fb90d1cb8b3a6'
DEBUG: Saving 'test.txt' to '.dvc/cache/a9/38454f3e0261239af50b945dd1041b'.
DEBUG: cache '.dvc/cache/a9/38454f3e0261239af50b945dd1041b' expected 'a938454f3e0261239af50b945dd1041b' actual 'None'
100% Add|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████|1.00/1.00 [00:00<00:00,  2.26file/s]
DEBUG: SELECT count from state_info WHERE rowid=?
DEBUG: fetched: [(0,)]
DEBUG: UPDATE state_info SET count = ? WHERE rowid = ?
ERROR: unexpected error - [Errno 1] Operation not permitted: '/home/wqiu/proj1/.dvc/cache/a9/38454f3e0261239af50b945dd1041b.m7RXGjecgMRKibx8aTrut7'
------------------------------------------------------------
Traceback (most recent call last):
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/main.py", line 49, in main
    ret = cmd.run()
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/command/add.py", line 25, in run
    fname=self.args.file,
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/repo/__init__.py", line 36, in wrapper
    ret = f(repo, *args, **kwargs)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/repo/scm_context.py", line 4, in run
    result = method(repo, *args, **kw)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/repo/add.py", line 56, in add
    stage.commit()
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/funcy/decorators.py", line 39, in wrapper
    return deco(call, *dargs, **dkwargs)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/stage.py", line 162, in rwlocked
    return call()
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/funcy/decorators.py", line 60, in __call__
    return self._func(*self._args, **self._kwargs)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/stage.py", line 814, in commit
    out.commit()
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/output/base.py", line 251, in commit
    self.cache.save(self.path_info, self.info)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/remote/base.py", line 509, in save
    self._save(path_info, checksum)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/remote/base.py", line 517, in _save
    self._save_file(path_info, checksum)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/remote/base.py", line 418, in _save_file
    self.move(path_info, cache_info)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/remote/local.py", line 165, in move
    move(from_info, to_info, mode=mode)
  File "/home/wqiu/anaconda3/lib/python3.7/site-packages/dvc/utils/fs.py", line 105, in move
    os.chmod(tmp, mode)
PermissionError: [Errno 1] Operation not permitted: '/home/wqiu/proj1/.dvc/cache/a9/38454f3e0261239af50b945dd1041b.m7RXGjecgMRKibx8aTrut7'
------------------------------------------------------------


Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!
```

The problem is solved after `dvc config cache.shared group` is configured.
However, I still consider a more informative error message is needed here.
Thanks a lot for the help.
Discord context: https://discordapp.com/channels/485586884165107732/485596304961962003/740974584437669930

Commit message: utils/fs: checking files ownership in 'move' (#4348)

Patch to review:
diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py
index d80bce6ede..975ab9c1f5 100644
--- a/dvc/utils/fs.py
+++ b/dvc/utils/fs.py
@@ -95,15 +95,19 @@ def move(src, dst, mode=None):
     dst = os.path.abspath(dst)
     tmp = f"{dst}.{uuid()}"
 
+    try:
+        if mode is not None:
+            os.chmod(src, mode)
+    except OSError:
+        # File not owned by us, raise exception
+        raise DvcException(f"File not owned by user:  {src}")
+
     if os.path.islink(src):
         shutil.copy(src, tmp)
         os.unlink(src)
     else:
         shutil.move(src, tmp)
 
-    if mode is not None:
-        os.chmod(tmp, mode)
-
     shutil.move(tmp, dst)
 
 


Code review comments:
diff_hunk: @@ -95,15 +95,19 @@ def move(src, dst, mode=None):
     dst = os.path.abspath(dst)
     tmp = f"{dst}.{uuid()}"
 
+    try:
+        if mode is not None:
+            os.chmod(src, mode)
+    except OSError:
+        # File not owned by us, raise exception
path: dvc/utils/fs.py
text: @user1:
The exception string seems to pass the same information. Do we need the comment?

@user1:
Also, since we are only raising another exception, we can make use of `reraise`, an example is [here](https://github.com/iterative/dvc/blob/411b38896014ae004d0782e25dbd49f5d33af2b7/dvc/external_repo.py#L170)

@author:
1. Good point. 
2. Not sure what you meant so I introduced a new exception and used `reraise`.