1023402 - API review for OS.File.setPermissions

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[:Paolo Amadini]

The API landed in bug 1001849 needs a non-blocking, high-level review.

Comment 1

[Zack Weinberg (:zwol)]

Bug 1022816 and bug 1022820 abstractly contemplate further additions/refinements to the API, and bug 1009465 is a user-visible arguable regression (I'm not sure we actually *want* the old behavior) that might be best served by adding a "read-only, please" option to the API.

Comment 2

[:Paolo Amadini]

(In reply to Zack Weinberg (:zwol) from comment #1)
> might be best served by adding a "read-only, please" option to the API.

This seems like a good idea to me.

Comment 3

[:Paolo Amadini]

Attachment: The patch

Now that the bug for which the first draft of setPermissions was created has been fixed and verified, I think it is time for making the API consistent with the rest of OS.File. I did the work and produced the attached patch. (I think it was fine to land the draft API without super-review in the interest of fixing the original bug soon).

In particular, I think OS.File.setPermissions(path, options) should be consistent with OS.File.writeAtomic(path, data, options) and OS.File.open(path, mode, options), when the "options" argument does not have a specific unixMode. The draft version has a set of default permissions that are not what the rest of OS.File uses.

In fact, per OS.File design choice, our default permissions should assume we create temporary files, or files in the profile directory, for which 0600 should be used.

Also, for the same consistency reasons, unixHonorUmask should default to "true". I wonder if we should change this option to unixIgnoreUmask instead.

This also adds a regression test for the Download-specific behavior.

David, let me know who could do the super-review for this API.

Tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=2069696cace0

Comment 4

[Zack Weinberg (:zwol)]

I am indifferent to most of the changes contemplated here, but not this one:

+      // Now that the file is completely downloaded, make it accessible by other
+      // users on this system.  On Unix, the umask of the process is respected.
+      // This call has no effect on Windows.
       try {
-        yield OS.File.setPermissions(aDownload.target.path);
+        yield OS.File.setPermissions(aDownload.target.path,
+                                     { unixMode: 0o666 });

This is not something the download manager should have to know how to do.  I insist that there must be an OS.File API, requiring _no special options_ (a boolean "doTheRightThing: true" would be okay, if we must) that has this effect on Unix, and has whatever analogous effect may be required on Windows.

Comment 5

[:Paolo Amadini]

(In reply to Zack Weinberg (:zwol) from comment #4)
>        try {
> -        yield OS.File.setPermissions(aDownload.target.path);
> +        yield OS.File.setPermissions(aDownload.target.path,
> +                                     { unixMode: 0o666 });
> 
> This is not something the download manager should have to know how to do.

I don't agree. As I've already outlined in bug 961080 comment 24, it is perfectly fine for that area of the code to use a platform-specific API.

> I insist that there must be an OS.File API, requiring _no special options_ (a
> boolean "doTheRightThing: true" would be okay, if we must) that has this
> effect on Unix, and has whatever analogous effect may be required on Windows.

If there was such an API, the download manager could use it, but I've not seen any consensus on the dev.platform thread you started that such an API should exist - on the contrary, the outcome seemed to be that it shouldn't, since we have a very limited use for it.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8451597 [details] [diff] [review]
The patch

Review of attachment 8451597 [details] [diff] [review]:
-----------------------------------------------------------------

That patch looks good to me.
I understand your point, Zack, but I believe that what you suggest would lead us to make the API over-generic before we know for sure what we need, in particular under Windows. I do not want to over-commit at this stage while we have a single user of the API, so I'm going with low-level approach.

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ +169,5 @@
>  
>  /**
> + * Tests the permissions of the final target file once the download finished.
> + */
> +add_task(function test_basic_unix_permissions()

Nit: `function*`

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_setPermissions.js
@@ +56,2 @@
>    let path = OS.Path.join(OS.Constants.Path.tmpDir,
> +                          "test_osfile_async_setPermissions.tmp");

Let's leave the `nonproto` and `proto` suffix, they make sure that we have no collision between tests.

@@ +85,5 @@
> +        if (options !== null) {
> +          do_print("Setting permissions to " + JSON.stringify(options));
> +          yield fd.setPermissions(options);
> +        }
> +  

Nit: Whitespace.

Comment 7

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/f0ef4ecb396e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0ef4ecb396e