Closed Bug 1690613 Opened 8 months ago Closed 3 months ago

Allow access to url/title/favIconUrl without "tabs" permission in session API

Categories

(WebExtensions :: Untriaged, enhancement, P5)

enhancement

Tracking

(firefox92 fixed)

RESOLVED FIXED
92 Branch
Tracking Status
firefox92 --- fixed

People

(Reporter: robwu, Assigned: karim, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 3 obsolete files)

Bug 1679688 changed some internals so that the tab's url, title and favIconUrl of various APIs are shown if the extension has host permissions, instead of only when the extension has the "tabs" permission (or activeTab).

The tab descriptors returned by the sessions API have url, title and favIconUrl properties too, but these still require the "tabs" permission. For consistency with the rest of the extension API, we can also consider exposing details of the tab when the extension has host permissions.

Fixing this bug is not difficult:

Once this bug is fixed, we should update the documentation at:

Mentor: rob
Keywords: good-first-bug
Severity: -- → N/A
Priority: -- → P5

Hey,
I want to work on this bug but idk where to get started, in the description it is written that the check needs to be replaced, I don't really have an idea as to how, it would be helpful if you can provide me with some example or documentation as to how to approach this, also any documentation on how to write an unit test would be helpful.
Thank you.

Hi James,

In the linked bug (bug 1679688), there is a reference to a Phabricator revision with the full context of the original code review. Somewhere at the end there is also an automated comment that links to the commit after the patch was merged. The commit is https://hg.mozilla.org/mozilla-central/rev/a5b3fa068d5e. You can look at this patch, and search (Ctrl-F) for .hasPermission("tabs") to find similar code and see try to understand what it did before, and what it replacement looks like.

Once you understand the issue, you can try to work on a patch. You can either try to write a unit test first, or fix the bug first (and then undo the bugfix to see if your new test is indeed confirming that the bugfix patch did indeed change something).

There is documentation available on getting started with setting up a development environment and running tests, see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Hi Rob,
Thanks for the help, I'll get started right away and try my best to see what I can do.

(In reply to Rob Wu [:robwu] from comment #2)

Hi James,

In the linked bug (bug 1679688), there is a reference to a Phabricator revision with the full context of the original code review. Somewhere at the end there is also an automated comment that links to the commit after the patch was merged. The commit is https://hg.mozilla.org/mozilla-central/rev/a5b3fa068d5e. You can look at this patch, and search (Ctrl-F) for .hasPermission("tabs") to find similar code and see try to understand what it did before, and what it replacement looks like.

Once you understand the issue, you can try to work on a patch. You can either try to write a unit test first, or fix the bug first (and then undo the bugfix to see if your new test is indeed confirming that the bugfix patch did indeed change something).

There is documentation available on getting started with setting up a development environment and running tests, see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Hey Rob,
I can't really understand as to how these functions work, specifically the .hasPermission and .hasTabPermission (I noticed that in the bug you mentioned as reference, the .hasPermission('Tabs') has been replaced by tab.hasTabPermission and a hasTabPermission() has been created which calls the matchesHostPermission() { return this.extension.allowedOrigins.matches(this._url);} ) I don't really understand as to what this.extension.allowedOrigins.matches(this._url);} does, it would be helpful if you could explain this to me or provide me with some documentation about this,
Thanks

Apologies for the delay, I missed the last message.

(In reply to james from comment #4)

I can't really understand as to how these functions work, specifically the .hasPermission and .hasTabPermission (I noticed that in the bug you mentioned as reference, the .hasPermission('Tabs') has been replaced by tab.hasTabPermission

This is indeed how this bug can be resolved.

and a hasTabPermission() has been created which calls the matchesHostPermission() { return this.extension.allowedOrigins.matches(this._url);} ) I don't really understand as to what this.extension.allowedOrigins.matches(this._url);} does,

The details are in the referenced bug, bug 1679688. For the work here, it suffices to know that the hasTabPermission getter accounts for the "tabs" permission, the activeTab permission and host permissions. The last one is enforced via the this.extension.allowedOrigins.matches(this._url) call. If you need to know anything more specific, try to search for the implementation on Searchfox.org or post a specific question here.

(In reply to Rob Wu [:robwu] from comment #5)

Apologies for the delay, I missed the last message.

(In reply to james from comment #4)

I can't really understand as to how these functions work, specifically the .hasPermission and .hasTabPermission (I noticed that in the bug you mentioned as reference, the .hasPermission('Tabs') has been replaced by tab.hasTabPermission

This is indeed how this bug can be resolved.

and a hasTabPermission() has been created which calls the matchesHostPermission() { return this.extension.allowedOrigins.matches(this._url);} ) I don't really understand as to what this.extension.allowedOrigins.matches(this._url);} does,

The details are in the referenced bug, bug 1679688. For the work here, it suffices to know that the hasTabPermission getter accounts for the "tabs" permission, the activeTab permission and host permissions. The last one is enforced via the this.extension.allowedOrigins.matches(this._url) call. If you need to know anything more specific, try to search for the implementation on Searchfox.org or post a specific question here.

I made the changes, taking the referenced bug as a guide line.

I get the following error: 925:6 error Expected JSDoc for 'tab' but found 'extension'. valid-jsdoc (eslint)

What is this error?

(In reply to james from comment #6)

I get the following error: 925:6 error Expected JSDoc for 'tab' but found 'extension'. valid-jsdoc (eslint)

What is this error?

The warning is self-explanatory, but if it is not clear, you can look up the more detailed version of the error message by searching for valid-jsdoc, and finding https://eslint.org/docs/rules/valid-jsdoc

(In reply to Rob Wu [:robwu] from comment #7)

(In reply to james from comment #6)

I get the following error: 925:6 error Expected JSDoc for 'tab' but found 'extension'. valid-jsdoc (eslint)

What is this error?

The warning is self-explanatory, but if it is not clear, you can look up the more detailed version of the error message by searching for valid-jsdoc, and finding https://eslint.org/docs/rules/valid-jsdoc

Hey, I have commited the changes but how do I push only a single commit as a phabricator patch?
When I run moz-phab it also starts submitting the previous bug which I worked on and it shows me a warning that it might reopen the bug

Assignee: nobody → avikpaulsept2002
Status: NEW → ASSIGNED

Hey I made the required changes and added the test file, Plz go through it and tell me where I went wrong and what I can do to fix it up

Changed the if condtion check at browser/components/extentions/parent/ext-browser.js

Apologies for so many patches, I stacked the commits on top of each other and didn't really know how to make it all in one

(In reply to james from comment #13)

Apologies for so many patches, I stacked the commits on top of each other and didn't really know how to make it all in one

See the list at "Submitting a Patch" at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch, specifically the first item. It explains how the patch is created and how you can squash all changes into one commit. Please do so and mark the other revisions as "Abandoned" in Phabricator (via the dropdown menu above the comment box at the bottom of the page).

Attachment #9205895 - Attachment description: Bug 1690613 - changed the if check at browser/components/extensions/parent/ext-browser.js#956 and added the test files. r=robwu → Bug 1690613 - made the required changes at browser/components/extensions/parent/ext-browser.js#956 and added the test files. r=robwu

I squashed all the changes in one commit, amended that and pushed the phabricator patch. Also marked the other two revisions as abandoned.
Please let me know if I did something wrong

Attachment #9205897 - Attachment is obsolete: true
Attachment #9205898 - Attachment is obsolete: true

How do I get the properties of a tab and verify them?

(In reply to james from comment #16)

How do I get the properties of a tab and verify them?

I assume that you're referring to the following part of comment #0 :

Add a unit test that verifies that the properties are present without the "tabs" permissions, at https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js

The test that I linked uses the browser.sessions API (specifically browser.sessions.getRecentlyClosed()) to retrieve a list of recently closed tabs.

This part of the test confirms that the expected properties are present with the right permissions ("sessions", "tabs"): https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js#17-25,46-51,108,113-117,126-136,138

This part of the test confirms that without the right permissions (only "sessions" but no "tabs"), the properties are not present: https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js#140-164

To add a unit test for this bug, you have to:

  1. Open a new window with a tab with a http:-URL (e.g. http://example.com/testpage) - note the unit tests do not actually load the real example.com page, but a dummy value from the local test-only server)
  2. Close the window.
  3. Load an extension with the "sessions", "http://example.com/*" permissions and use the browser.sessions.getRecentlyClosed() to query the set of closed tabs, and confirm that the expected properties (url / title) are present.

Hey James! Are you still interested in working on this bug?

Flags: needinfo?(avikpaulsept2002)
Flags: needinfo?(avikpaulsept2002)

Hi James, we're going to open this issue up to other contributors. :) If you'd like to come back to continue working on it, feel free to submit an updated PR!

Assignee: avikpaulsept2002 → nobody
Status: ASSIGNED → NEW

Hello,

I'm interested in working on this!

Assignee: nobody → karim
Status: NEW → ASSIGNED
Attachment #9205895 - Attachment is obsolete: true
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/3bc13f11ddad
Allow access to url/title/favIconUrl with extension host permission in Tab.convertFromSessionStoreClosedData; add unit test. r=robwu
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch

Woohoo, thanks so much for the patch, Karim! 🎉 Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition.

Hope to see you around the project in the future!

Many thanks! 🎉🎉

You need to log in before you can comment on or make changes to this bug.