Closed Bug 1028983 Opened 10 years ago Closed 10 years ago

Use OS.File to set file permissions in the webapp installer

Categories

(Firefox Graveyard :: Web Apps, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: marco, Assigned: darkowlzz, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 5 obsolete files)

We should use the new API introduced in bug 1001849 to set the file permissions in the webapps installer code (in toolkit/webapps).

Currently we're using nsIFile: http://mxr.mozilla.org/mozilla-central/search?string=permissions&find=%2Ftoolkit%2Fwebapps%2F&tree=mozilla-central
OS: Linux → All
Priority: -- → P3
Hardware: x86_64 → All
Whiteboard: [good first bug] → [good first bug][lang=js]
i would like to patch this bug .. please help me work on it as im new to all this!! ty
Hey Abhirav, have you already set up a Firefox building environment? If not, follow the steps outlined here: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Once you have managed to build, you'll need to modify the two files here (http://mxr.mozilla.org/mozilla-central/search?string=permissions&find=%2Ftoolkit%2Fwebapps%2F&tree=mozilla-central) to use the API introduced in bug 1001849 (unfortunately still undocumented on MDN) instead of nsIFile.
If you need more info, feel free to ping me on IRC (I'm marco on #openwebapps).

P.S.: The API is not documented on MDN, but you should be able to understand how to use it by looking at its tests here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_osfile_async_setPermissions.js
Attached patch bug1028983.patch (obsolete) — Splinter Review
Submitting this patch.

Didn't see any activity by Abhirav for the past 1 week. I'm fine if he comes back and takes on this bug.

Ran mochitests on toolkit/webapps/ , no failures.

Please review the patch and let me know if anything else if required.
Attachment #8484856 - Flags: feedback?(mar.castelluccio)
Comment on attachment 8484856 [details] [diff] [review]
bug1028983.patch

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

Looks good! Sorry for the delay in the review!
It would be great if you could write a test to verify that this works correctly (and to avoid potential regressions). You could extend these two tests to check the permissions of the affected files:
http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/test_hosted.xul
http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/test_packaged.xul
Attachment #8484856 - Flags: feedback?(mar.castelluccio) → feedback+
Added tests for the configuration files, checking their file permissions.

Let me know if the tests are correct.
Attachment #8484856 - Attachment is obsolete: true
Attachment #8490869 - Flags: feedback?(mar.castelluccio)
Comment on attachment 8490869 [details] [diff] [review]
Set file persmissions using OS.File with tests.

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

::: toolkit/webapps/tests/test_hosted.xul
@@ +106,5 @@
>      yield wait(1000);
>    }
>    ok(true, "App launchable");
> +
> +  stat = yield OS.File.stat(testAppInfo.webappINI);

You should test the permissions of this file only on Mac and the permissions of the desktopINI file only on Linux.

You could do something like this:
> if (MAC) {
>   // Test testAppInfo.webappINI
> } else if (LINUX) {
>   // Test testAppInfo.desktopINI (you'd need to add this property to the TestAppInfo object in http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/head.js)
> }

Nit: I'd use octal values instead of decimal, it's clearer.

The same comments apply to test_packaged.xul.
Attachment #8490869 - Flags: feedback?(mar.castelluccio) → feedback+
Update the test.

Thanks for the pointers to correct the tests :)
Attachment #8490869 - Attachment is obsolete: true
Attachment #8492019 - Flags: feedback?(mar.castelluccio)
Comment on attachment 8492019 [details] [diff] [review]
Set file persmissions using OS.File with tests.

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

One last bit missing, thank you for the patch! Do you have try access? (https://wiki.mozilla.org/ReleaseEngineering/TryServer)

::: toolkit/webapps/tests/test_hosted.xul
@@ +111,5 @@
> +    stat = yield OS.File.stat(testAppInfo.webappINI);
> +    is(stat.unixMode, 0o644, "Configuration file persmissions correct");
> +  } else if (LINUX) {
> +    stat = yield OS.File.stat(testAppInfo.desktopINI);
> +    is(stat.unixMode, 0o644, "Configuration file persmissions correct");

This should be 0o744 (because the PERMS_FILE value is ORed with S_IXUSR).

Nit: s/persmissions/permissions :D

::: toolkit/webapps/tests/test_packaged.xul
@@ +124,5 @@
> +    stat = yield OS.File.stat(testAppInfo.webappINI);
> +    is(stat.unixMode, 0o644, "Configuration file persmissions correct");
> +  } else if (LINUX) {
> +    stat = yield OS.File.stat(testAppInfo.desktopINI);
> +    is(stat.unixMode, 0o644, "Configuration file persmissions correct");

Idem.
Attachment #8492019 - Flags: feedback?(mar.castelluccio) → feedback+
Sorry for the long delay.

Made the changes and pushed to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b7f27d8e83d3
Attachment #8492019 - Attachment is obsolete: true
Attachment #8502379 - Flags: feedback?(mar.castelluccio)
Comment on attachment 8502379 [details] [diff] [review]
Set file persmissions using OS.File with tests.

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

No worries!

Looks like the tests are failing on Linux. The problem is that the function in which you're
calling OS.File.setPermissions is not a "task", so you can't use yield in that way (if you're curious,
here's the Task.jsm documentation: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm).

The fix is actually pretty simple, you just need to "wrap" the method with Task.async (http://mykzilla.blogspot.com/2014/03/simplify-asynchronous-method.html)

So:
  _createSystemFiles: function(aInstallDir) {
    ...
  },
would become:
  _createSystemFiles: Task.async(function*(aInstallDir) {

  }),

After this change, the callers of _createSystemFiles will need to call it using yield:
  yield _createSystemFiles(...);
Attachment #8502379 - Flags: feedback?(mar.castelluccio) → feedback+
Made the changes and pushed to try.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2ce37207413d
Attachment #8502379 - Attachment is obsolete: true
Attachment #8503037 - Flags: feedback?(mar.castelluccio)
Comment on attachment 8503037 [details] [diff] [review]
Set file persmissions using OS.File with tests.

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

Looks good to me! Please send this to try again before requesting a checkin, to make sure tests are passing on Linux now.
Attachment #8503037 - Flags: feedback?(mar.castelluccio) → review+
Pushed again 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=810bda6d908c
Assignee: nobody → indiasuny000
Attachment #8503037 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Thank you! Tests are passing now, so you can request a checkin (you just need to set "checkin-needed" in the Keywords)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/400b8c7a7f01
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/400b8c7a7f01
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 36
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.