From f83d3a04538febc146c16eb04cddca395ffdd684 Mon Sep 17 00:00:00 2001 From: Uditha Atukorala Date: Thu, 13 Jul 2023 22:09:25 +0100 Subject: [PATCH 1/3] (datastore) drop lookup identities in favour of retrieve identities by sub --- src/datastore/identities.cpp | 20 +++++------ src/datastore/identities.h | 4 +-- src/datastore/identities_test.cpp | 55 ++++++++++++++++++++----------- 3 files changed, 47 insertions(+), 32 deletions(-) diff --git a/src/datastore/identities.cpp b/src/datastore/identities.cpp index c0d6ed3f..5842dfa3 100644 --- a/src/datastore/identities.cpp +++ b/src/datastore/identities.cpp @@ -81,7 +81,7 @@ void Identity::store() const { _rev = res.at(0, 0).as(); } -Identities LookupIdentities(const std::string &sub) { +Identity RetrieveIdentity(const std::string &id) { std::string_view qry = R"( select _id, @@ -90,20 +90,18 @@ Identities LookupIdentities(const std::string &sub) { attrs from identities where - sub = $1::text; + _id = $1::text; )"; - auto res = pg::exec(qry, sub); - - Identities identities; - for (const auto &row : res) { - identities.push_back(Identity(row)); + auto res = pg::exec(qry, id); + if (res.empty()) { + throw err::DatastoreIdentityNotFound(); } - return identities; + return Identity(res[0]); } -Identity RetrieveIdentity(const std::string &id) { +Identity RetrieveIdentityBySub(const std::string &sub) { std::string_view qry = R"( select _id, @@ -112,10 +110,10 @@ Identity RetrieveIdentity(const std::string &id) { attrs from identities where - _id = $1::text; + sub = $1::text; )"; - auto res = pg::exec(qry, id); + auto res = pg::exec(qry, sub); if (res.empty()) { throw err::DatastoreIdentityNotFound(); } diff --git a/src/datastore/identities.h b/src/datastore/identities.h index 532c5f0c..840829c0 100644 --- a/src/datastore/identities.h +++ b/src/datastore/identities.h @@ -46,6 +46,6 @@ class Identity { using Identities = std::vector; -Identities LookupIdentities(const std::string &sub); -Identity RetrieveIdentity(const std::string &id); +Identity RetrieveIdentity(const std::string &id); +Identity RetrieveIdentityBySub(const std::string &sub); } // namespace datastore diff --git a/src/datastore/identities_test.cpp b/src/datastore/identities_test.cpp index cb476e4e..23e7948a 100644 --- a/src/datastore/identities_test.cpp +++ b/src/datastore/identities_test.cpp @@ -45,25 +45,6 @@ TEST_F(IdentitiesTest, discard) { EXPECT_EQ(0, count); } -TEST_F(IdentitiesTest, lookup) { - // Success: - { - const datastore::Identity identity({ - .sub = "sub:IdentitiesTest.lookup", - }); - - // expect no entries before adding to collection - auto identities = datastore::LookupIdentities(identity.sub()); - EXPECT_EQ(0, identities.size()); - - ASSERT_NO_THROW(identity.store()); - - // expect single entry - identities = datastore::LookupIdentities(identity.sub()); - EXPECT_EQ(1, identities.size()); - } -} - TEST_F(IdentitiesTest, retrieve) { // Success: retrieve data { @@ -124,8 +105,44 @@ TEST_F(IdentitiesTest, retrieve) { EXPECT_EQ(R"({"foo": "bar"})", identity.attrs()); } + // Success: retrieve by sub + { + std::string_view qry = R"( + insert into identities ( + _id, + _rev, + sub, + attrs + ) values ( + $1::text, + $2::integer, + $3::text, + $4::jsonb + ); + )"; + + try { + datastore::pg::exec( + qry, + "_id:IdentitiesTest.retrieve-sub", + 2205, + "sub:IdentitiesTest.retrieve-sub", + R"({"key": "value"})"); + } catch (const std::exception &e) { + FAIL() << e.what(); + } + + auto identity = datastore::RetrieveIdentityBySub("sub:IdentitiesTest.retrieve-sub"); + EXPECT_EQ("_id:IdentitiesTest.retrieve-sub", identity.id()); + EXPECT_EQ(2205, identity.rev()); + EXPECT_EQ(R"({"key": "value"})", identity.attrs()); + } + // Error: not found { EXPECT_THROW(datastore::RetrieveIdentity("_id:dummy"), err::DatastoreIdentityNotFound); } + + // Error: not found (by sub) + { EXPECT_THROW(datastore::RetrieveIdentityBySub("sub:dummy"), err::DatastoreIdentityNotFound); } } TEST_F(IdentitiesTest, rev) { From a08de6dfbc1be95fe2e5e8701537a3147290a827 Mon Sep 17 00:00:00 2001 From: Uditha Atukorala Date: Thu, 13 Jul 2023 22:27:01 +0100 Subject: [PATCH 2/3] (service) drop lookup identities in favour of retrieving by sub --- proto/gk/v1/gatekeeper.proto | 20 +++------------ src/service/grpc.cpp | 21 ++++++---------- src/service/grpc.h | 4 --- src/service/grpc_test.cpp | 49 ++++++++++++++++++------------------ src/service/mappers.cpp | 7 ------ src/service/mappers.h | 1 - 6 files changed, 35 insertions(+), 67 deletions(-) diff --git a/proto/gk/v1/gatekeeper.proto b/proto/gk/v1/gatekeeper.proto index 439411ad..5f7e947c 100644 --- a/proto/gk/v1/gatekeeper.proto +++ b/proto/gk/v1/gatekeeper.proto @@ -88,12 +88,6 @@ service Gatekeeper { }; } - rpc LookupIdentities(LookupIdentitiesRequest) returns (LookupIdentitiesResponse) { - option (google.api.http) = { - get : "/v1/identities" - }; - } - rpc RetrieveIdentity(RetrieveIdentityRequest) returns (Identity) { option (google.api.http) = { get : "/v1/identities/{id}" @@ -283,17 +277,11 @@ message CreateIdentityRequest { optional google.protobuf.Struct attrs = 3; } -message LookupIdentitiesRequest { - optional string sub = 1; -} - -message LookupIdentitiesResponse { - repeated Identity data = 1; - Meta meta = 2; -} - message RetrieveIdentityRequest { - string id = 1; + oneof identity { + string id = 1; + string sub = 2; + } } message UpdateIdentityRequest { diff --git a/src/service/grpc.cpp b/src/service/grpc.cpp index c107fe62..25500b43 100644 --- a/src/service/grpc.cpp +++ b/src/service/grpc.cpp @@ -346,8 +346,13 @@ grpc::ServerUnaryReactor *Grpc::RetrieveIdentity( auto *reactor = context->DefaultReactor(); try { - auto identity = datastore::RetrieveIdentity(request->id()); - map(identity, response); + if (request->has_sub()) { + auto identity = datastore::RetrieveIdentityBySub(request->sub()); + map(identity, response); + } else { + auto identity = datastore::RetrieveIdentity(request->id()); + map(identity, response); + } } catch (const err::DatastoreIdentityNotFound &) { reactor->Finish(grpc::Status(grpc::StatusCode::NOT_FOUND, "Document not found")); return reactor; @@ -360,18 +365,6 @@ grpc::ServerUnaryReactor *Grpc::RetrieveIdentity( return reactor; } -grpc::ServerUnaryReactor *Grpc::LookupIdentities( - grpc::CallbackServerContext *context, const gk::v1::LookupIdentitiesRequest *request, - gk::v1::LookupIdentitiesResponse *response) { - auto *reactor = context->DefaultReactor(); - - const auto identities = datastore::LookupIdentities(request->sub()); - - map(identities, response); - reactor->Finish(grpc::Status::OK); - return reactor; -} - grpc::ServerUnaryReactor *Grpc::UpdateIdentity( grpc::CallbackServerContext *context, const gk::v1::UpdateIdentityRequest *request, gk::v1::Identity *response) { diff --git a/src/service/grpc.h b/src/service/grpc.h index 57838db1..6b2079e8 100644 --- a/src/service/grpc.h +++ b/src/service/grpc.h @@ -58,10 +58,6 @@ class Grpc final : public gk::v1::Gatekeeper::CallbackService { grpc::CallbackServerContext *context, const gk::v1::RetrieveIdentityRequest *request, gk::v1::Identity *response) override; - grpc::ServerUnaryReactor *LookupIdentities( - grpc::CallbackServerContext *context, const gk::v1::LookupIdentitiesRequest *request, - gk::v1::LookupIdentitiesResponse *response) override; - grpc::ServerUnaryReactor *UpdateIdentity( grpc::CallbackServerContext *context, const gk::v1::UpdateIdentityRequest *request, gk::v1::Identity *response) override; diff --git a/src/service/grpc_test.cpp b/src/service/grpc_test.cpp index 900c47ed..e26d0ef1 100644 --- a/src/service/grpc_test.cpp +++ b/src/service/grpc_test.cpp @@ -854,6 +854,30 @@ TEST_F(GrpcTest, RetrieveIdentity) { EXPECT_FALSE(response.has_attrs()); } + // Success: retrieve identity by sub + { + const datastore::Identity identity({ + .id = "id:GrpcTest.RetrieveIdentity-by_sub", + .sub = "sub:GrpcTest.RetrieveIdentity-by_sub", + }); + ASSERT_NO_THROW(identity.store()); + + grpc::CallbackServerContext ctx; + grpc::testing::DefaultReactorTestPeer peer(&ctx); + gk::v1::Identity response; + + gk::v1::RetrieveIdentityRequest request; + request.set_sub(identity.sub()); + + auto reactor = service.RetrieveIdentity(&ctx, &request, &response); + EXPECT_TRUE(peer.test_status_set()); + EXPECT_TRUE(peer.test_status().ok()); + EXPECT_EQ(peer.reactor(), reactor); + EXPECT_EQ(identity.id(), response.id()); + EXPECT_EQ(identity.sub(), response.sub()); + EXPECT_FALSE(response.has_attrs()); + } + // Success: retrieve identity by id with attrs { const datastore::Identity identity({ @@ -898,31 +922,6 @@ TEST_F(GrpcTest, RetrieveIdentity) { } } -TEST_F(GrpcTest, LookupIdentities) { - service::Grpc service; - - // Success: lookup identities by sub - { - const datastore::Identity identity( - {.id = "id:GrpcTest.LookupIdentities", .sub = "sub:GrpcTest.LookupIdentities"}); - EXPECT_NO_THROW(identity.store()); - - grpc::CallbackServerContext ctx; - grpc::testing::DefaultReactorTestPeer peer(&ctx); - gk::v1::LookupIdentitiesResponse response; - - gk::v1::LookupIdentitiesRequest request; - request.set_sub(identity.sub()); - - auto reactor = service.LookupIdentities(&ctx, &request, &response); - EXPECT_TRUE(peer.test_status_set()); - EXPECT_TRUE(peer.test_status().ok()); - EXPECT_EQ(peer.reactor(), reactor); - EXPECT_EQ(response.data().size(), 1); - EXPECT_EQ(response.data()[0].id(), identity.id()); - } -} - TEST_F(GrpcTest, UpdateIndentity) { service::Grpc service; diff --git a/src/service/mappers.cpp b/src/service/mappers.cpp index af53fd66..99232a8b 100644 --- a/src/service/mappers.cpp +++ b/src/service/mappers.cpp @@ -105,13 +105,6 @@ void map(const datastore::Identity &from, gk::v1::Identity *to) { } } -void map(const datastore::Identities &from, gk::v1::LookupIdentitiesResponse *to) { - for (const auto &identity : from) { - auto i = to->add_data(); - map(identity, i); - } -} - void map(const datastore::Policies &from, gk::v1::CheckAccessResponse *to) { for (const auto &policy : from) { auto p = to->add_policies(); diff --git a/src/service/mappers.h b/src/service/mappers.h index 458b4204..9cb52b63 100644 --- a/src/service/mappers.h +++ b/src/service/mappers.h @@ -18,7 +18,6 @@ void map(const datastore::AccessPolicy &from, gk::v1::AccessPolicy *to); void map(const datastore::AccessPolicy &from, gk::v1::Policy *to); void map(const datastore::Collection &from, gk::v1::Collection *to); void map(const datastore::Identity &from, gk::v1::Identity *to); -void map(const datastore::Identities &from, gk::v1::LookupIdentitiesResponse *to); void map(const datastore::Policies &from, gk::v1::CheckAccessResponse *to); void map(const datastore::Policies &from, gk::v1::CheckRbacResponse *to); void map(const datastore::RbacPolicy &from, gk::v1::RbacPolicy *to); From 3e3adaa1505ed5b4bfad8a434530141de7c2cc42 Mon Sep 17 00:00:00 2001 From: Uditha Atukorala Date: Thu, 13 Jul 2023 22:31:02 +0100 Subject: [PATCH 3/3] (service) tidy-up tests --- src/service/grpc_test.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/service/grpc_test.cpp b/src/service/grpc_test.cpp index e26d0ef1..c6df5002 100644 --- a/src/service/grpc_test.cpp +++ b/src/service/grpc_test.cpp @@ -834,9 +834,11 @@ TEST_F(GrpcTest, RetrieveIdentity) { // Success: retrieve identity by id { - const datastore::Identity identity( - {.id = "id:GrpcTest.RetrieveIdentity", .sub = "sub:GrpcTest.RetrieveIdentity"}); - EXPECT_NO_THROW(identity.store()); + const datastore::Identity identity({ + .id = "id:GrpcTest.RetrieveIdentity", + .sub = "sub:GrpcTest.RetrieveIdentity", + }); + ASSERT_NO_THROW(identity.store()); grpc::CallbackServerContext ctx; grpc::testing::DefaultReactorTestPeer peer(&ctx); @@ -913,7 +915,7 @@ TEST_F(GrpcTest, RetrieveIdentity) { gk::v1::Identity response; gk::v1::RetrieveIdentityRequest request; - request.set_id("id:GrpcTest.RetrieveIdentity-not-found"); + request.set_id("id:GrpcTest.RetrieveIdentity-not_found"); auto reactor = service.RetrieveIdentity(&ctx, &request, &response); EXPECT_TRUE(peer.test_status_set());