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

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

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.

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

Added tests for the configuration files, checking their file permissions. Let me know if the tests are correct.

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.

Update the test. Thanks for the pointers to correct the tests :)

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.

Sorry for the long delay. Made the changes and pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b7f27d8e83d3

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(...);

Made the changes and pushed to try. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2ce37207413d

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.

Pushed again https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=810bda6d908c

Thank you! Tests are passing now, so you can request a checkin (you just need to set "checkin-needed" in the Keywords)