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

Inconsistent view on file/directory ownership via FakeOsModule and FakePathlibModule #678

Closed
wulpj opened this issue May 19, 2022 · 4 comments · Fixed by #682
Closed

Inconsistent view on file/directory ownership via FakeOsModule and FakePathlibModule #678

wulpj opened this issue May 19, 2022 · 4 comments · Fixed by #682
Labels

Comments

@wulpj
Copy link

wulpj commented May 19, 2022

Describe the bug
Since version 4.5.1 of pyfakefs there is an implementation of pathlib.Path.owner(), while previous version delate to the original pathlib implementation which uses os.stat. The new implementation assumes that the process owner is also the owner of the file. The result is an inconsistent view on ownership of the same file/directory depending on the method of obtaining the ownership data.

How To Reproduce

from pyfakefs import fake_filesystem, fake_pathlib
import pwd

fake_filesystem.set_uid(1004)
filesystem = fake_filesystem.FakeFilesystem()
os_module = fake_filesystem.FakeOsModule(filesystem)
pathlib_module = fake_pathlib.FakePathlibModule(filesystem)
filesystem.create_dir('test')
print(os.module.stat('test').st_uid)
print(pwd.getpwnam(pathlib_module.Path('test').owner()).pw_uid)

Your environment

Linux-5.15.0-30-generic-x86_64-with-redhat-7.7-Maipo
Python 3.6.8 (default, Jun 11 2019, 15:15:01
[GCC 4.8.5 20150623 (Red Hat 4.8.5-39)]
pyfakefs 4.5.6
@mrbean-bremen
Copy link
Member

mrbean-bremen commented May 19, 2022

Thanks! Ownership is implemented only very rudimentary in pyfakefs.
As for the mentioned change in 4.5.1: this actually should not have changed the behavior. The pathlib implementation just moved the owner implementation to the accessor class, which we mock, so we had to implement it, but the implementation mirrors the pathlib implementation exactly (as a side node: in Python 3.11, the accessor has been removed, so the implementation is back to the previous one - sigh...).
Anyway, there is currently (and has nether been) a synchronization between the real user IDs and the fake ones, as this has not been a use case so far. I will have a closer look at this later, but only after the adaptions needed for Python 3.11 (see the recent issue), which may take a while.

Also, it would help to understand your use case better, e.g. what are you testing and what is failing - your example code has a couple of syntax errors, so I'm not exactly sure what you are doing there.

@wulpj
Copy link
Author

wulpj commented May 19, 2022

Thanks for the quick response. Sorry for the syntax errors. I could not copy paste the running code easily so I quickly typed it over making a couple of mistakes along the way.

In my case I want to test ownership of a file which does not match the process owner. The application is a backup/restore framework that normally executes under its own system user and operates on files owned by a different user. In version 4.5.0 this used to work. I used fake_filesystem.set_uid to set the user id. In the code under test (which uses pathlib) a file ownership test would fail or succeed based on the uid that was being set in the surrounding test case.

Today I took some time to see why newer versions of pyfakefs were causing a failures. Based on the documentation I was expecting the old behaviour which corresponds with the uid/gid that is returned with the patched version of os.stat(). In other words the property that I was relying on is that given a filename 'test' I expect os.stat('test') to contain the same uid as the uid "associated with" fake_pathlib.Path('test').

In my full test setup I also patch pwd.getpwuid and pwd.getpwnam to control the mapping from uid to name. So I have no real need for synchronisation between real user IDs and fake ones. I simply make them up for the test.

@mrbean-bremen
Copy link
Member

Ok, thank you for the explanation! If you patch these functions, this makes more sense to me, and I will see if I can understand and fix the problem. It just may take a while until I get to it.

@mrbean-bremen
Copy link
Member

After having a second look (sorry it took me so long) I don't really understand why I implemented it the way it is. I think I did it that way because the fake filesystem is always created by the current user, and that way you have a valid user id, while setting some random user id to a file after creation may not be compatible with the real users, and owner would raise an exception in this case. But: if you are setting a user id in the test (or call chown on the file), you either have to make sure that it is valid, or patch pwd.getpwuid as you did anyway, so I think it should not be a problem. Also, there is the case of real files mapped into the fake file system, that get the real user/group id.

All that said, using the default implementation (using stat) as before is probably the right (and easy) thing to do.

mrbean-bremen added a commit to mrbean-bremen/pyfakefs that referenced this issue Jun 6, 2022
- both methods now look up the real user/group name for the user/group id
  that is associated with the fake file
- assumes a valid user and group id is set
- closes pytest-dev#678
mrbean-bremen added a commit that referenced this issue Jun 7, 2022
- both methods now look up the real user/group name for the user/group id
  that is associated with the fake file
- assumes a valid user and group id is set
- closes #678
github-actions bot pushed a commit that referenced this issue Jun 7, 2022
…hods now look up the real user/group name for the user/group id that is associated with the fake file - assumes a valid user and group id is set - closes #678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants