From d8a3012b12495cbad8059beb8f9c7ec3ff0aeed1 Mon Sep 17 00:00:00 2001 From: Serhii Lozynskyi Date: Wed, 3 Jun 2020 16:04:05 +0300 Subject: [PATCH] Refactor Permission structure in Decision API Change Permission from pair to a struct and add missing resource field. Resolves: OLPSUP-10514 Signed-off-by: Serhii Lozynskyi --- .../olp/authentication/AuthorizeResult.h | 65 ++++++++++++-- .../src/AuthenticationClientUtils.cpp | 22 +++-- .../src/AuthenticationClientUtils.h | 2 +- olp-cpp-sdk-authentication/src/Constants.cpp | 1 + olp-cpp-sdk-authentication/src/Constants.h | 1 + .../tests/DecisionApiClientTest.cpp | 62 ++++++++----- .../DecisionApiTest.cpp | 87 ++++++++++--------- .../AuthenticationClientTest.cpp | 5 +- 8 files changed, 164 insertions(+), 81 deletions(-) diff --git a/olp-cpp-sdk-authentication/include/olp/authentication/AuthorizeResult.h b/olp-cpp-sdk-authentication/include/olp/authentication/AuthorizeResult.h index ef1419e48..4f3f5bab2 100644 --- a/olp-cpp-sdk-authentication/include/olp/authentication/AuthorizeResult.h +++ b/olp-cpp-sdk-authentication/include/olp/authentication/AuthorizeResult.h @@ -32,6 +32,60 @@ namespace authentication { */ enum class DecisionType { kAllow, kDeny }; +/** + * @brief Represents the permission with the action, policy decision, and + * associated resource. + */ +class AUTHENTICATION_API Permission { + public: + /** + * @brief Sets the action associated with the resource. + * + * @param The action to associate with. + */ + void SetAction(std::string action) { action_ = std::move(action); } + + /** + * @brief Gets the action that is associated with the resource. + * + * @return A string that represents the action. + */ + const std::string& GetAction() const { return action_; } + + /** + * @brief Sets the resource with which the action and decision are associated. + * + * @param The resource to associate with the decision and action. + */ + void SetResource(std::string resource) { resource_ = std::move(resource); } + + /** + * @brief Gets the resource with which the action and decision are associated. + * + * @return The resource name. + */ + const std::string& GetResource() const { return resource_; } + + /** + * @brief Sets the decision associated with the resource. + * + * @param The decision to associate with the resource. + */ + void SetDecision(DecisionType decision) { decision_ = decision; } + + /** + * @brief Gets the decision associated with the resource. + * + * @return The decision for the associated resource. + */ + DecisionType GetDecision() const { return decision_; } + + private: + std::string action_; + std::string resource_; + DecisionType decision_; +}; + /** * @brief Represents each action-resource pair response with an individual * policy decision for that action: DENY or ALLOW. @@ -41,11 +95,6 @@ enum class DecisionType { kAllow, kDeny }; */ class AUTHENTICATION_API ActionResult { public: - /** - * @brief Represents the permission pair with the action and policy decision. - */ - using Permissions = std::pair; - /** * @brief Gets the overall policy decision. * @@ -77,20 +126,20 @@ class AUTHENTICATION_API ActionResult { * * @return The list of permissions. */ - const std::vector& GetPermissions() const { return permissions_; } + const std::vector& GetPermissions() const { return permissions_; } /** * @brief Sets the list of permissions. * * @param permissions The vector of the action-decision pair. */ - void SetPermissions(std::vector permissions) { + void SetPermissions(std::vector permissions) { permissions_ = std::move(permissions); } private: DecisionType decision_{DecisionType::kDeny}; - std::vector permissions_; + std::vector permissions_; }; /** diff --git a/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.cpp b/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.cpp index e2904aeb7..499040af0 100644 --- a/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.cpp +++ b/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.cpp @@ -193,7 +193,7 @@ IntrospectAppResult GetIntrospectAppResult(const rapidjson::Document& doc) { return result; } -DecisionType GetPermission(const std::string& str) { +DecisionType GetDecision(const std::string& str) { return (str.compare("allow") == 0) ? DecisionType::kAllow : DecisionType::kDeny; } @@ -205,22 +205,26 @@ std::vector GetDiagnostics(rapidjson::Document& doc) { ActionResult action; if (element.HasMember(Constants::DECISION)) { action.SetDecision( - GetPermission(element[Constants::DECISION].GetString())); + GetDecision(element[Constants::DECISION].GetString())); // get permissions if avialible if (element.HasMember(Constants::PERMISSIONS) && element[Constants::PERMISSIONS].IsArray()) { - std::vector permissions; + std::vector permissions; const auto& permissions_array = element[Constants::PERMISSIONS].GetArray(); for (auto& permission_element : permissions_array) { - ActionResult::Permissions permission; + Permission permission; if (permission_element.HasMember(Constants::ACTION)) { - permission.first = - permission_element[Constants::ACTION].GetString(); + permission.SetAction( + permission_element[Constants::ACTION].GetString()); } if (permission_element.HasMember(Constants::DECISION)) { - permission.second = GetPermission( - permission_element[Constants::DECISION].GetString()); + permission.SetDecision(GetDecision( + permission_element[Constants::DECISION].GetString())); + } + if (permission_element.HasMember(Constants::RESOURCE)) { + permission.SetResource( + permission_element[Constants::RESOURCE].GetString()); } permissions.push_back(std::move(permission)); } @@ -247,7 +251,7 @@ AuthorizeResult GetAuthorizeResult(rapidjson::Document& doc) { } if (doc.HasMember(Constants::DECISION)) { - result.SetDecision(GetPermission(doc[Constants::DECISION].GetString())); + result.SetDecision(GetDecision(doc[Constants::DECISION].GetString())); } // get diagnostics if available diff --git a/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.h b/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.h index da1a1929c..d1043afc9 100644 --- a/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.h +++ b/olp-cpp-sdk-authentication/src/AuthenticationClientUtils.h @@ -95,7 +95,7 @@ IntrospectAppResult GetIntrospectAppResult(const rapidjson::Document& doc); * @param str string representation of decision. * @return result DecisionType. */ -DecisionType GetPermission(const std::string& str); +DecisionType GetDecision(const std::string& str); /* * @brief Parse json document to vector of ActionResults. diff --git a/olp-cpp-sdk-authentication/src/Constants.cpp b/olp-cpp-sdk-authentication/src/Constants.cpp index 081cf6701..09c878957 100644 --- a/olp-cpp-sdk-authentication/src/Constants.cpp +++ b/olp-cpp-sdk-authentication/src/Constants.cpp @@ -58,6 +58,7 @@ const char* Constants::IDENTITY = "identity"; const char* Constants::USER_ID = "userId"; const char* Constants::DECISION = "decision"; const char* Constants::ACTION = "action"; +const char* Constants::RESOURCE = "resource"; const char* Constants::PERMISSIONS = "permissions"; const char* Constants::DIAGNOSTICS = "diagnostics"; diff --git a/olp-cpp-sdk-authentication/src/Constants.h b/olp-cpp-sdk-authentication/src/Constants.h index 1414f457c..d1b1ef798 100644 --- a/olp-cpp-sdk-authentication/src/Constants.h +++ b/olp-cpp-sdk-authentication/src/Constants.h @@ -64,6 +64,7 @@ class Constants { static const char* USER_ID; static const char* DECISION; static const char* ACTION; + static const char* RESOURCE; static const char* PERMISSIONS; static const char* DIAGNOSTICS; }; diff --git a/olp-cpp-sdk-authentication/tests/DecisionApiClientTest.cpp b/olp-cpp-sdk-authentication/tests/DecisionApiClientTest.cpp index 72b60da63..af1df3cac 100644 --- a/olp-cpp-sdk-authentication/tests/DecisionApiClientTest.cpp +++ b/olp-cpp-sdk-authentication/tests/DecisionApiClientTest.cpp @@ -22,18 +22,21 @@ #include namespace { -using namespace olp::authentication; +namespace auth = olp::authentication; TEST(DecisionApiClientTest, AuthorizeRequestTest) { - EXPECT_EQ(AuthorizeRequest().WithServiceId("ServiceId").GetServiceId(), + EXPECT_EQ(auth::AuthorizeRequest().WithServiceId("ServiceId").GetServiceId(), "ServiceId"); - EXPECT_EQ( - AuthorizeRequest().WithContractId("ContractId").GetContractId().get(), - "ContractId"); - auto request = AuthorizeRequest().WithAction("action1").WithAction( + EXPECT_EQ(auth::AuthorizeRequest() + .WithContractId("ContractId") + .GetContractId() + .get(), + "ContractId"); + auto request = auth::AuthorizeRequest().WithAction("action1").WithAction( "action2", std::string("hrn::test")); - EXPECT_EQ(AuthorizeRequest().GetDiagnostics(), false); - EXPECT_EQ(AuthorizeRequest().WithDiagnostics(true).GetDiagnostics(), true); + EXPECT_EQ(auth::AuthorizeRequest().GetDiagnostics(), false); + EXPECT_EQ(auth::AuthorizeRequest().WithDiagnostics(true).GetDiagnostics(), + true); EXPECT_EQ(request.GetActions().size(), 2); auto actions_it = request.GetActions().begin(); EXPECT_EQ(actions_it->first, "action1"); @@ -42,10 +45,10 @@ TEST(DecisionApiClientTest, AuthorizeRequestTest) { EXPECT_EQ(actions_it->first, "action2"); EXPECT_EQ(actions_it->second, "hrn::test"); EXPECT_EQ(request.GetOperatorType(), - AuthorizeRequest::DecisionOperatorType::kAnd); - request.WithOperatorType(AuthorizeRequest::DecisionOperatorType::kOr); + auth::AuthorizeRequest::DecisionOperatorType::kAnd); + request.WithOperatorType(auth::AuthorizeRequest::DecisionOperatorType::kOr); EXPECT_EQ(request.GetOperatorType(), - AuthorizeRequest::DecisionOperatorType::kOr); + auth::AuthorizeRequest::DecisionOperatorType::kOr); request.WithServiceId("service"); EXPECT_EQ(request.CreateKey(), "service"); request.WithContractId("contract"); @@ -53,18 +56,33 @@ TEST(DecisionApiClientTest, AuthorizeRequestTest) { } TEST(DecisionApiClientTest, AuthorizeResponceTest) { - EXPECT_EQ(AuthorizeResult().GetDecision(), DecisionType::kDeny); - EXPECT_EQ(ActionResult().GetDecision(), DecisionType::kDeny); - EXPECT_EQ(AuthorizeResult().GetClientId(), ""); - ActionResult action; - action.SetDecision(DecisionType::kAllow); - action.SetPermissions({{"read", DecisionType::kAllow}}); - AuthorizeResult decision; + EXPECT_EQ(auth::AuthorizeResult().GetDecision(), auth::DecisionType::kDeny); + EXPECT_EQ(auth::ActionResult().GetDecision(), auth::DecisionType::kDeny); + EXPECT_EQ(auth::AuthorizeResult().GetClientId(), ""); + auth::ActionResult action; + action.SetDecision(auth::DecisionType::kAllow); + auth::Permission permission; + permission.SetAction("read"); + permission.SetResource("hrn:test"); + permission.SetDecision(auth::DecisionType::kAllow); + action.SetPermissions({permission}); + auth::AuthorizeResult decision; decision.SetActionResults({action}); - EXPECT_EQ(decision.GetActionResults().front().GetPermissions().front().first, - "read"); - EXPECT_EQ(decision.GetActionResults().front().GetPermissions().front().second, - DecisionType::kAllow); + EXPECT_EQ( + decision.GetActionResults().front().GetPermissions().front().GetAction(), + "read"); + EXPECT_EQ(decision.GetActionResults() + .front() + .GetPermissions() + .front() + .GetDecision(), + auth::DecisionType::kAllow); + EXPECT_EQ(decision.GetActionResults() + .front() + .GetPermissions() + .front() + .GetResource(), + "hrn:test"); } } // namespace diff --git a/tests/functional/olp-cpp-sdk-authentication/DecisionApiTest.cpp b/tests/functional/olp-cpp-sdk-authentication/DecisionApiTest.cpp index 6ea539e70..238b37c2f 100644 --- a/tests/functional/olp-cpp-sdk-authentication/DecisionApiTest.cpp +++ b/tests/functional/olp-cpp-sdk-authentication/DecisionApiTest.cpp @@ -33,7 +33,7 @@ namespace { constexpr auto kLogTag = "AuthenticationClientTestAuthorize"; -using namespace ::olp::authentication; +namespace auth = ::olp::authentication; class AuthenticationClientTestAuthorize : public ::testing::Test { protected: @@ -54,104 +54,111 @@ class AuthenticationClientTestAuthorize : public ::testing::Test { task_scheduler_ = olp::client::OlpClientSettingsFactory::CreateDefaultTaskScheduler(); - AuthenticationSettings settings; + auth::AuthenticationSettings settings; settings.network_request_handler = network_; settings.task_scheduler = task_scheduler_; - client_ = std::make_unique(settings); + client_ = std::make_unique(settings); } void TearDown() override { client_.reset(); network_.reset(); } - AuthenticationClient::SignInClientResponse SignInClient( - const AuthenticationCredentials& credentials, - unsigned int expires_in = kLimitExpiry, bool do_cancel = false) { - std::shared_ptr response; + auth::AuthenticationClient::SignInClientResponse SignInClient( + const auth::AuthenticationCredentials& credentials, + unsigned int expires_in = auth::kLimitExpiry, bool do_cancel = false) { + std::shared_ptr response; unsigned int retry = 0u; do { if (retry > 0u) { OLP_SDK_LOG_WARNING(kLogTag, "Request retry attempted (" << retry << ")"); std::this_thread::sleep_for( - std::chrono::seconds(retry * kRetryDelayInSecs)); + std::chrono::seconds(retry * auth::kRetryDelayInSecs)); } - std::promise request; + std::promise request; auto request_future = request.get_future(); - AuthenticationClient::SignInProperties props; + auth::AuthenticationClient::SignInProperties props; props.expires_in = std::chrono::seconds(expires_in); auto cancel_token = client_->SignInClient( credentials, props, - [&](const AuthenticationClient::SignInClientResponse& resp) { + [&](const auth::AuthenticationClient::SignInClientResponse& resp) { request.set_value(resp); }); if (do_cancel) { cancel_token.Cancel(); } - response = std::make_shared( - request_future.get()); - } while ((!response->IsSuccessful()) && (++retry < kMaxRetryCount) && + response = + std::make_shared( + request_future.get()); + } while ((!response->IsSuccessful()) && (++retry < auth::kMaxRetryCount) && !do_cancel); return *response; } - AuthorizeResponse Authorize(const std::string& access_token, - AuthorizeRequest request, - bool do_cancel = false) { - std::shared_ptr response; + auth::AuthorizeResponse Authorize(const std::string& access_token, + auth::AuthorizeRequest request, + bool do_cancel = false) { + std::shared_ptr response; auto retry = 0u; do { if (retry > 0u) { OLP_SDK_LOG_WARNING(kLogTag, "Request retry attempted (" << retry << ")"); std::this_thread::sleep_for( - std::chrono::seconds(retry * kRetryDelayInSecs)); + std::chrono::seconds(retry * auth::kRetryDelayInSecs)); } - std::promise resp; + std::promise resp; auto request_future = resp.get_future(); - auto cancel_token = client_->Authorize( - access_token, std::move(request), - [&](const AuthorizeResponse& responce) { resp.set_value(responce); }); + auto cancel_token = + client_->Authorize(access_token, std::move(request), + [&](const auth::AuthorizeResponse& responce) { + resp.set_value(responce); + }); if (do_cancel) { cancel_token.Cancel(); } - response = std::make_shared(request_future.get()); - } while ((!response->IsSuccessful()) && (++retry < kMaxRetryCount) && + response = + std::make_shared(request_future.get()); + } while ((!response->IsSuccessful()) && (++retry < auth::kMaxRetryCount) && !do_cancel); return *response; } std::string GetErrorId( - const AuthenticationClient::SignInUserResponse& response) const { + const auth::AuthenticationClient::SignInUserResponse& response) const { return response.GetResult().GetErrorResponse().error_id; } }; TEST_F(AuthenticationClientTestAuthorize, AuthorizeAllow) { - AuthenticationCredentials credentials(id_, secret_); - auto singin_responce = SignInClient(credentials, kExpiryTime); + auth::AuthenticationCredentials credentials(id_, secret_); + auto singin_responce = SignInClient(credentials, auth::kExpiryTime); EXPECT_TRUE(singin_responce.IsSuccessful()); - EXPECT_EQ(olp::http::HttpStatusCode::OK, singin_responce.GetResult().GetStatus()); + EXPECT_EQ(olp::http::HttpStatusCode::OK, + singin_responce.GetResult().GetStatus()); const auto& token = singin_responce.GetResult().GetAccessToken(); - auto request = olp::authentication::AuthorizeRequest().WithServiceId(service_id_); + auto request = + olp::authentication::AuthorizeRequest().WithServiceId(service_id_); request.WithAction("getTileCore"); auto response = Authorize(token, std::move(request)); EXPECT_TRUE(response.IsSuccessful()); EXPECT_FALSE(response.GetResult().GetClientId().empty()); - ASSERT_EQ(response.GetResult().GetDecision(), olp::authentication::DecisionType::kAllow); + ASSERT_EQ(response.GetResult().GetDecision(), + olp::authentication::DecisionType::kAllow); } TEST_F(AuthenticationClientTestAuthorize, AuthorizeDeny) { @@ -159,10 +166,11 @@ TEST_F(AuthenticationClientTestAuthorize, AuthorizeDeny) { olp::authentication::AuthorizeRequest().WithServiceId("Wrong_service"); request.WithAction("getTileCore"); - AuthenticationCredentials credentials(id_, secret_); - auto singin_responce = SignInClient(credentials, kExpiryTime); + auth::AuthenticationCredentials credentials(id_, secret_); + auto singin_responce = SignInClient(credentials, auth::kExpiryTime); EXPECT_TRUE(singin_responce.IsSuccessful()); - EXPECT_EQ(olp::http::HttpStatusCode::OK, singin_responce.GetResult().GetStatus()); + EXPECT_EQ(olp::http::HttpStatusCode::OK, + singin_responce.GetResult().GetStatus()); const auto& token = singin_responce.GetResult().GetAccessToken(); auto response = Authorize(token, std::move(request)); @@ -183,11 +191,12 @@ TEST_F(AuthenticationClientTestAuthorize, AuthorizeWithTwoActions) { olp::authentication::AuthorizeRequest::DecisionOperatorType::kOr) .WithDiagnostics(true); - AuthenticationCredentials credentials(id_, secret_); - auto singin_responce = SignInClient(credentials, kExpiryTime); + auth::AuthenticationCredentials credentials(id_, secret_); + auto singin_responce = SignInClient(credentials, auth::kExpiryTime); EXPECT_TRUE(singin_responce.IsSuccessful()); - EXPECT_EQ(olp::http::HttpStatusCode::OK, singin_responce.GetResult().GetStatus()); + EXPECT_EQ(olp::http::HttpStatusCode::OK, + singin_responce.GetResult().GetStatus()); const auto& token = singin_responce.GetResult().GetAccessToken(); auto response = Authorize(token, std::move(request)); @@ -198,8 +207,8 @@ TEST_F(AuthenticationClientTestAuthorize, AuthorizeWithTwoActions) { olp::authentication::DecisionType::kAllow); auto it = response.GetResult().GetActionResults().begin(); ASSERT_EQ(it->GetDecision(), olp::authentication::DecisionType::kAllow); - ASSERT_EQ(it->GetPermissions().front().first, "getTileCore"); - ASSERT_EQ(it->GetPermissions().front().second, + ASSERT_EQ(it->GetPermissions().front().GetAction(), "getTileCore"); + ASSERT_EQ(it->GetPermissions().front().GetDecision(), olp::authentication::DecisionType::kAllow); ASSERT_EQ((++it)->GetDecision(), olp::authentication::DecisionType::kDeny); EXPECT_TRUE(it->GetPermissions().empty()); diff --git a/tests/integration/olp-cpp-sdk-authentication/AuthenticationClientTest.cpp b/tests/integration/olp-cpp-sdk-authentication/AuthenticationClientTest.cpp index 4a399041e..a95eca46d 100644 --- a/tests/integration/olp-cpp-sdk-authentication/AuthenticationClientTest.cpp +++ b/tests/integration/olp-cpp-sdk-authentication/AuthenticationClientTest.cpp @@ -1405,8 +1405,9 @@ TEST_F(AuthenticationClientTest, Authorize) { auto it = result.GetActionResults().begin(); ASSERT_EQ(it->GetDecision(), olp::authentication::DecisionType::kAllow); - ASSERT_EQ(it->GetPermissions().front().first, "read"); - ASSERT_EQ(it->GetPermissions().front().second, + ASSERT_EQ(it->GetPermissions().front().GetAction(), "read"); + ASSERT_EQ(it->GetPermissions().front().GetResource(), "some_resource"); + ASSERT_EQ(it->GetPermissions().front().GetDecision(), olp::authentication::DecisionType::kAllow); testing::Mock::VerifyAndClearExpectations(network_.get());