ensure feature and behavior parity in GCS

Author: scotthart

ci/cloudbuild/builds/cmake-install.sh

@@ -133,7 +133,6 @@ expected_dirs+=(
   ./include/google/cloud/storage/internal/grpc
   ./include/google/cloud/storage/internal/rest
   ./include/google/cloud/storage/mocks
-  ./include/google/cloud/storage/oauth2
   ./include/google/cloud/storage/testing
   # no gRPC services in google/cloud/workflows/type.
   ./include/google/cloud/workflows/type

google/cloud/credentials.cc

@@ -48,13 +48,13 @@ std::shared_ptr<Credentials> MakeImpersonateServiceAccountCredentials(
 std::shared_ptr<Credentials> MakeServiceAccountCredentials(
     std::string json_object, Options opts) {
   return std::make_shared<internal::ServiceAccountConfig>(
-      std::move(json_object), std::move(opts));
+      std::move(json_object), absl::nullopt, std::move(opts));
 }
 
 std::shared_ptr<Credentials> MakeServiceAccountCredentialsFromFile(
     std::string const& file_path, Options opts) {
-  return std::make_shared<internal::ServiceAccountConfig>("", file_path,
-                                                          std::move(opts));
+  return std::make_shared<internal::ServiceAccountConfig>(
+      absl::nullopt, file_path, std::move(opts));
 }
 
 std::shared_ptr<Credentials> MakeExternalAccountCredentials(

google/cloud/credentials.h

@@ -335,6 +335,9 @@ std::shared_ptr<Credentials> MakeServiceAccountCredentials(
  * [service account key]:
  * https://cloud.google.com/iam/docs/creating-managing-service-account-keys#iam-service-account-keys-create-cpp
  *
+ * Use `ScopesOption` to restrict the authentication scope for the obtained
+ * credentials.
+ *
  * @ingroup guac
  *
  * @note While JSON file formats are supported for both REST and gRPC transport,

google/cloud/internal/credentials_impl.cc

@@ -87,13 +87,9 @@ std::vector<std::string> const& ImpersonateServiceAccountConfig::delegates()
   return options_.get<DelegatesOption>();
 }
 
-ServiceAccountConfig::ServiceAccountConfig(std::string json_object,
-                                           Options opts)
-    : json_object_(std::move(json_object)),
-      options_(PopulateAuthOptions(std::move(opts))) {}
-
-ServiceAccountConfig::ServiceAccountConfig(std::string json_object,
-                                           std::string file_path, Options opts)
+ServiceAccountConfig::ServiceAccountConfig(
+    absl::optional<std::string> json_object,
+    absl::optional<std::string> file_path, Options opts)
     : json_object_(std::move(json_object)),
       file_path_(std::move(file_path)),
       options_(PopulateAuthOptions(std::move(opts))) {}

google/cloud/internal/credentials_impl.h

@@ -144,19 +144,23 @@ class ImpersonateServiceAccountConfig : public Credentials {
 
 class ServiceAccountConfig : public Credentials {
  public:
-  ServiceAccountConfig(std::string json_object, Options opts);
-  ServiceAccountConfig(std::string json_object, std::string file_path,
-                       Options opts);
-
-  std::string const& json_object() const { return json_object_; }
+  // Only one of json_object or file_path should have a value.
+  // TODO(#15886): Use the C++ type system to make better constructors that
+  //   enforces this comment.
+  ServiceAccountConfig(absl::optional<std::string> json_object,
+                       absl::optional<std::string> file_path, Options opts);
+
+  absl::optional<std::string> const& json_object() const {
+    return json_object_;
+  }
   absl::optional<std::string> const& file_path() const { return file_path_; }
   Options const& options() const { return options_; }
 
  private:
   void dispatch(CredentialsVisitor& v) const override { v.visit(*this); }
 
-  std::string json_object_;
-  absl::optional<std::string> file_path_ = absl::nullopt;
+  absl::optional<std::string> json_object_;
+  absl::optional<std::string> file_path_;
   Options options_;
 };
 

google/cloud/internal/oauth2_service_account_credentials.cc

@@ -13,15 +13,18 @@
 // limitations under the License.
 
 #include "google/cloud/internal/oauth2_service_account_credentials.h"
+#include "google/cloud/credentials.h"
 #include "google/cloud/internal/absl_str_join_quiet.h"
 #include "google/cloud/internal/getenv.h"
 #include "google/cloud/internal/make_jwt_assertion.h"
 #include "google/cloud/internal/make_status.h"
 #include "google/cloud/internal/oauth2_google_credentials.h"
 #include "google/cloud/internal/oauth2_universe_domain.h"
+#include "google/cloud/internal/parse_service_account_p12_file.h"
 #include "google/cloud/internal/rest_response.h"
 #include "google/cloud/internal/sign_using_sha256.h"
 #include <nlohmann/json.hpp>
+#include <fstream>
 #include <functional>
 
 namespace google {
@@ -240,6 +243,81 @@ StatusOr<std::string> MakeSelfSignedJWT(
                                  info.private_key);
 }
 
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromJsonContents(
+    std::string const& contents, Options const& options,
+    HttpClientFactory client_factory) {
+  auto info = ParseServiceAccountCredentials(contents, "memory");
+  if (!info) return info.status();
+
+  std::set<std::string> scopes;
+  if (options.has<ScopesOption>()) {
+    auto s = options.get<ScopesOption>();
+    scopes.insert(s.begin(), s.end());
+    info->scopes = std::move(scopes);
+  }
+
+  // Verify this is usable before returning it.
+  auto const tp = std::chrono::system_clock::time_point{};
+  auto const components = AssertionComponentsFromInfo(*info, tp);
+  auto jwt = MakeJWTAssertionNoThrow(components.first, components.second,
+                                     info->private_key);
+  if (!jwt) return jwt.status();
+  return StatusOr<std::shared_ptr<Credentials>>(
+      std::make_shared<ServiceAccountCredentials>(*info, options,
+                                                  std::move(client_factory)));
+}
+
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromJsonFilePath(
+    std::string const& path, Options const& options,
+    HttpClientFactory client_factory) {
+  std::ifstream is(path);
+  if (!is.is_open()) {
+    // We use kUnknown here because we don't know if the file does not exist, or
+    // if we were unable to open it for some other reason.
+    return google::cloud::internal::UnknownError(
+        "Cannot open credentials file " + path, GCP_ERROR_INFO());
+  }
+  std::string contents(std::istreambuf_iterator<char>{is}, {});
+  return CreateServiceAccountCredentialsFromJsonContents(
+      std::move(contents), options, std::move(client_factory));
+}
+
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromP12FilePath(
+    std::string const& path, Options const& options,
+    HttpClientFactory client_factory) {
+  auto info = ParseServiceAccountP12File(path);
+  if (!info) {
+    return std::move(info).status();
+  }
+  std::set<std::string> scopes;
+  if (options.has<ScopesOption>()) {
+    auto s = options.get<ScopesOption>();
+    scopes.insert(s.begin(), s.end());
+    info->scopes = std::move(scopes);
+  }
+
+  // These are supplied as extra parameters to this method, not in the P12
+  // file.
+  info->scopes = std::move(scopes);
+  return StatusOr<std::shared_ptr<Credentials>>(
+      std::make_shared<ServiceAccountCredentials>(*info, options,
+                                                  std::move(client_factory)));
+}
+
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromFilePath(std::string const& path,
+                                            Options const& options,
+                                            HttpClientFactory client_factory) {
+  auto credentials = CreateServiceAccountCredentialsFromJsonFilePath(
+      path, options, client_factory);
+  if (credentials) return *credentials;
+  return CreateServiceAccountCredentialsFromP12FilePath(
+      path, options, std::move(client_factory));
+}
+
 ServiceAccountCredentials::ServiceAccountCredentials(
     ServiceAccountCredentialsInfo info, Options options,
     HttpClientFactory client_factory)
@@ -313,7 +391,8 @@ bool ServiceAccountUseOAuth(ServiceAccountCredentialsInfo const& info) {
 }
 
 bool ServiceAccountCredentials::UseOAuth() {
-  return ServiceAccountUseOAuth(info_);
+  return options_.has<DisableSelfSignedJWTOption>() ||
+         ServiceAccountUseOAuth(info_);
 }
 
 StatusOr<AccessToken> ServiceAccountCredentials::GetTokenOAuth(

google/cloud/internal/oauth2_service_account_credentials.h

@@ -24,6 +24,7 @@
 #include "absl/types/optional.h"
 #include <chrono>
 #include <string>
+#include <variant>
 #include <vector>
 
 namespace google {
@@ -127,6 +128,30 @@ StatusOr<std::string> MakeSelfSignedJWT(
     ServiceAccountCredentialsInfo const& info,
     std::chrono::system_clock::time_point tp);
 
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromJsonContents(
+    std::string const& contents, Options const& options,
+    HttpClientFactory client_factory);
+
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromJsonFilePath(
+    std::string const& path, Options const& options,
+    HttpClientFactory client_factory);
+
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromP12FilePath(
+    std::string const& path, Options const& options,
+    HttpClientFactory client_factory);
+
+StatusOr<std::shared_ptr<Credentials>>
+CreateServiceAccountCredentialsFromFilePath(std::string const& path,
+                                            Options const& options,
+                                            HttpClientFactory client_factory);
+
+struct DisableSelfSignedJWTOption {
+  using Type = std::monostate;
+};
+
 /**
  * Implements service account credentials for REST clients.
  *

google/cloud/internal/unified_grpc_credentials.cc

@@ -120,9 +120,14 @@ std::shared_ptr<GrpcAuthenticationStrategy> CreateAuthenticationStrategy(
         std::string contents(std::istreambuf_iterator<char>{is}, {});
         result = std::make_unique<GrpcServiceAccountAuthentication>(
             std::move(contents), std::move(options));
-      } else {
+      } else if (cfg.json_object().has_value()) {
         result = std::make_unique<GrpcServiceAccountAuthentication>(
-            cfg.json_object(), std::move(options));
+            *cfg.json_object(), std::move(options));
+      } else {
+        result = std::make_unique<GrpcErrorCredentialsAuthentication>(
+            ErrorCredentialsConfig{InternalError(
+                "ServiceAccountConfig has neither json_object nor file_path",
+                GCP_ERROR_INFO())});
       }
     }
     void visit(ExternalAccountConfig const& cfg) override {

google/cloud/internal/unified_rest_credentials.cc

@@ -15,6 +15,7 @@
 #include "google/cloud/internal/unified_rest_credentials.h"
 #include "google/cloud/common_options.h"
 #include "google/cloud/internal/make_jwt_assertion.h"
+#include "google/cloud/internal/make_status.h"
 #include "google/cloud/internal/oauth2_access_token_credentials.h"
 #include "google/cloud/internal/oauth2_anonymous_credentials.h"
 #include "google/cloud/internal/oauth2_api_key_credentials.h"
@@ -25,8 +26,6 @@
 #include "google/cloud/internal/oauth2_google_credentials.h"
 #include "google/cloud/internal/oauth2_impersonate_service_account_credentials.h"
 #include "google/cloud/internal/oauth2_service_account_credentials.h"
-#include "google/cloud/internal/parse_service_account_p12_file.h"
-#include <fstream>
 
 namespace google {
 namespace cloud {
@@ -51,67 +50,6 @@ std::shared_ptr<oauth2_internal::Credentials> MakeErrorCredentials(
   return std::make_shared<oauth2_internal::ErrorCredentials>(std::move(status));
 }
 
-StatusOr<std::shared_ptr<oauth2_internal::Credentials>>
-CreateServiceAccountCredentialsFromJsonContents(
-    std::string const& contents, Options const& options,
-    oauth2_internal::HttpClientFactory client_factory) {
-  auto info =
-      oauth2_internal::ParseServiceAccountCredentials(contents, "memory");
-  if (!info) return info.status();
-  // Verify this is usable before returning it.
-  auto const tp = std::chrono::system_clock::time_point{};
-  auto const components = AssertionComponentsFromInfo(*info, tp);
-  auto jwt = internal::MakeJWTAssertionNoThrow(
-      components.first, components.second, info->private_key);
-  if (!jwt) return jwt.status();
-  return StatusOr<std::shared_ptr<oauth2_internal::Credentials>>(
-      std::make_shared<oauth2_internal::ServiceAccountCredentials>(
-          *info, options, std::move(client_factory)));
-}
-
-StatusOr<std::shared_ptr<oauth2_internal::Credentials>>
-CreateServiceAccountCredentialsFromJsonFilePath(
-    std::string const& path, absl::optional<std::set<std::string>>,
-    absl::optional<std::string>, Options const& options,
-    oauth2_internal::HttpClientFactory client_factory) {
-  std::ifstream is(path);
-  std::string contents(std::istreambuf_iterator<char>{is}, {});
-  return CreateServiceAccountCredentialsFromJsonContents(
-      std::move(contents), options, std::move(client_factory));
-}
-
-std::shared_ptr<oauth2_internal::Credentials>
-CreateServiceAccountCredentialsFromP12FilePath(
-    std::string const& path, absl::optional<std::set<std::string>> scopes,
-    absl::optional<std::string> subject, Options const& options,
-    oauth2_internal::HttpClientFactory client_factory) {
-  auto info = oauth2_internal::ParseServiceAccountP12File(path);
-  if (!info) {
-    return MakeErrorCredentials(std::move(info).status());
-  }
-  // These are supplied as extra parameters to this method, not in the P12
-  // file.
-  info->subject = std::move(subject);
-  info->scopes = std::move(scopes);
-  return std::make_shared<oauth2_internal::ServiceAccountCredentials>(
-      *info, options, std::move(client_factory));
-}
-
-std::shared_ptr<oauth2_internal::Credentials>
-CreateServiceAccountCredentialsFromFilePath(
-    std::string const& path, absl::optional<std::set<std::string>> scopes,
-    absl::optional<std::string> subject, Options const& options,
-    oauth2_internal::HttpClientFactory client_factory) {
-  auto credentials = CreateServiceAccountCredentialsFromJsonFilePath(
-      path, scopes, subject, options, client_factory);
-  if (credentials) {
-    return *credentials;
-  }
-  return CreateServiceAccountCredentialsFromP12FilePath(
-      path, std::move(scopes), std::move(subject), options,
-      std::move(client_factory));
-}
-
 }  // namespace
 
 std::shared_ptr<oauth2_internal::Credentials> MapCredentials(
@@ -165,18 +103,27 @@ std::shared_ptr<oauth2_internal::Credentials> MapCredentials(
 
     void visit(ServiceAccountConfig const& cfg) override {
       if (cfg.file_path().has_value()) {
-        result = Decorate(CreateServiceAccountCredentialsFromFilePath(
-                              *cfg.file_path(), {}, {}, cfg.options(),
-                              std::move(client_factory_)),
-                          cfg.options());
-      } else {
-        auto creds = CreateServiceAccountCredentialsFromJsonContents(
-            cfg.json_object(), cfg.options(), std::move(client_factory_));
+        auto creds =
+            oauth2_internal::CreateServiceAccountCredentialsFromFilePath(
+                *cfg.file_path(), cfg.options(), std::move(client_factory_));
+        if (creds) {
+          result = Decorate(*creds, cfg.options());
+        } else {
+          result = MakeErrorCredentials(std::move(creds).status());
+        }
+      } else if (cfg.json_object().has_value()) {
+        auto creds =
+            oauth2_internal::CreateServiceAccountCredentialsFromJsonContents(
+                *cfg.json_object(), cfg.options(), std::move(client_factory_));
         if (creds) {
           result = Decorate(std::move(*creds), cfg.options());
           return;
         }
         result = MakeErrorCredentials(std::move(creds).status());
+      } else {
+        result = MakeErrorCredentials(internal::InternalError(
+            "ServiceAccountConfig has neither json_object nor file_path",
+            GCP_ERROR_INFO()));
       }
     }
 

google/cloud/internal/unified_rest_credentials_test.cc

@@ -405,7 +405,7 @@ TEST(UnifiedRestCredentialsTest, ServiceAccount) {
   EXPECT_CALL(client_factory, Call).Times(0);
 
   auto const config =
-      internal::ServiceAccountConfig(contents.dump(), Options{});
+      internal::ServiceAccountConfig(contents.dump(), absl::nullopt, Options{});
   auto credentials = MapCredentials(config, client_factory.AsStdFunction());
   auto access_token = credentials->GetToken(now);
   ASSERT_STATUS_OK(access_token);