846420 - Add an is_privileged field for packaged apps

Status: RESOLVED FIXED

(Marketplace Graveyard :: General, defect, P1)

Description

[Rob Hudson [:robhudson]]

See bug 842831

The end goal is to be able to signify an app is privileged on the review page so reviewers know to give the code a more in-depth review.

This is the first step.

I believe we can simply check the packaged app manifest and look for permissions. If any of the permissions requested required privileged access, set this flag.

This will need to be per-version since permissions can change between versions.

Comment 1

[Wil Clouser [:clouserw]]

Longer term we're talking about tracking specific privileges per app so we can do Buchner's bucket stuff.  Would it be better to track the privileges now instead of just a boolean?

Comment 2

[Andrew Williamson [:eviljeff]]

For review purposes we need to know the type (web/privileged) rather than the permissions.  So we would need a boolean as well.

Comment 3

[Matt Basta [:basta]]

For the review process, the validator will create errors if the developer uses permissions that their app's type does not permit. So we don't need to worry about that too much.

And on the review page, it shows a list of the permissions that the app uses along with the app type that the app must have in order to use those permissions.

Comment 4

[Matt Basta [:basta]]

I'm hijacking this bug to make it better. Instead of adding another column to the database, the better route here would be to do the following:

- Show the permissions box (generated by JS on the reviewer tools' app detail page) by default
- Add a field at the top of that box for the "type" field in the manifest.

This is a pretty simple update and shouldn't involve any server-side code at all.

Comment 5

[Andrew Williamson [:eviljeff]]

(In reply to Matt Basta [:basta] from comment #4)
> I'm hijacking this bug to make it better. Instead of adding another column
> to the database, the better route here would be to do the following:
> 
> - Show the permissions box (generated by JS on the reviewer tools' app
> detail page) by default
> - Add a field at the top of that box for the "type" field in the manifest.
> 
> This is a pretty simple update and shouldn't involve any server-side code at
> all.

that's missing the point - we don't just need it on the review page- we need it on the queue page; we need it in the reviewer search results; we probably need it on the lookup tool result page.

Comment 6

[Matt Basta [:basta]]

Why do we need this feature?(In reply to Andrew Williamson [:eviljeff] from comment #5)
> we need it on the queue page; we need it in the reviewer search results; we probably
> need it on the lookup tool result page.

Why?

Comment 7

[Andrew Williamson [:eviljeff]]

(In reply to Matt Basta [:basta] from comment #6)
> Why do we need this feature?(In reply to Andrew Williamson [:eviljeff] from
> comment #5)
> > we need it on the queue page; we need it in the reviewer search results; we probably
> > need it on the lookup tool result page.
> 
> Why?

So reviewers don't have to into the page to find out they can't review the app and generally because its more useful for app review purposes than knowing if its packaged. If you see comment #0 the original purpose of this bug is to enable searching for privileged apps.

Comment 8

[Wil Clouser [:clouserw]]

I'm expecting an is_privileged() hanging off of the app model.  On the backend it looks at basta's list of permissions an app uses and either replies yes or no.  So it's not a separate column, but someone only going as deep as the API or the model interface wouldn't know.

Comment 9

[Andrew Williamson [:eviljeff]]

(just to clarify): if an app is privileged doesn't depend on the permissions an App requests - it depends on the type declared in the manifest (defaulting to 'web'); and that then determines which permissions it is able to request (and which would be ignored by the runtime, and accepted/rejected by the validator).

Comment 10

[Mathieu Pillard [:mat]]

This is fixed in https://github.com/mozilla/zamboni/commit/85efc7af621cb6e60708f22a3bc51875369bd8b0

This commit:
- Adds an is_privileged property on Version
- Make an icon appear for privileged apps in the reviewer queues
- Change the 'Type' displayed for privileged apps to 'Privileged Packaged App' on the reviewer tools app details page

Note: search is unaffected for now, we need to add it to a new latest_version entry in the API results and be able to filter on it. This is bug https://bugzilla.mozilla.org/show_bug.cgi?id=848446 and https://bugzilla.mozilla.org/show_bug.cgi?id=842831

Steps to reproduce:
- Find or upload a Privileged Packaged app (simply adding "type": "privileged" to the manifest of a packaged app should do the trick)
- Look at the review queue, a new "P" icon should appear for this app
- Look in the reviewer details, the "Type" should say "Privileged Packaged app" instead of juts "Packaged app".