File API changes needed for safely passing user selected file entities from the file browser comp...

File API changes needed for safely passing user selected file entities from the file browser component extension to a 3rd party extension. 

BUG=chromium-os:11996
TEST=FileAccessPermissionsTest.FileAccessChecks
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81860

[zel, 2011-04-08 04:21:52]

Please note that code for this CL was originally in
http://codereview.chromium.org/6749021/. This now contains only File API and
related permission changes needed by that original CL.

[michaeln, 2011-04-09 00:16:44]

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:92:
CachedResults::const_iterator found = cached_results_.find(origin);
ooops... thnx

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:134:
ExtensionSpecialStoragePolicy::SpecialCollection::Clear();
i think you may not need the ExtensionSpecialStoragePolicy:: prefix here in the
method body

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:195:
ExtensionSpecialStoragePolicy::SpecialPathCollection::GetExtension(
i've seen other wrapping styles for this in the project which may help
readability

const Extension*
ExtensionSpecialStoragePolicy::SpecialPathCollection::GetExtension(

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.h:58: class
SpecialPathCollection : public SpecialCollection {
The list of paths per origin is a little different than the other extension
permissions the 'policy' reflects.

Would it make sense to keep the specific collection of 'paths' an origin has
access to in the CrosMountPointProvider instead of in this policy class? So the
division of labor is that the 'policy' will tell you if hosted_app or extension
associated with origin has the 'localfile' permission, and the
mountPointProvider would keep track of which virtual_paths each origin with that
permission can actually poke at.

http://codereview.chromium.org/6810037/diff/6006/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/6810037/diff/6006/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.h:36: const FilePath& virtual_path);
is there a use case for revoking access?

[ericu, 2011-04-09 01:45:13]

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy_unittest.cc
(right):

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy_unittest.cc:62:
scoped_refptr<Extension> CreateComponentApp() {
I'm not really sure what you're testing here.  Can some extension person cover
this bit?

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy_unittest.cc:223:
const GURL& hanlder_url = handler_app->url();
s/hanlder/handler/ throughout.

[zel, 2011-04-13 00:44:25]

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:134:
ExtensionSpecialStoragePolicy::SpecialCollection::Clear();
On 2011/04/09 00:16:45, michaeln wrote:
> i think you may not need the ExtensionSpecialStoragePolicy:: prefix here in
the
> method body

Done.

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:195:
ExtensionSpecialStoragePolicy::SpecialPathCollection::GetExtension(
On 2011/04/09 00:16:45, michaeln wrote:
> i've seen other wrapping styles for this in the project which may help
> readability
> 
> const Extension*
> ExtensionSpecialStoragePolicy::SpecialPathCollection::GetExtension(

Done.

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.h:58: class
SpecialPathCollection : public SpecialCollection {
On 2011/04/09 00:16:45, michaeln wrote:
> The list of paths per origin is a little different than the other extension
> permissions the 'policy' reflects.
> 
> Would it make sense to keep the specific collection of 'paths' an origin has
> access to in the CrosMountPointProvider instead of in this policy class? So
the
> division of labor is that the 'policy' will tell you if hosted_app or
extension
> associated with origin has the 'localfile' permission, and the
> mountPointProvider would keep track of which virtual_paths each origin with
that
> permission can actually poke at.

I could move this around, but I felt that this class generically deals with file
system permissions from extension context. All file driven extension permissions
are handed here as well.

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy_unittest.cc
(right):

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy_unittest.cc:62:
scoped_refptr<Extension> CreateComponentApp() {
On 2011/04/09 01:45:13, ericu wrote:
> I'm not really sure what you're testing here.  Can some extension person cover
> this bit?

I've added some comments that describe what is going on in these tests.

http://codereview.chromium.org/6810037/diff/6006/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/6810037/diff/6006/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.h:36: const FilePath& virtual_path);
On 2011/04/09 00:16:45, michaeln wrote:
> is there a use case for revoking access?

Not right now. All extension permissions are revoked when extension is unloaded
and that is already handled by ExtensionSpecialStoragePolicy.

[ericu, 2011-04-13 20:12:45]

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy_unittest.cc
(right):

http://codereview.chromium.org/6810037/diff/6006/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy_unittest.cc:62:
scoped_refptr<Extension> CreateComponentApp() {
On 2011/04/13 00:44:26, zel wrote:
> On 2011/04/09 01:45:13, ericu wrote:
> > I'm not really sure what you're testing here.  Can some extension person
cover
> > this bit?
> 
> I've added some comments that describe what is going on in these tests.

I like the comments, and the tests are clear--it's just that I can't really tell
if CreateComponentApp and CreateHandlerApp have any issues, as I'm not familiar
with the Extension class.  It is just test code, and it clearly seems to set up
what the tests need, but I can't guarantee that there aren't any holes here. 
Anyway, it's not a big deal.

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:33: virtual void
GrantLocalFileSystemAccess(const GURL& origin,
Just looked at this again, and it doesn't look right.  A virtual path doesn't
contain origin or filesystem type information.  I don't think this has enough
information for general use [both for IsLocalFileSystemAccessAllowed and
GrantLocalFileSystemAccess].  It seems like you'd need to pass in a full URL of
the file to which you're granting access.

[zel, 2011-04-13 20:38:55]

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:33: virtual void
GrantLocalFileSystemAccess(const GURL& origin,
On 2011/04/13 20:12:46, ericu wrote:
> Just looked at this again, and it doesn't look right.  A virtual path doesn't
> contain origin or filesystem type information.  I don't think this has enough
> information for general use [both for IsLocalFileSystemAccessAllowed and
> GrantLocalFileSystemAccess].  It seems like you'd need to pass in a full URL
of
> the file to which you're granting access.

The only reason I went with virtual part of the file was that all other
information from URL was redundant - origin is a separate param here and it's
always about local filesystem access. There is no other info in filesystem: urls
that I needed to keep a track of.  

I've felt that this approach required less memory to keep around, but I have no
problems of moving this to full filesystem: urls instead.

[ericu, 2011-04-13 22:51:27]

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:33: virtual void
GrantLocalFileSystemAccess(const GURL& origin,
On 2011/04/13 20:38:55, zel wrote:
> On 2011/04/13 20:12:46, ericu wrote:
> > Just looked at this again, and it doesn't look right.  A virtual path
doesn't
> > contain origin or filesystem type information.  I don't think this has
enough
> > information for general use [both for IsLocalFileSystemAccessAllowed and
> > GrantLocalFileSystemAccess].  It seems like you'd need to pass in a full URL
> of
> > the file to which you're granting access.
> 
> The only reason I went with virtual part of the file was that all other
> information from URL was redundant - origin is a separate param here and it's
> always about local filesystem access. There is no other info in filesystem:
urls
> that I needed to keep a track of.  
> 
> I've felt that this approach required less memory to keep around, but I have
no
> problems of moving this to full filesystem: urls instead.

While I put the comment here, it's more of a big deal when you put similar
methods/params into FileSystemMountPointProvider.  There you really need either
the full URL--even the origin/type/triplet isn't enough.

It doesn't make sense to grant access to anything in the sandbox with just the
triplet--either an origin has access to its own sandbox, or it doesn't. 
However, if you were granting e.g. an extension access to a file in someone
else's sandbox, that would make sense [even though we have no plans to do that
at present].  Providing the origin of the requester and the full URL of the
target file would supply enough information for that.

[michaeln, 2011-04-13 22:59:40]

> chrome/browser/extensions/extension_special_storage_policy.h:58: class
> SpecialPathCollection : public SpecialCollection {
> On 2011/04/09 00:16:45, michaeln wrote:
> > The list of paths per origin is a little different than the other extension
> > permissions the 'policy' reflects.
> > 
> > Would it make sense to keep the specific collection of 'paths' an origin has
> > access to in the CrosMountPointProvider instead of in this policy class? So
> the
> > division of labor is that the 'policy' will tell you if hosted_app or
> extension
> > associated with origin has the 'localfile' permission, and the
> > mountPointProvider would keep track of which virtual_paths each origin with
> that
> > permission can actually poke at.
> 
> I could move this around, but I felt that this class generically deals with
file
> system permissions from extension context. All file driven extension
permissions
> are handed here as well.

I do think it may be better to encapsulate the set of cros local directories an
origin poke at in the cros mountpoint provider.

* Its not clear that the new methods on FileSystemMountPointProvider will apply
to anything other than the CrosMountPointProvider and i don't think the method
signatures currently in place convey enough info to be useful...
sandbox_provider->GrantAccess(<someorigin>, FilePath('/'))... not sure what that
would mean?

* The motivation behind the SpecialStoragePolicy class is to provide an
abstraction thru which code under webkit/* can figure out if an extension or
hostedApp has been installed with 'extra rights' at a given origin. So putting
this cros local filesystem specific state (map of origins to 'virtual_paths') in
there really jumps out at me.

* If/when we do a desktop os local filesystem provider, it may be easier to work
on that as a separate piece (and perhaps borrow/refactor the cros specific code
if applicable).

Also... just want to be sure that the intent is to grant access to 'hostedApps'
living at http urls in addition to 'extensions' living at chrome-extension urls.
If it's just extensions and not apps things could be simplified is why i ask.

[michaeln, 2011-04-14 01:26:25]

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/6810037/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.cc:199: if
(iter->second->OverlapsWithOrigin(origin))
ooops... forgot to hit send earlier today...

Consider replacing this with a test for equality instead of groping the
webextent... or even better a map lookup.

Also, based on our discussion, this 'right' should only be grantable to
extension.url() (which i believe equates to the chrome-extensions://<id>
origin). In this sense, this SpecialCollection subclass is somewhat different
from the base class which are much more about extending the 'right' to the
webextent of a hosted app... that's why the base class uses
OverlapsWithOrigin().

[zel, 2011-04-14 20:10:52]

PTAL

I have implemented all suggestions from Eric and Michael in this iteration -
access control for individual files for 'external' file systems is not govern by
SpecialStoragePolicy  anymore

[michaeln, 2011-04-14 21:25:34]

Thank you for revisiting the StoragePolicy parts of this change!

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.cc:7: #include
"base/command_line.h"
still needed?

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.cc:9: #include
"chrome/common/chrome_switches.h"
still needed?

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:10: #include <set>
needed?

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:13: #include
"base/file_path.h"
needed?

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:31: virtual bool
IsFileHanlder(const std::string& extension_id);
typo... IsFileHandler

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:42: virtual
~SpecialCollection();
this can be de-virtualized now

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:48: bool
ContainsExtension(const std::string& extension_id);
nit: move this method declaration up under Contains(origin) and similarly keep
the methods in the .cc file in the same order as declared

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:50: protected:
can remain private

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:31: {"/",                  
"media"},
strange that media and tmp both map to the same thing, and that it's '/'

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:35: const char
CrosMountPointProvider::kExternalName[] = "External";
should this string value be specific to CROS some how... prefixed?

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:124: const std::string&
extension_id, const FilePath& virtual_path) {
would it make sense to test IsFileHandler() here too?

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context_unittest.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context_unittest.cc:37: virtual bool
IsFileHanlder(const std::string& unused) {
nit: might as well name the unused param extension_id to make it a little less
mysterious

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager_unittest.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager_unittest.cc:176: virtual bool
IsFileHanlder(const std::string& unused) {
nit: unused -> extension_id

http://codereview.chromium.org/6810037/diff/21016/webkit/quota/special_storag...
File webkit/quota/special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/quota/special_storag...
webkit/quota/special_storage_policy.h:12: class FilePath;
don't think this is needed anymore

http://codereview.chromium.org/6810037/diff/21016/webkit/quota/special_storag...
webkit/quota/special_storage_policy.h:33: // Unlimited storage is not subject to
'quotas'.
copy/paste comment does not apply... and method name typo

[zel, 2011-04-14 21:46:20]

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.cc:7: #include
"base/command_line.h"
On 2011/04/14 21:25:34, michaeln wrote:
> still needed?

Done.

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.cc:9: #include
"chrome/common/chrome_switches.h"
On 2011/04/14 21:25:34, michaeln wrote:
> still needed?

Done.

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:10: #include <set>
On 2011/04/14 21:25:34, michaeln wrote:
> needed?

Done.

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:13: #include
"base/file_path.h"
On 2011/04/14 21:25:34, michaeln wrote:
> needed?

Done.

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:31: virtual bool
IsFileHanlder(const std::string& extension_id);
On 2011/04/14 21:25:34, michaeln wrote:
> typo... IsFileHandler

Done.

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:42: virtual
~SpecialCollection();
On 2011/04/14 21:25:34, michaeln wrote:
> this can be de-virtualized now

Done.

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:48: bool
ContainsExtension(const std::string& extension_id);
On 2011/04/14 21:25:34, michaeln wrote:
> nit: move this method declaration up under Contains(origin) and similarly keep
> the methods in the .cc file in the same order as declared

Done.

http://codereview.chromium.org/6810037/diff/21016/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:50: protected:
On 2011/04/14 21:25:34, michaeln wrote:
> can remain private

Done.

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:31: {"/",                  
"media"},
On 2011/04/14 21:25:34, michaeln wrote:
> strange that media and tmp both map to the same thing, and that it's '/'

Actually they don't. The mapping is following:

tmp -> /tmp
media -> /media

This data definition was just friendlier to GetFileSystemRootPath()
implementation.

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:35: const char
CrosMountPointProvider::kExternalName[] = "External";
On 2011/04/14 21:25:34, michaeln wrote:
> should this string value be specific to CROS some how... prefixed?

Darin said that 'external' is already part of Pepper and that we should have in
on WebCore side as well. So, I don't think that this part is ChromeOS-specific
since all parsing and url creation methods need to understand it as well. See
https://bugs.webkit.org/show_bug.cgi?id=58456 for details.

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:124: const std::string&
extension_id, const FilePath& virtual_path) {
On 2011/04/14 21:25:34, michaeln wrote:
> would it make sense to test IsFileHandler() here too?

The code won't let you get here unless the extension is a handler, but DCHECK is
surely appropriate. Done.

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context_unittest.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context_unittest.cc:37: virtual bool
IsFileHanlder(const std::string& unused) {
On 2011/04/14 21:25:34, michaeln wrote:
> nit: might as well name the unused param extension_id to make it a little less
> mysterious

Done.

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager_unittest.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager_unittest.cc:176: virtual bool
IsFileHanlder(const std::string& unused) {
On 2011/04/14 21:25:34, michaeln wrote:
> nit: unused -> extension_id

Done.

http://codereview.chromium.org/6810037/diff/21016/webkit/quota/special_storag...
File webkit/quota/special_storage_policy.h (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/quota/special_storag...
webkit/quota/special_storage_policy.h:12: class FilePath;
On 2011/04/14 21:25:34, michaeln wrote:
> don't think this is needed anymore

Done.

http://codereview.chromium.org/6810037/diff/21016/webkit/quota/special_storag...
webkit/quota/special_storage_policy.h:33: // Unlimited storage is not subject to
'quotas'.
On 2011/04/14 21:25:34, michaeln wrote:
> copy/paste comment does not apply... and method name typo

Done.

[michaeln, 2011-04-14 22:36:58]

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:31: {"/",                  
"media"},
> Actually they don't. The mapping is following:
>   tmp -> /tmp
>   media -> /media
> This data definition was just friendlier to GetFileSystemRootPath()
> implementation.  

ah... got it!

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:35: const char
CrosMountPointProvider::kExternalName[] = "External";
On 2011/04/14 21:46:20, zel wrote:
> On 2011/04/14 21:25:34, michaeln wrote:
> > should this string value be specific to CROS some how... prefixed?
> 
> Darin said that 'external' is already part of Pepper and that we should have
in
> on WebCore side as well. So, I don't think that this part is ChromeOS-specific
> since all parsing and url creation methods need to understand it as well. See
> https://bugs.webkit.org/show_bug.cgi?id=58456 for details.

ok... i'm not so familiar with the plan for extending this to 'external' file
systems, it just caught my eye that cros specific .cc files held generic looking
string identifiers.

Does kExternalDirName[] need to match the value defined in file_system_util.cc
   static const char kExternalDir[] = "/external/";
if so, we should probably have one source for this value in chrome code. And is
there a similar redifinition of CrosMountPointProvider::kExternalName somewhere
else in chrome sources that needs to be kept in sync?

http://codereview.chromium.org/6810037/diff/21022/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:132: }
Did you put the DCHECK go in the wrong method or did you mean to DCHECK() and
also branch in non-debug code?

[zel, 2011-04-14 22:53:03]

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6810037/diff/21016/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:35: const char
CrosMountPointProvider::kExternalName[] = "External";
On 2011/04/14 22:36:58, michaeln wrote:
> On 2011/04/14 21:46:20, zel wrote:
> > On 2011/04/14 21:25:34, michaeln wrote:
> > > should this string value be specific to CROS some how... prefixed?
> > 
> > Darin said that 'external' is already part of Pepper and that we should have
> in
> > on WebCore side as well. So, I don't think that this part is
ChromeOS-specific
> > since all parsing and url creation methods need to understand it as well.
See
> > https://bugs.webkit.org/show_bug.cgi?id=58456 for details.
> 
> ok... i'm not so familiar with the plan for extending this to 'external' file
> systems, it just caught my eye that cros specific .cc files held generic
looking
> string identifiers.
> 
> Does kExternalDirName[] need to match the value defined in file_system_util.cc
>    static const char kExternalDir[] = "/external/";
> if so, we should probably have one source for this value in chrome code. And
is
> there a similar redifinition of CrosMountPointProvider::kExternalName
somewhere
> else in chrome sources that needs to be kept in sync?

I have moved all these constants to file_system_util.h/cc where most of the
others were defined.

http://codereview.chromium.org/6810037/diff/21022/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:132: }
On 2011/04/14 22:36:58, michaeln wrote:
> Did you put the DCHECK go in the wrong method or did you mean to DCHECK() and
> also branch in non-debug code?

yes, wrong place. done

[michaeln, 2011-04-15 01:02:01]

thank you for taking the trouble to massage things

ok... from my limited point of view... lgtm,
but u should probably wait for eric to take give it a look too?

[ericu, 2011-04-15 01:46:41]

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:126: // All we care about
here is access from extensions access for now.
extra "access"

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
File webkit/chromeos/fileapi/file_access_permissions.cc (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
webkit/chromeos/fileapi/file_access_permissions.cc:40: // has access to it.
Don't you need to loop over the paths in the PathSet that is *path_map_iter?

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
File webkit/chromeos/fileapi/file_access_permissions.h (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
webkit/chromeos/fileapi/file_access_permissions.h:6: #define
CHROME_BROWSER_EXTENSIONS_EXTENSION_SPECIAL_STORAGE_POLICY_H_
Wrong guard macro.

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:26: const FilePath&
virtual_path) = 0;
I thought we'd decided that virtual_path doesn't work here--it's not enough info
for the general case, which would need a full filesystem GURL.

Alternatively I guess we could get away with origin, type, virtual_path, which
would then match the method in FileSystemPathManager.

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:55: // An interface to cotnrol
external file system access permissions.
typo: control

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:65: const FilePath&
virtual_path) = 0;
Is this a virtual path, or a real full path?

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.h:86: bool IsAllowedFileSystemType(const
GURL& origin, FileSystemType type,
This is no longer really about the type.  How about changing the name to
IsAccessAllowed?

SandboxMPP just turns around and calls IsAllowedScheme, and will probably just
keep doing that.  We should just move IsAllowedScheme to SandboxMPP [inlined in
the body of the impl of this method], BTW, but that can wait until a later CL.

[zel, 2011-04-15 02:18:55]

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:126: // All we care about
here is access from extensions access for now.
On 2011/04/15 01:46:42, ericu wrote:
> extra "access"

Done.

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
File webkit/chromeos/fileapi/file_access_permissions.cc (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
webkit/chromeos/fileapi/file_access_permissions.cc:40: // has access to it.
On 2011/04/15 01:46:42, ericu wrote:
> Don't you need to loop over the paths in the PathSet that is *path_map_iter?

It actually traverses dir structure backward from the file that we are testing
to see if any of its parent dirs is permitted. We do the same type of file
access check in ChildProcessSecurityPolicy.

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
File webkit/chromeos/fileapi/file_access_permissions.h (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
webkit/chromeos/fileapi/file_access_permissions.h:6: #define
CHROME_BROWSER_EXTENSIONS_EXTENSION_SPECIAL_STORAGE_POLICY_H_
On 2011/04/15 01:46:42, ericu wrote:
> Wrong guard macro.

whoops, good catch... don't know how I haven't see this in presumbit before.
done.

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:26: const FilePath&
virtual_path) = 0;
On 2011/04/15 01:46:42, ericu wrote:
> I thought we'd decided that virtual_path doesn't work here--it's not enough
info
> for the general case, which would need a full filesystem GURL.
> 
> Alternatively I guess we could get away with origin, type, virtual_path, which
> would then match the method in FileSystemPathManager.

Sorry, I've missed that one. Done.

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:55: // An interface to cotnrol
external file system access permissions.
On 2011/04/15 01:46:42, ericu wrote:
> typo: control

Done.

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:65: const FilePath&
virtual_path) = 0;
On 2011/04/15 01:46:42, ericu wrote:
> Is this a virtual path, or a real full path?

This is virtual path (which corresponds to Entry.fullPath).

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.h:86: bool IsAllowedFileSystemType(const
GURL& origin, FileSystemType type,
On 2011/04/15 01:46:42, ericu wrote:
> This is no longer really about the type.  How about changing the name to
> IsAccessAllowed?
> 
> SandboxMPP just turns around and calls IsAllowedScheme, and will probably just
> keep doing that.  We should just move IsAllowedScheme to SandboxMPP [inlined
in
> the body of the impl of this method], BTW, but that can wait until a later CL.

Rename done.

[ericu, 2011-04-15 02:31:04]

LGTM.

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
File webkit/chromeos/fileapi/file_access_permissions.cc (right):

http://codereview.chromium.org/6810037/diff/32003/webkit/chromeos/fileapi/fil...
webkit/chromeos/fileapi/file_access_permissions.cc:40: // has access to it.
On 2011/04/15 02:18:55, zel wrote:
> On 2011/04/15 01:46:42, ericu wrote:
> > Don't you need to loop over the paths in the PathSet that is *path_map_iter?
> 
> It actually traverses dir structure backward from the file that we are testing
> to see if any of its parent dirs is permitted. We do the same type of file
> access check in ChildProcessSecurityPolicy.
> 
>  

Ah--I see--I was misreading the find as being in a string, not a set.