Implementation of the device attributes API.

chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc

Index: chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
diff --git a/chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc b/chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..925943f36d48694e74d834ed2aa59d40b6e6a117
--- /dev/null
+++ b/chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_apitest.cc
@@ -0,0 +1,164 @@
+// Copyright (c) 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/chromeos/policy/device_policy_cros_browser_test.h"
+#include "chrome/browser/chromeos/policy/stub_enterprise_install_attributes.h"
+#include "chrome/browser/extensions/api/enterprise_device_attributes/enterprise_device_attributes_api.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "chromeos/dbus/fake_session_manager_client.h"
+#include "chromeos/login/user_names.h"
+#include "extensions/browser/api_test_utils.h"
+#include "extensions/common/manifest_constants.h"
+#include "extensions/common/permissions/permissions_info.h"
+
+namespace {
+  const std::string kDeviceId = "device_id";

[pneubeck (no reviews), 2015/07/23 12:53:22]

nit: indentation is wrong (just run 'git cl format')

[Polina Bondarenko, 2015/07/24 16:47:58]

Done.

+}
+
+namespace extensions {
+
+// Base class for apitests - inits the testing enviroment init, provides API for

[pneubeck (no reviews), 2015/07/23 12:53:22]

this comment can be removed

instead rename it to include 'Test' in the class name.
you can also move it into the anonymous namespace

[Polina Bondarenko, 2015/07/24 16:47:59]

Done.

+// testing.
+class EnterpriseDeviceAttributesBase : public ExtensionApiTest {
+ public:
+  EnterpriseDeviceAttributesBase()
+      : fake_session_manager_client_(new chromeos::FakeSessionManagerClient) {
+

[pneubeck (no reviews), 2015/07/23 12:53:22]

nit: remove empty line

[Polina Bondarenko, 2015/07/24 16:47:59]

Done.

+  }
+
+ protected:
+  void SetUpInProcessBrowserTestFixture() override {
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
+      scoped_ptr<chromeos::SessionManagerClient>(fake_session_manager_client_));
+    ExtensionApiTest::SetUpInProcessBrowserTestFixture();
+  }
+
+  // Inits the device policy and install attributes, should be invoked before
+  // browser init.
+  void InitTest(const std::string& domain) {
+    // Set up fake install attributes.
+    scoped_ptr<policy::StubEnterpriseInstallAttributes> attributes(
+        new policy::StubEnterpriseInstallAttributes());
+
+    attributes->SetDomain(domain);
+    attributes->SetRegistrationUser(chromeos::login::kStubUser);
+    policy::BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(
+        attributes.release());
+
+    test_helper_.InstallOwnerKey();
+    // Init the device policy.
+    policy::DevicePolicyBuilder* device_policy = test_helper_.device_policy();
+    device_policy->SetDefaultSigningKey();
+    device_policy->policy_data().set_directory_api_id(kDeviceId);
+    device_policy->Build();
+
+    fake_session_manager_client_->set_device_policy(device_policy->GetBlob());
+    fake_session_manager_client_->OnPropertyChangeComplete(true);
+  }
+
+  // Runs test with the expected directory device ID value and the extension
+  // location.
+  void RunTest(const std::string& expect_value,
+               const Manifest::Location location) {
+    // Init the testing extension function.
+    scoped_refptr<EnterpriseDeviceAttributesGetDirectoryDeviceIdFunction>
+    get_device_id_function(
+        new EnterpriseDeviceAttributesGetDirectoryDeviceIdFunction());
+
+    // |feature_full_name| = permission:<Extension API name>
+    // The get_device_id_function - is a permission feature.

[pneubeck (no reviews), 2015/07/23 12:53:22]

I don't understand this part of the comment

[Polina Bondarenko, 2015/07/24 16:47:59]

Done.

+    std::string feature_full_name = std::string("permission:") +
+        EnterpriseDeviceAttributesGetDirectoryDeviceIdFunction::function_name();
+    feature_full_name.erase(feature_full_name.find_last_of('.'),
+                            feature_full_name.size());
+    get_device_id_function->set_name(feature_full_name.c_str());

[pneubeck (no reviews), 2015/07/23 12:53:22]

as far as I understand, you're testing several orthogonal things here:

1) Whether the getDirectoryDeviceId function is returning the right result
depending on the user affiliation

2) Whether ...function->HasPermission() is returning the right result

Only for 2) you seem to have to call set_name with the right argument.
I think you should either rely on 2) to be implemented correctly in the
extension system or at least separate that into another test.

[Polina Bondarenko, 2015/07/24 16:47:58]

Done.

+    get_device_id_function->set_has_callback(true);
+
+    // Init manifest for the extension.
+    base::DictionaryValue manifest;
+    manifest.SetString(manifest_keys::kVersion, "1.0.0.0");
+    manifest.SetString(manifest_keys::kName, "test");
+    base::ListValue* permission_list = new base::ListValue;
+    // Should have permission to use the testing extension API.
+    permission_list->Append(
+        new base::StringValue(PermissionsInfo::GetInstance()->
+            GetByID(APIPermission::kEnterpriseDeviceAttributes)->name()));
+    manifest.Set(manifest_keys::kPermissions, permission_list);
+
+    // Creates the extension with appropriate location and permissions.
+    scoped_refptr<extensions::Extension> extension =
+        api_test_utils::CreateExtension(location, &manifest, "");
+    get_device_id_function->set_extension(extension);
+
+    // Check if the extension has permissions to the testing function.
+    EXPECT_EQ(location == Manifest::EXTERNAL_POLICY,
+              get_device_id_function->HasPermission());
+
+    // Run the testing function and check result.
+    scoped_ptr<base::Value> result(
+        api_test_utils::RunFunctionAndReturnSingleResult(
+        get_device_id_function.get(), "[]", browser()->profile()));
+
+    std::string value;
+    EXPECT_TRUE(result->GetAsString(&value));
+    EXPECT_EQ(expect_value, value);
+  }
+
+ private:
+  chromeos::FakeSessionManagerClient* fake_session_manager_client_;
+  policy::DevicePolicyCrosTestHelper test_helper_;
+};
+
+// Creates affiliated user before browser init.

[pneubeck (no reviews), 2015/07/23 12:53:22]

nit: s/init/initializes/

[Polina Bondarenko, 2015/07/24 16:47:59]

Done.

+class EnterpriseDeviceAttributesAffiliatedTest :
+  public EnterpriseDeviceAttributesBase {
+ protected:
+  void SetUpInProcessBrowserTestFixture() override {
+    EnterpriseDeviceAttributesBase::SetUpInProcessBrowserTestFixture();
+
+    InitTest(kDomain);
+  }
+ private:
+  const std::string kDomain = "gmail.com";

[pneubeck (no reviews), 2015/07/23 12:53:22]

could you instead pass this to the Base constructor or move it to a virtual
function that you override in each derived class?
That way you could remove SetUpInProcess... from the derived classes.
And move InitTest into
EnterpriseDeviceAttributesBase::SetUpInProcessBrowserTestFixture

[Polina Bondarenko, 2015/07/24 16:47:58]

Done.

+};
+
+// Creates non-affiliated user before browser init.

[pneubeck (no reviews), 2015/07/23 12:53:22]

nit: s/init/initializes/

[Polina Bondarenko, 2015/07/24 16:47:58]

Done.

+class EnterpriseDeviceAttributesNonAffiliatedTest :
+  public EnterpriseDeviceAttributesBase {
+ protected:
+  void SetUpInProcessBrowserTestFixture() override {
+    EnterpriseDeviceAttributesBase::SetUpInProcessBrowserTestFixture();
+
+    InitTest(kDomain);
+  }
+ private:
+  const std::string kDomain = "example.com";
+};
+
+// Tests the case when the user is affiliated and the extension has been

[pneubeck (no reviews), 2015/07/23 12:53:22]

better describe what the rule or fact is that you want to check.
currently you're only describing the precondition and don't mention the
expectation.

same for the other tests.

[Polina Bondarenko, 2015/07/24 16:47:59]

Done.

+// force installed by policy.
+IN_PROC_BROWSER_TEST_F(EnterpriseDeviceAttributesAffiliatedTest, Success) {
+  RunTest(kDeviceId, Manifest::EXTERNAL_POLICY);
+}
+
+// Tests the case when the user is affiliated and the extension is the part of
+// external component of Chrome OS.
+IN_PROC_BROWSER_TEST_F(EnterpriseDeviceAttributesAffiliatedTest, Failure) {
+    RunTest(kDeviceId, Manifest::EXTERNAL_COMPONENT);

[pneubeck (no reviews), 2015/07/23 12:53:22]

why component?
you might instead want to ensure that user downloaded extensions are not able to
use the api:  EXTERNAL_PREF_DOWNLOAD

the expected result of the function should then be "" and not kDeviceId.

[Polina Bondarenko, 2015/07/24 16:47:58]

Done.

+}
+
+// Tests the case when the user is non affiliated and the extension has been
+// force installed by policy.
+IN_PROC_BROWSER_TEST_F(EnterpriseDeviceAttributesNonAffiliatedTest,
+                       EmptyString) {
+  RunTest("", Manifest::EXTERNAL_POLICY);
+}
+
+// Tests the case when the user is non affiliated and the extension is the part
+// of an integrat component of Chrome.
+IN_PROC_BROWSER_TEST_F(EnterpriseDeviceAttributesNonAffiliatedTest, Failure) {
+  RunTest("", Manifest::COMPONENT);
+}
+} //  namespace extensions