Track permissions granted to extensions in prefs

Track permissions granted to extensions in prefs

Adds a section to the extension preferences to store the recognized permissions that the user has granted the extension.
Ignores unknown permissions when installing extensions.
Disables extension and prompts user to re-enable when unknown permissions become recognized (e.g., through browser upgrade).
Allows extensions to remove a permission in one version, then add them back in the next without prompting the user.

BUG=42742
TEST=ExtensionsServiceTest, ExtensionPrefsGrantedPermissions, ExtensionTest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67500

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67544

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67616

[jstritar, 2010-11-09 21:14:14]

Aaron, could you take a look at this one? I could use some feedback, since I'm
not entirely familiar with the different types of extensions and how the
permissions effect them. 

On a side note, I think ExtensionsService and Extension could use some
refractoring to better encapsulate the permissions and policies for different
extension types.

[Aaron Boodman, 2010-11-12 00:05:24]

The way we're losing error handling stinks. I've created a new bug to track the
fix for that: crbug.com/62897

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (left):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:214: if (!ext->HasKey(pref_key))
return false;
Why change this?

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:81: // when unknown permissions
become known
Missing period at end of sentence.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:232: DictionaryValue* ext =
GetExtensionPref(extension_id);
Nice little cleanup, thanks.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:129: void GrantPermissions(const
std::string& extension_id,
Nit: how about "SetLastGrantedPermissions" and "GetLastGrantedPermissions". It's
a minor change, but the verb "grant" to me sounds like this doing more than what
it actually is doing. Also update the comments to match.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service.cc:1555: // known set of
permissions, so that we can prompt the user when unknown
It might be nice to include a little example here to clarify.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1022:
.AppendASCII("permissions-unknown-v1.crx");
It is better to use unpacked extensions in tests so that they can easily be
changed.

Since you want to test the behavior of installing, you can start with an
unpacked directory that is checked in, and create a crx dynamically during the
test, then install that. See PackAndInstallExtension() in this file.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1025:
InstallExtension(path, true);
Add a test to validate that there are no granted permissions prior to
installation?

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1067:
InitializeInstalledExtensionsService(pref_path, source_install_dir);
I think it would be a little bit better to test this using more of the C++ API,
instead of manipulating the preferences directly.

You could:

* InitializeEmptyExtensionsService()
* Install a packed extension that has some known set of permissions
* Use ExtensionPrefs API to set the granted permissions to some smaller set
* Reinitialize extensions service
* Expect disabledness

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1084: FilePath
extensions_path;
Same thing here.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1114:
ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
Same thing here.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:1133: // so that extensions can remove and
re-add permissions during upgrades.
I don't understand this comment. Can you combine these now? They seem likely to
get out of date otherwise.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:1762: // backwards compatability.
This is a good place to add a link to the bug, so future readers can dig deeper.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_extent.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_extent.cc:63: void
ExtensionExtent::GetAsStringSet(
It seems ugly somehow to pass these around as string sets. Can you push the
details of serializing ExtensionExtents to strings and parsing ExtensionExtents
from strings out to the edges more? I would like to see ExtensionExtent being
passed, eg, to Extension::IsPermissionIncrease(), rather than
std::set<std::string>

Only ExtensionPrefs should have methods that accept and return ExtensionExtents
and it should deal with the parsing internally.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_manifests_unittest.cc:313: // the extension
should successfully load for any of these.
This comment seems kinda cryptic. How about renaming the test so that it's like
"AllowUnrecognizedPermissions" or something?

Also, do we have a test that handles the case of invalid URLPatterns in host
permissions?

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_unittest.cc:255: // We now allow unknown
permissions, so this will be valid.
In general don't say things like "we now do xyz" in comments since future
readers won't remember what we did before. Just update the code so that it makes
sense in the new world.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_unittest.cc:330: // This will no longer throw
an error message for invalid permissions.
Same here.

[jstritar, 2010-11-19 21:38:36]

I uploaded a new CL that incorporates your feedback. A nice side effect of these
changes is that extensions can now remove permissions in one version, and then
add them back in the next without prompting the user (since the pref remembers
that they have already been granted).

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (left):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:214: if (!ext->HasKey(pref_key))
return false;
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> Why change this?

The HasKey call seemed redundant since the Get methods return false when the
preference doesn't exist. I also needed to change it because HasKey doesn't
support paths (granted_permissions.initialized) while the Get methods do.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:81: // when unknown permissions
become known
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> Missing period at end of sentence.

Done.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:129: void GrantPermissions(const
std::string& extension_id,
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> Nit: how about "SetLastGrantedPermissions" and "GetLastGrantedPermissions".
It's
> a minor change, but the verb "grant" to me sounds like this doing more than
what
> it actually is doing. Also update the comments to match.

I didn't include "Last" in the name since these hold the maximum set of
permissions granted to the extension (it performs a union), so that extensions
can remove a permission in one version, then add it back without prompting the
user. I updated the names to GetGrantedPermissions and AddGrantedPermissions.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service.cc:1555: // known set of
permissions, so that we can prompt the user when unknown
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> It might be nice to include a little example here to clarify.

I updated the comment to make more sense and added an example. Hopefully the
example isn't too confusing...

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1022:
.AppendASCII("permissions-unknown-v1.crx");
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> It is better to use unpacked extensions in tests so that they can easily be
> changed.
> 
> Since you want to test the behavior of installing, you can start with an
> unpacked directory that is checked in, and create a crx dynamically during the
> test, then install that. See PackAndInstallExtension() in this file.

Done.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1025:
InstallExtension(path, true);
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> Add a test to validate that there are no granted permissions prior to
> installation?

Now that I'm packing the extensions dynamically, I'm not sure how I'd do this
since the IDs aren't predictable. Would I include a *.pem file and update
PackAndInstallExtension to use that?

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1067:
InitializeInstalledExtensionsService(pref_path, source_install_dir);
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> I think it would be a little bit better to test this using more of the C++
API,
> instead of manipulating the preferences directly.
> 
> You could:
> 
> * InitializeEmptyExtensionsService()
> * Install a packed extension that has some known set of permissions
> * Use ExtensionPrefs API to set the granted permissions to some smaller set
> * Reinitialize extensions service
> * Expect disabledness

Done. I also combined these tests into one, so I don't have to do the same setup
each time.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1084: FilePath
extensions_path;
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> Same thing here.

Done.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:1114:
ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> Same thing here.

Done.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:1133: // so that extensions can remove and
re-add permissions during upgrades.
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> I don't understand this comment. Can you combine these now? They seem likely
to
> get out of date otherwise.

Okay, I combined both IsPrivilegeIncreases and updated the calls to it from
ExtensionService::OnExtensionLoaded. As an added benefit, extensions can now
remove permissions in one upgrade, then add them back via a silent upgrade since
we know they were already granted.

In adding this, I created a static version of
Extension::HasEffectiveAccessToAllHosts. This isn't in the right spot in the
*.cc file based on the conventions we discussed in other code reviews, but none
of the static methods are, so it seemed better not to rearrange them given how
big the file is.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:1762: // backwards compatability.
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> This is a good place to add a link to the bug, so future readers can dig
deeper.

Done.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_extent.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_extent.cc:63: void
ExtensionExtent::GetAsStringSet(
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> It seems ugly somehow to pass these around as string sets. Can you push the
> details of serializing ExtensionExtents to strings and parsing
ExtensionExtents
> from strings out to the edges more? I would like to see ExtensionExtent being
> passed, eg, to Extension::IsPermissionIncrease(), rather than
> std::set<std::string>
> 
> Only ExtensionPrefs should have methods that accept and return
ExtensionExtents
> and it should deal with the parsing internally.

Done. I now have an "AddPattern" helper method in a few different files
(extension_prefs.cc, extension_service_unittest.cc and
extension_prefs_unittest.cc). Is there a good place to put this or should I
leave it copy+pasted?

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_manifests_unittest.cc:313: // the extension
should successfully load for any of these.
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> This comment seems kinda cryptic. How about renaming the test so that it's
like
> "AllowUnrecognizedPermissions" or something?
> 
> Also, do we have a test that handles the case of invalid URLPatterns in host
> permissions?

I renamed the test and updated the comment.

As for testing invalid URLPatterns, ChromeURLPermissionInvalid checks for an
error when using the "chrome://newtab/" permission and
ChromeURLContentScriptInvalid tests a similar case for the content_script
matches field. I don't think we can test beyond valid schemes, since we assume
they're unrecognized API permissions when they fail parsing. We could probably
improve error handling by validating the API permissions, by for instance,
assuming it's a URLPattern if it contains one of these chars ".:/". I don't do
this right now though.

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/4687005/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_unittest.cc:255: // We now allow unknown
permissions, so this will be valid.
On 2010/11/12 00:05:24, Aaron Boodman wrote:
> In general don't say things like "we now do xyz" in comments since future
> readers won't remember what we did before. Just update the code so that it
makes
> sense in the new world.

Done.

[Aaron Boodman, 2010-11-22 07:57:53]

Cool. We can talk about the tests on IRC tomorrow.

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/4687005/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.h:129: void GrantPermissions(const
std::string& extension_id,
On 2010/11/19 21:38:36, jstritar1 wrote:
> On 2010/11/12 00:05:24, Aaron Boodman wrote:
> > Nit: how about "SetLastGrantedPermissions" and "GetLastGrantedPermissions".
> It's
> > a minor change, but the verb "grant" to me sounds like this doing more than
> what
> > it actually is doing. Also update the comments to match.
> 
> I didn't include "Last" in the name since these hold the maximum set of
> permissions granted to the extension (it performs a union), so that extensions
> can remove a permission in one version, then add it back without prompting the
> user. I updated the names to GetGrantedPermissions and AddGrantedPermissions.

Neat!

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:486:
DCHECK(Extension::IdIsValid(extension_id));
You can leave these if you like, but it's a little over-paranoid compared to the
rest of Chrome IMO. We don't typically assert input arguments are valid.

Up to you though.

If you do leave it: DCHECK->CHECK

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:503: AddPattern(host_extent, *i);
No need for the helper function here since what it is doing is just one line
anyway. Also, you should use the same valid schemes as Extension uses when it
creates its own host permissions during parse. You might want to expose the
value Extension uses to avoid duplication.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:512:
DCHECK(Extension::IdIsValid(extension_id));
DCHECK->CHECK

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:513:
UpdateExtensionPref(extension_id, kPrefGrantedPermissionsInitialized,
Is it possible to remove this and just use the presence of the subtree itself as
a flag that it has been initialized?

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1528: if (extensions_enabled()
||
I think this long check can be removed now. It is from a time when extensions
were disabled by default.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1536: // We keep track of all
permissions the user has granted each extension.
How about factoring all this big chunk of upgrade logic into its own method.
That way this comment can go there.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1564: if (extension->location()
!= Extension::COMPONENT &&
It seems like we only want to store the privileges in the case of INTERNAL
extensions, to help keep the Preferences file size down, not COMPONENT or
EXTERNAL_*.

In that case, the call site for OnExtensionLoaded for component extensions could
be changed to pass |true| for allow_privilege_increase, then this code could be
simpler:

if (!allow_privilege_increase || !GetGrantedPermissions()) {
  GrantPermissions();
}

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1569:
DCHECK(extension_prefs_->GetGrantedPermissions(
Danger! Never do work with side-effects in DCHECK. This gets compiled out in
release builds!

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1580: if ((extension->location()
== Extension::INTERNAL || is_extension_upgrade)
And we could remove the location() check here too:

if (is_extension_upgrade && !allow_privilege_increase) {

}

http://codereview.chromium.org/4687005/diff/12001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/4687005/diff/12001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1087: DCHECK(new_extension->location() !=
Extension::COMPONENT);
Two things:

1) DCHECK is for wusses, and causes lots of problems. It is compiled out of
release builds, so execution continues past broken assumptions and we finally
crash somewhere else, long past the actual point of failure. When you find
yourself using DCHECK consider whether a) this really cannot happen ever unless
a programmer has made a mistake (in which case use CHECK), or b) this could
happen if some external system was misbehaving or not meeting our expectations
(in which case handle the error gracefully).

2) In this case, I think the [D]CHECK is not buying anything. This is a nice
generic function that we might want to use in other scenarios in the future. In
the current configuration we do not call it with component extensions, but there
is no reason to lock it down.

[jstritar, 2010-11-22 23:01:08]

Uploaded a new CL.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:486:
DCHECK(Extension::IdIsValid(extension_id));
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> You can leave these if you like, but it's a little over-paranoid compared to
the
> rest of Chrome IMO. We don't typically assert input arguments are valid.
> 
> Up to you though.
> 
> If you do leave it: DCHECK->CHECK

Got rid of all of them except the Extension::IdIsValid check, and renamed that
one to CHECK.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:503: AddPattern(host_extent, *i);
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> No need for the helper function here since what it is doing is just one line
> anyway. Also, you should use the same valid schemes as Extension uses when it
> creates its own host permissions during parse. You might want to expose the
> value Extension uses to avoid duplication.

Done.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:512:
DCHECK(Extension::IdIsValid(extension_id));
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> DCHECK->CHECK

Done.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:513:
UpdateExtensionPref(extension_id, kPrefGrantedPermissionsInitialized,
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> Is it possible to remove this and just use the presence of the subtree itself
as
> a flag that it has been initialized?

I think we need this because empty lists and dictionaries get pruned from the
tree when it's serialized, so we can't distinguish between it not being
initialized and the extension having no permissions.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1528: if (extensions_enabled()
||
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> I think this long check can be removed now. It is from a time when extensions
> were disabled by default.

Done.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1536: // We keep track of all
permissions the user has granted each extension.
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> How about factoring all this big chunk of upgrade logic into its own method.
> That way this comment can go there.

Done.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1564: if (extension->location()
!= Extension::COMPONENT &&
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> It seems like we only want to store the privileges in the case of INTERNAL
> extensions, to help keep the Preferences file size down, not COMPONENT or
> EXTERNAL_*.
> 
> In that case, the call site for OnExtensionLoaded for component extensions
could
> be changed to pass |true| for allow_privilege_increase, then this code could
be
> simpler:
> 
> if (!allow_privilege_increase || !GetGrantedPermissions()) {
>   GrantPermissions();
> }

Done. I ended up needing to change the allow_privilege_increase value in a
couple more places (Extension::InstallExtension and
download_util::OpenChromeExtension).

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1569:
DCHECK(extension_prefs_->GetGrantedPermissions(
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> Danger! Never do work with side-effects in DCHECK. This gets compiled out in
> release builds!

Wow, yeah good point. Fixed.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1580: if ((extension->location()
== Extension::INTERNAL || is_extension_upgrade)
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> And we could remove the location() check here too:
> 
> if (is_extension_upgrade && !allow_privilege_increase) {
> 
> }

Done.

http://codereview.chromium.org/4687005/diff/12001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/4687005/diff/12001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1087: DCHECK(new_extension->location() !=
Extension::COMPONENT);
On 2010/11/22 07:57:53, Aaron Boodman wrote:
> Two things:
> 
> 1) DCHECK is for wusses, and causes lots of problems. It is compiled out of
> release builds, so execution continues past broken assumptions and we finally
> crash somewhere else, long past the actual point of failure. When you find
> yourself using DCHECK consider whether a) this really cannot happen ever
unless
> a programmer has made a mistake (in which case use CHECK), or b) this could
> happen if some external system was misbehaving or not meeting our expectations
> (in which case handle the error gracefully).
> 
> 2) In this case, I think the [D]CHECK is not buying anything. This is a nice
> generic function that we might want to use in other scenarios in the future.
In
> the current configuration we do not call it with component extensions, but
there
> is no reason to lock it down.

Thanks for the explanation... I went a little crazy with the DHCECKs :). I
removed this one, and most of the others.

[Aaron Boodman, 2010-11-23 00:06:38]

Sorry a few more changes.

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:513:
UpdateExtensionPref(extension_id, kPrefGrantedPermissionsInitialized,
On 2010/11/22 23:01:08, jstritar1 wrote:
> On 2010/11/22 07:57:53, Aaron Boodman wrote:
> > Is it possible to remove this and just use the presence of the subtree
itself
> as
> > a flag that it has been initialized?
> 
> I think we need this because empty lists and dictionaries get pruned from the
> tree when it's serialized, so we can't distinguish between it not being
> initialized and the extension having no permissions.

I see. It's a shame to bloat the preferences file with this extra bit of state
if every single object. It would be better if we could migrate the preferences
file once and write out all these granted permissions, and then assume from then
forward that if there are no granted permissions it means the user granted no
permissions.

We have a pattern for this in other parts of Chrome. See
chrome/browser/dom_ui/new_tab_ui.cc ::UpdateUserPrefsVersion() for one example
of it.

Right now there are two subsystems that I can see that do this: NTP and Content
Settings. We might want to consider making one master pref_version key for the
entire Preferences file and having a MigrateUserPrefs in Profile and calling all
the components that have migration code from there, just like we do today for
RegisterUserPrefs.

But again that could be a separate refactor. For now, at least please add the
preference_version key and migration code to ExtensionPrefs.

http://codereview.chromium.org/4687005/diff/20001/chrome/browser/extensions/e...
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/4687005/diff/20001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1568: const Extension*
extension, bool allow_privilege_increase) {
As we talked about in IRC, I think everything would be nicer if we just get rid
of the allow_privilege_increase param here and let this method figure it out on
its own.

http://codereview.chromium.org/4687005/diff/20001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/4687005/diff/20001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1089: if (old_extension->plugins().size()
> 0)
I think this method should not be passed the old_extension object at all in
order to be robust.

Right now, it works, but imagine this future situation:

- we add some new API that counts as "full permissions" (like plugins).
- we change this code to recognize it.
- we ship a new version of Chrome to users.

Now when someone upgrades their extension to take advantage of the new
permission or capability that counts as 'full permission', this code won't do
the right thing, because old_extension will actually be new code that knows how
to parse the new capability.

Basically, I think we need to store a 'full_permissions' bit in addition to
host_permissions and api_permissions, and pass it in here.

This probably also indicates the need for an ExtensionPermissionSet object that
can be parsed either from stored preferences, or from an extension manifest. But
we can do that some other day :).

[jstritar, 2010-11-24 20:08:25]

I realized we don't need the migration code after adding the "full permissions"
bit, since that never gets pruned out of the tree. In this CL, I stripped out
all the allow_privilege_increase things, added the "full permissions" bit and
got rid of the old extension from Extension::IsPrivilegeIncrease. 

I think things are looking pretty good now -- the permissions policies are
isolated to a few methods and hopefully a lot easier to follow. I agree that the
next step should be to factor the granted permissions into a class that can
initialize itself from a manifest and from the preferences. This class can be
comparable so that we don't need Extension::IsPrivilegeIncrease. I've saved that
for after we get this in.

Hope you're having a good time snowboarding!

On 2010/11/23 00:06:38, Aaron Boodman wrote:
> Sorry a few more changes.
> 
>
http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_prefs.cc (right):
> 
>
http://codereview.chromium.org/4687005/diff/12001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_prefs.cc:513:
> UpdateExtensionPref(extension_id, kPrefGrantedPermissionsInitialized,
> On 2010/11/22 23:01:08, jstritar1 wrote:
> > On 2010/11/22 07:57:53, Aaron Boodman wrote:
> > > Is it possible to remove this and just use the presence of the subtree
> itself
> > as
> > > a flag that it has been initialized?
> > 
> > I think we need this because empty lists and dictionaries get pruned from
the
> > tree when it's serialized, so we can't distinguish between it not being
> > initialized and the extension having no permissions.
> 
> I see. It's a shame to bloat the preferences file with this extra bit of state
> if every single object. It would be better if we could migrate the preferences
> file once and write out all these granted permissions, and then assume from
then
> forward that if there are no granted permissions it means the user granted no
> permissions.
> 
> We have a pattern for this in other parts of Chrome. See
> chrome/browser/dom_ui/new_tab_ui.cc ::UpdateUserPrefsVersion() for one example
> of it.
> 
> Right now there are two subsystems that I can see that do this: NTP and
Content
> Settings. We might want to consider making one master pref_version key for the
> entire Preferences file and having a MigrateUserPrefs in Profile and calling
all
> the components that have migration code from there, just like we do today for
> RegisterUserPrefs.
> 
> But again that could be a separate refactor. For now, at least please add the
> preference_version key and migration code to ExtensionPrefs.
> 
>
http://codereview.chromium.org/4687005/diff/20001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extensions_service.cc (right):
> 
>
http://codereview.chromium.org/4687005/diff/20001/chrome/browser/extensions/e...
> chrome/browser/extensions/extensions_service.cc:1568: const Extension*
> extension, bool allow_privilege_increase) {
> As we talked about in IRC, I think everything would be nicer if we just get
rid
> of the allow_privilege_increase param here and let this method figure it out
on
> its own.
> 
>
http://codereview.chromium.org/4687005/diff/20001/chrome/common/extensions/ex...
> File chrome/common/extensions/extension.cc (right):
> 
>
http://codereview.chromium.org/4687005/diff/20001/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:1089: if
(old_extension->plugins().size()
> > 0)
> I think this method should not be passed the old_extension object at all in
> order to be robust.
> 
> Right now, it works, but imagine this future situation:
> 
> - we add some new API that counts as "full permissions" (like plugins).
> - we change this code to recognize it.
> - we ship a new version of Chrome to users.
> 
> Now when someone upgrades their extension to take advantage of the new
> permission or capability that counts as 'full permission', this code won't do
> the right thing, because old_extension will actually be new code that knows
how
> to parse the new capability.
> 
> Basically, I think we need to store a 'full_permissions' bit in addition to
> host_permissions and api_permissions, and pass it in here.
> 
> This probably also indicates the need for an ExtensionPermissionSet object
that
> can be parsed either from stored preferences, or from an extension manifest.
But
> we can do that some other day :).

[Aaron Boodman, 2010-11-24 21:08:04]

LGTM

This is good for a first commit. For my two comments below, you can either do
and then commit without further review, or create bugs for yourself and do after
the holidays.

Regarding the migration code, I still think it would be better to have it since
then, we won't have to serialize "full": false in every extension. The default
value would be false. But again, that can be separate.

Thanks so much for getting this done!

http://codereview.chromium.org/4687005/diff/30001/chrome/browser/extensions/e...
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/4687005/diff/30001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:954: extension->plugins().size()
> 0,
You should factor this line into a method of Extension like
Extension::HasFullPermissions(), and call it here and in ExtensionInstallUI and
IsPrivilegeIncrease() so that if we ever add to the list of things that can
cause full permission we don't forget to update all the places that use it.

http://codereview.chromium.org/4687005/diff/30001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_service.cc:1653: // CrxInstaller should
have guaranteed that we aren't downgrading.
This should be done for EXTERNAL_* extension too (basically location() !=
INTERNAL). Your change actually changes the current behavior which is also
wrong, because it runs for location() == INTERNAL when it should not. See bug
63980 for more info.

[jstritar, 2010-11-29 19:25:19]

On 2010/11/24 21:08:04, Aaron Boodman wrote:
http://codereview.chromium.org/4687005/diff/30001/chrome/browser/extensions/e...
> chrome/browser/extensions/extensions_service.cc:1653: // CrxInstaller should
> have guaranteed that we aren't downgrading.
> This should be done for EXTERNAL_* extension too (basically location() !=
> INTERNAL). Your change actually changes the current behavior which is also
> wrong, because it runs for location() == INTERNAL when it should not. See bug
> 63980 for more info.

Which part is this referring to? The whole "if (is_extension_upgrade)" block?

[Aaron Boodman, 2010-11-29 20:23:27]

On 2010/11/29 19:25:19, jstritar1 wrote:
> On 2010/11/24 21:08:04, Aaron Boodman wrote:
>
http://codereview.chromium.org/4687005/diff/30001/chrome/browser/extensions/e...
> > chrome/browser/extensions/extensions_service.cc:1653: // CrxInstaller should
> > have guaranteed that we aren't downgrading.
> > This should be done for EXTERNAL_* extension too (basically location() !=
> > INTERNAL). Your change actually changes the current behavior which is also
> > wrong, because it runs for location() == INTERNAL when it should not. See
bug
> > 63980 for more info.
> 
> Which part is this referring to? The whole "if (is_extension_upgrade)" block?

No, just CHECK(extension->version()->CompareTo(*(old->version())) >= 0);

[jstritar, 2010-11-29 20:53:03]

On 2010/11/29 20:23:27, Aaron Boodman wrote:
> On 2010/11/29 19:25:19, jstritar1 wrote:
> > On 2010/11/24 21:08:04, Aaron Boodman wrote:
> >
>
http://codereview.chromium.org/4687005/diff/30001/chrome/browser/extensions/e...
> > > chrome/browser/extensions/extensions_service.cc:1653: // CrxInstaller
should
> > > have guaranteed that we aren't downgrading.
> > > This should be done for EXTERNAL_* extension too (basically location() !=
> > > INTERNAL). Your change actually changes the current behavior which is also
> > > wrong, because it runs for location() == INTERNAL when it should not. See
> bug
> > > 63980 for more info.
> > 
> > Which part is this referring to? The whole "if (is_extension_upgrade)"
block?
> 
> No, just CHECK(extension->version()->CompareTo(*(old->version())) >= 0);

I'm pretty sure the old code was running this for location() == INTERNAL (I
think it was run for all extension types, since extensions_enabled() would be
true). From the bug you referred me to, it sounds like unpacked extensions
should be able to downgrade. Should all external extensions be able to
downgrade? In that case it should be "if (location() == INTERNAL)" instead of
"if (location() != INTERNAL)".