Fix mode of METS file after atomic write (fix issue #403)

[stweil]

Signed-off-by: Stefan Weil [email protected]

[codecov-commenter]

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #608      +/-   ##
==========================================
+ Coverage   84.87%   84.89%   +0.02%     
==========================================
  Files          52       52              
  Lines        2890     2894       +4     
  Branches      564      564              
==========================================
+ Hits         2453     2457       +4     
  Misses        328      328              
  Partials      109      109              

Impacted Files	Coverage Δ
ocrd/ocrd/workspace.py	69.23% <100.00%> (+0.39%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9c3d32...96d54d7. Read the comment docs.

[bertsky]

Shouldn't this use the permissions self.mets_target had before (if it existed at all, otherwise 0o666)?

[stweil]

That would require additional code. In most (= all typical) cases the result would be the same. Therefore I don't think that is necessary.

[bertsky]

Sure, but then why do you care about the umask? You could just set it to 666 regardless.

There are use cases for this already BTW: when serving workspaces on the web (e.g. as part of a repository) the service daemon should usually not be allowed write access. Or on a cluster users might share their workspaces but disallow write access beyond the group.

[stweil]

I use umask to get the default file mode for new files (typically 0622). That's the same mode which we would have if we did not use atomic_write. And it fits the use cases which you describe. 0666 would allow write access for anyone which is not desired in most cases.

[bertsky]

That's the same mode which we would have if we did not use atomic_write.

Not true. Python's open does not change the permissions if the file already existed, but your code (still) does.

And it fits the use cases which you describe.

It happens to (if the umask is set that way). My point was that it's up to the user to decide file permissions, and that should always be respected by programs like ours.

0666 would allow write access for anyone which is not desired in most cases.

Neither is reverting a file's permissions to the umask default.

[kba]

Not true. Python's open does not change the permissions if the file already existed, but your code (still) does.

os.open does take umask into account: https://docs.python.org/3.5/library/os.html

os.open(path, flags, mode=0o777, *, dir_fd=None)
Open the file path and set various flags according to flags and possibly its mode according to mode. When computing mode, the current umask value is first masked out.

[stweil]

How should we proceed with the issue and this fix?

I think that the current situation with a mets.xml which cannot be read by anyone beside the owner is bad. Several times I now have used such files on our web server and only noted later that they could not work.

This pull request makes sure that the mets.xml has the same permissions as other files which are created by OCR-D processors. I think that is good and sufficient, and it solves my problem.

@bertsky thinks that any file permissions of an existing mets.xml should be preserved when the file is rewritten by OCR-D. I disagree, don't think that such additional code is necessary and would prefer to keep the code simple. If others think that preserving the permissions is better than using the default permissions and that it should be implemented now (it could be added any time later if someone misses it) I would not object. The final result would be the same for my use cases, so it would solve the initial problem, too.

[bertsky]

I think that the current situation with a mets.xml which cannot be read by anyone beside the owner is bad.

Of course.

I think that is good and sufficient, and it solves my problem.

It just shifts the problem (still changed permissions).

@bertsky thinks that any file permissions of an existing mets.xml should be preserved when the file is rewritten by OCR-D. I disagree, don't think that such additional code is necessary and would prefer to keep the code simple.

You still did not say why the permissions should not be preserved.

Saving the actual permissions before the atomic_write and imposing them afterwards would actually be simpler than your approach based on umask.

[stweil]

Saving the actual permissions before the atomic_write and imposing them afterwards would actually be simpler than your approach based on umask.

Feel free to prove that. I have no solution which is simpler than mine.

[kba]

I also favor not messing with umask and just resetting the mets.xml file mode after atomic_write. I'll send a PR.

[kba]

#625

[stweil]

This PR is superseded by PR #625, so it can be closed.