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.

Sign up for GitHub

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

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

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ocrd/ocrd/workspace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import io
from os import makedirs, unlink, listdir, path
from os import chmod, makedirs, umask, unlink, listdir, path
from pathlib import Path

import cv2
Expand Down Expand Up @@ -248,8 +248,13 @@ def save_mets(self):
log.info("Saving mets '%s'", self.mets_target)
if self.automatic_backup:
WorkspaceBackupManager(self).add()

mask = umask(0)
umask(mask)
mode = 0o666 & ~mask
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

with atomic_write(self.mets_target, overwrite=True) as f:
f.write(self.mets.to_xml(xmllint=True).decode('utf-8'))
chmod(self.mets_target, mode)

def resolve_image_exif(self, image_url):
"""
Expand Down