[CHrome OS] Provides nicer abstractions around ownership API

[CHrome OS] Provides nicer abstractions around ownership API

There are two categories of operations that can be performed on the Chrome OS owner-signed settings store:
1) doing stuff to the whitelist (adding/removing/checking)
2) Storing/Retrieving arbitrary name=value pairs

Unfortunately, it is currently a limitation that only one of a each category can be in-flight at a time.  I've filed an issue on me to remove that restriction.

The pattern of use here is that the caller instantiates some subclass of SignedSettings by calling one of the create methods.  Then, call Execute() on this object from the UI thread.  It'll go off and do work (on the FILE thread and over DBus), and then call the appropriate method of the Delegate you passed in -- again, on the UI thread.

BUG=chromium-os:4488
TEST=Unit tests

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

[Chris Masone, 2010-09-02 15:40:40]

Probably going to take all day to get try runs that don't time out during the
link stage or fail UI tests on a platform where this code isn't even compiled,
but PTAL.

[Chris Masone, 2010-09-03 00:41:32]

Per hallway discussion with Zel about this API not being suitable for handling
bulk options/preferences:

Yes, that's true.  This isn't really intended to be general-purpose preference
storage.  Chrome already has facilities for that.  This is just intended to be
for settings that must be visible to all users but settable only by the owner of
the device.  If we hold to that, only being able to get/set one property at a
time shouldn't be that onerous.  That said, it'd definitely be doable to enable
the getting/setting of lists of properties in a single call.  I kind of wanted
to get the simple case right first, though.

On 2010/09/02 15:40:40, Chris Masone wrote:
> Probably going to take all day to get try runs that don't time out during the
> link stage or fail UI tests on a platform where this code isn't even compiled,
> but PTAL.

[kuan, 2010-09-03 10:24:39]

mostly nits, with 2 questions to help me understand usage of the API:

1) by "only one of each category can be in flight at a time", do u mean:
a) a whitelist op can't happen with a property op? or
b) a property retrieve op can't happen with another retrieve op or store op?

2) since bulk handling of properties is not supported, is there a limit to the
size of value? there'll be a few properties just for proxy config. if value can
be long, one way i cld work around is to serialize all info into one property
using delimiters. is this the preferred approach?

http://codereview.chromium.org/3345002/diff/2001/3001
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/3345002/diff/2001/3001#newcode20
chrome/browser/chromeos/login/signed_settings.cc:20: :
service_(OwnershipService::GetSharedInstance()){
space before {

http://codereview.chromium.org/3345002/diff/2001/3001#newcode28
chrome/browser/chromeos/login/signed_settings.cc:28:
SignedSettings::Delegate<bool>* d);
indentation

http://codereview.chromium.org/3345002/diff/2001/3001#newcode92
chrome/browser/chromeos/login/signed_settings.cc:92: 
only need 1 empty line

http://codereview.chromium.org/3345002/diff/2001/3001#newcode112
chrome/browser/chromeos/login/signed_settings.cc:112: }
insert an empty line

http://codereview.chromium.org/3345002/diff/2001/3001#newcode129
chrome/browser/chromeos/login/signed_settings.cc:129:
SignedSettings::Delegate<bool>* d)
indentation

http://codereview.chromium.org/3345002/diff/2001/3001#newcode213
chrome/browser/chromeos/login/signed_settings.cc:213: else
don't need else here?

http://codereview.chromium.org/3345002/diff/2001/3002
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/3345002/diff/2001/3002#newcode19
chrome/browser/chromeos/login/signed_settings.h:19: // Unfortunately, it is
currently a limitation that only one of a each
one of a each -> one of each

http://codereview.chromium.org/3345002/diff/2001/3002#newcode28
chrome/browser/chromeos/login/signed_settings.h:28: //-- again, on the UI
thread.
shld there be a space after //?

http://codereview.chromium.org/3345002/diff/2001/3003
File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):

http://codereview.chromium.org/3345002/diff/2001/3003#newcode24
chrome/browser/chromeos/login/signed_settings_unittest.cc:24: namespace chromeos
{
insert an empty line.

http://codereview.chromium.org/3345002/diff/2001/3003#newcode293
chrome/browser/chromeos/login/signed_settings_unittest.cc:293:
FailingStorePropertyOp(OwnerManager::KEY_UNAVAILABLE);
FailingRetrievePropertyOp

http://codereview.chromium.org/3345002/diff/2001/3003#newcode297
chrome/browser/chromeos/login/signed_settings_unittest.cc:297:
FailingStorePropertyOp(OwnerManager::OPERATION_FAILED);
FailingRetrievePropertyOp

[Chris Masone, 2010-09-03 14:41:54]

On Fri, Sep 3, 2010 at 3:24 AM, <[email protected]> wrote:

> mostly nits, with 2 questions to help me understand usage of the API:
>
> 1) by "only one of each category can be in flight at a time", do u mean:
> a) a whitelist op can't happen with a property op? or
> b) a property retrieve op can't happen with another retrieve op or store
> op?
>

I mean there can be only one whitelist/unwhitelist/checkwhitelist at a time
and one property store/retrieve at a time.  Basically, there are two storage
entities on the back end: the whitelist and the property store.  You can
talk to both of them at the same time, but only be doing a single operation
on each (right now).


>
> 2) since bulk handling of properties is not supported, is there a limit to
> the
> size of value? there'll be a few properties just for proxy config. if value
> can
> be long, one way i cld work around is to serialize all info into one
> property
> using delimiters. is this the preferred approach?
>
>
There isn't currently a limit to the size of the value, no.  As long as
you're always planning to get/set these properties as a group, then the
approach you suggest makes sense.


>
> http://codereview.chromium.org/3345002/diff/2001/3001
> File chrome/browser/chromeos/login/signed_settings.cc (right):
>
> http://codereview.chromium.org/3345002/diff/2001/3001#newcode20
> chrome/browser/chromeos/login/signed_settings.cc:20: :
> service_(OwnershipService::GetSharedInstance()){
> space before {
>
> http://codereview.chromium.org/3345002/diff/2001/3001#newcode28
> chrome/browser/chromeos/login/signed_settings.cc:28:
> SignedSettings::Delegate<bool>* d);
> indentation
>
> http://codereview.chromium.org/3345002/diff/2001/3001#newcode92
> chrome/browser/chromeos/login/signed_settings.cc:92:
> only need 1 empty line
>
> http://codereview.chromium.org/3345002/diff/2001/3001#newcode112
> chrome/browser/chromeos/login/signed_settings.cc:112: }
> insert an empty line
>
> http://codereview.chromium.org/3345002/diff/2001/3001#newcode129
> chrome/browser/chromeos/login/signed_settings.cc:129:
> SignedSettings::Delegate<bool>* d)
> indentation
>
> http://codereview.chromium.org/3345002/diff/2001/3001#newcode213
> chrome/browser/chromeos/login/signed_settings.cc:213: else
> don't need else here?
>
> http://codereview.chromium.org/3345002/diff/2001/3002
> File chrome/browser/chromeos/login/signed_settings.h (right):
>
> http://codereview.chromium.org/3345002/diff/2001/3002#newcode19
> chrome/browser/chromeos/login/signed_settings.h:19: // Unfortunately, it
>
> is currently a limitation that only one of a each
> one of a each -> one of each
>
> http://codereview.chromium.org/3345002/diff/2001/3002#newcode28
> chrome/browser/chromeos/login/signed_settings.h:28: //-- again, on the
> UI thread.
> shld there be a space after //?
>
> http://codereview.chromium.org/3345002/diff/2001/3003
> File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):
>
> http://codereview.chromium.org/3345002/diff/2001/3003#newcode24
> chrome/browser/chromeos/login/signed_settings_unittest.cc:24: namespace
> chromeos {
> insert an empty line.
>
> http://codereview.chromium.org/3345002/diff/2001/3003#newcode293
> chrome/browser/chromeos/login/signed_settings_unittest.cc:293:
> FailingStorePropertyOp(OwnerManager::KEY_UNAVAILABLE);
> FailingRetrievePropertyOp
>
> http://codereview.chromium.org/3345002/diff/2001/3003#newcode297
> chrome/browser/chromeos/login/signed_settings_unittest.cc:297:
> FailingStorePropertyOp(OwnerManager::OPERATION_FAILED);
> FailingRetrievePropertyOp
>
>
> http://codereview.chromium.org/3345002/show
>

[zel, 2010-09-03 16:10:35]

http://codereview.chromium.org/3345002/diff/2001/3001
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/3345002/diff/2001/3001#newcode251
chrome/browser/chromeos/login/signed_settings.cc:251: success =
CrosLibrary::Get()->GetLoginLibrary()->StorePropertyAsync(name_,
Is LoginLibrary the best place to put this in? Wound't something more generic
like SystemLibrary be a better choice?

http://codereview.chromium.org/3345002/diff/2001/3002
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/3345002/diff/2001/3002#newcode23
chrome/browser/chromeos/login/signed_settings.h:23: //The pattern of use here is
that the caller instantiates some
spaces after //

http://codereview.chromium.org/3345002/diff/2001/3002#newcode33
chrome/browser/chromeos/login/signed_settings.h:33: class SignedSettings :
public base::RefCountedThreadSafe<SignedSettings>,
"Signed-" is implementation method attribute that should not care about. I think
"Protected-" or a similar prefix would be more correct.

http://codereview.chromium.org/3345002/diff/10001/11002#newcode67
chrome/browser/chromeos/login/signed_settings.h:67: 
We should be able to fetch number of properties within the single read/write
call. Even though today we might have only few such properties, I expect that
their number will only grow. This problem will be especially visible on places
like options UI (dom). We will end up calling back into javascript during each
property read operation. It will also complicate code for all other places in
the code where you need to read N things out of the property store in order to
proceed with their processing.

[Chris Masone, 2010-09-03 16:18:55]

http://codereview.chromium.org/3345002/diff/2001/3001
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/3345002/diff/2001/3001#newcode20
chrome/browser/chromeos/login/signed_settings.cc:20: :
service_(OwnershipService::GetSharedInstance()){
On 2010/09/03 10:24:39, kuan wrote:
> space before {

Done.

http://codereview.chromium.org/3345002/diff/2001/3001#newcode28
chrome/browser/chromeos/login/signed_settings.cc:28:
SignedSettings::Delegate<bool>* d);
On 2010/09/03 10:24:39, kuan wrote:
> indentation

Done.

http://codereview.chromium.org/3345002/diff/2001/3001#newcode92
chrome/browser/chromeos/login/signed_settings.cc:92: 
On 2010/09/03 10:24:39, kuan wrote:
> only need 1 empty line

Done.

http://codereview.chromium.org/3345002/diff/2001/3001#newcode112
chrome/browser/chromeos/login/signed_settings.cc:112: }
On 2010/09/03 10:24:39, kuan wrote:
> insert an empty line

Done.

http://codereview.chromium.org/3345002/diff/2001/3001#newcode129
chrome/browser/chromeos/login/signed_settings.cc:129:
SignedSettings::Delegate<bool>* d)
On 2010/09/03 10:24:39, kuan wrote:
> indentation

Done.

http://codereview.chromium.org/3345002/diff/2001/3001#newcode213
chrome/browser/chromeos/login/signed_settings.cc:213: else
On 2010/09/03 10:24:39, kuan wrote:
> don't need else here?

Done.

http://codereview.chromium.org/3345002/diff/2001/3002
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/3345002/diff/2001/3002#newcode19
chrome/browser/chromeos/login/signed_settings.h:19: // Unfortunately, it is
currently a limitation that only one of a each
On 2010/09/03 10:24:39, kuan wrote:
> one of a each -> one of each

Done.

http://codereview.chromium.org/3345002/diff/2001/3002#newcode28
chrome/browser/chromeos/login/signed_settings.h:28: //-- again, on the UI
thread.
On 2010/09/03 10:24:39, kuan wrote:
> shld there be a space after //?

Done.

http://codereview.chromium.org/3345002/diff/2001/3003
File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):

http://codereview.chromium.org/3345002/diff/2001/3003#newcode24
chrome/browser/chromeos/login/signed_settings_unittest.cc:24: namespace chromeos
{
On 2010/09/03 10:24:39, kuan wrote:
> insert an empty line.

Done.

http://codereview.chromium.org/3345002/diff/2001/3003#newcode293
chrome/browser/chromeos/login/signed_settings_unittest.cc:293:
FailingStorePropertyOp(OwnerManager::KEY_UNAVAILABLE);
On 2010/09/03 10:24:39, kuan wrote:
> FailingRetrievePropertyOp

Good catch :-)

http://codereview.chromium.org/3345002/diff/2001/3003#newcode297
chrome/browser/chromeos/login/signed_settings_unittest.cc:297:
FailingStorePropertyOp(OwnerManager::OPERATION_FAILED);
On 2010/09/03 10:24:39, kuan wrote:
> FailingRetrievePropertyOp

Done.

[Chris Masone, 2010-09-03 16:36:05]

http://codereview.chromium.org/3345002/diff/2001/3001
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/3345002/diff/2001/3001#newcode251
chrome/browser/chromeos/login/signed_settings.cc:251: success =
CrosLibrary::Get()->GetLoginLibrary()->StorePropertyAsync(name_,
On 2010/09/03 16:10:35, zel wrote:
> Is LoginLibrary the best place to put this in? Wound't something more generic
> like SystemLibrary be a better choice?

It shares machinery with some of the other stuff in chromeos_login.h. 
SetOwnerKeyAsync uses the same signal-monitoring infrastructure for callbacks
with this stuff.  Also, I want to keep this fairly tightly associated with
setting the owner to emphasize that this is not general-purpose storage.  It's
only for stuff that's critical to protect.

http://codereview.chromium.org/3345002/diff/2001/3002
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/3345002/diff/2001/3002#newcode33
chrome/browser/chromeos/login/signed_settings.h:33: class SignedSettings :
public base::RefCountedThreadSafe<SignedSettings>,
On 2010/09/03 16:10:35, zel wrote:
> "Signed-" is implementation method attribute that should not care about. I
think
> "Protected-" or a similar prefix would be more correct.

Owner-, maybe?  That's what they are, settings writable only by the owner.

http://codereview.chromium.org/3345002/diff/10001/11002#newcode67
chrome/browser/chromeos/login/signed_settings.h:67: 
On 2010/09/03 16:10:35, zel wrote:
> We should be able to fetch number of properties within the single read/write
> call. Even though today we might have only few such properties, I expect that
> their number will only grow. This problem will be especially visible on places
> like options UI (dom). We will end up calling back into javascript during each
> property read operation. It will also complicate code for all other places in
> the code where you need to read N things out of the property store in order to
> proceed with their processing.

I kind of addressed these comments earlier in the thread.

First, I think we should be judicious about what stuff we're storing in this
signed storage.  If it doesn't need to be owner-only, storing it here is
overkill.

Second, I agree that your request would be useful, but I don't know that it's
necessary right now, and we kind of only have time for things that are
necessary.  For stuff that should be stored/retrieved as a group, just serialize
it and set it all in a blob as kuan suggested.  That will be FAR more efficient
than storing each one as a separately signed blob anyhow.

[kuan, 2010-09-03 17:03:12]

well, for now, i think i can take the approach of serializing the proxy info
into one property value, so this LGTM. :)