Add revocationTimestamp parameter into revocation entry
BUG: 27313743
Change-Id: Ic1d5ebbe771ac3437d9340ca91840d04fa960c57
Reviewed-on: https://weave-review.googlesource.com/2727
Reviewed-by: Alex Vakulenko <avakulenko@google.com>
diff --git a/src/access_api_handler.cc b/src/access_api_handler.cc
index fc7b97b..ec22094 100644
--- a/src/access_api_handler.cc
+++ b/src/access_api_handler.cc
@@ -23,6 +23,7 @@
const char kUserId[] = "userId";
const char kApplicationId[] = "applicationId";
const char kExpirationTime[] = "expirationTime";
+const char kRevocationTimestamp[] = "revocationTimestamp";
const char kBlackList[] = "revocationListEntries";
bool GetIds(const base::DictionaryValue& parameters,
@@ -65,6 +66,9 @@
"applicationId": {
"type": "string"
},
+ "revocationTimestamp": {
+ "type": "integer"
+ },
"expirationTime": {
"type": "integer"
}
@@ -85,6 +89,9 @@
"applicationId": {
"type": "string"
},
+ "revocationTimestamp": {
+ "type": "integer"
+ },
"expirationTime": {
"type": "integer"
}
@@ -132,15 +139,25 @@
return;
}
- int time_j2k = 0;
- if (!parameters.GetInteger(kExpirationTime, &time_j2k)) {
+ int expiration_j2k = 0;
+ if (!parameters.GetInteger(kExpirationTime, &expiration_j2k)) {
Error::AddToPrintf(&error, FROM_HERE, errors::commands::kInvalidPropValue,
"Expiration time is missing");
command->Abort(error.get(), nullptr);
return;
}
- manager_->Block(user_id, app_id, FromJ2000Time(time_j2k),
+ int revocation_j2k = 0;
+ if (!parameters.GetInteger(kRevocationTimestamp, &revocation_j2k)) {
+ Error::AddToPrintf(&error, FROM_HERE, errors::commands::kInvalidPropValue,
+ "Revocation timestamp is missing");
+ command->Abort(error.get(), nullptr);
+ return;
+ }
+
+ manager_->Block(AccessBlackListManager::Entry{user_id, app_id,
+ FromJ2000Time(revocation_j2k),
+ FromJ2000Time(expiration_j2k)},
base::Bind(&AccessApiHandler::OnCommandDone,
weak_ptr_factory_.GetWeakPtr(), cmd));
}
diff --git a/src/access_api_handler.h b/src/access_api_handler.h
index 83f0973..19fe799 100644
--- a/src/access_api_handler.h
+++ b/src/access_api_handler.h
@@ -20,8 +20,7 @@
// Objects of the class subscribe for notification from CommandManager and
// execute incoming commands.
// Handled commands:
-// accessControlBlackList.block
-// accessControlBlackList.unblock
+// accessControlBlackList.revoke
// accessControlBlackList.list
class AccessApiHandler final {
public:
diff --git a/src/access_api_handler_unittest.cc b/src/access_api_handler_unittest.cc
index 79f2d49..0a01d4f 100644
--- a/src/access_api_handler_unittest.cc
+++ b/src/access_api_handler_unittest.cc
@@ -104,6 +104,9 @@
"applicationId": {
"type": "string"
},
+ "revocationTimestamp": {
+ "type": "integer"
+ },
"expirationTime": {
"type": "integer"
}
@@ -124,6 +127,9 @@
"applicationId": {
"type": "string"
},
+ "revocationTimestamp": {
+ "type": "integer"
+ },
"expirationTime": {
"type": "integer"
}
@@ -150,9 +156,14 @@
}
TEST_F(AccessApiHandlerTest, Revoke) {
- EXPECT_CALL(access_manager_, Block(std::vector<uint8_t>{1, 2, 3},
- std::vector<uint8_t>{3, 4, 5}, _, _))
- .WillOnce(WithArgs<3>(
+ EXPECT_CALL(
+ access_manager_,
+ Block(AccessBlackListManager::Entry{std::vector<uint8_t>{1, 2, 3},
+ std::vector<uint8_t>{3, 4, 5},
+ base::Time::FromTimeT(946686034),
+ base::Time::FromTimeT(946692690)},
+ _))
+ .WillOnce(WithArgs<1>(
Invoke([](const DoneCallback& callback) { callback.Run(nullptr); })));
EXPECT_CALL(access_manager_, GetSize()).WillRepeatedly(Return(1));
@@ -162,7 +173,8 @@
'parameters': {
'userId': 'AQID',
'applicationId': 'AwQF',
- 'expirationTime': 1234
+ 'expirationTime': 7890,
+ 'revocationTimestamp': 1234
}
})");
@@ -174,8 +186,14 @@
TEST_F(AccessApiHandlerTest, List) {
std::vector<AccessBlackListManager::Entry> entries{
- {{11, 12, 13}, {21, 22, 23}, base::Time::FromTimeT(1410000000)},
- {{31, 32, 33}, {41, 42, 43}, base::Time::FromTimeT(1420000000)},
+ {{11, 12, 13},
+ {21, 22, 23},
+ base::Time::FromTimeT(1310000000),
+ base::Time::FromTimeT(1410000000)},
+ {{31, 32, 33},
+ {41, 42, 43},
+ base::Time::FromTimeT(1300000000),
+ base::Time::FromTimeT(1420000000)},
};
EXPECT_CALL(access_manager_, GetEntries()).WillOnce(Return(entries));
EXPECT_CALL(access_manager_, GetSize()).WillRepeatedly(Return(4));
diff --git a/src/access_black_list_manager.h b/src/access_black_list_manager.h
index eb3f06a..edbb3f2 100644
--- a/src/access_black_list_manager.h
+++ b/src/access_black_list_manager.h
@@ -14,6 +14,16 @@
class AccessBlackListManager {
public:
struct Entry {
+ Entry() = default;
+
+ Entry(const std::vector<uint8_t>& user,
+ const std::vector<uint8_t>& app,
+ base::Time revocation_ts,
+ base::Time expiration_ts)
+ : user_id{user},
+ app_id{app},
+ revocation{revocation_ts},
+ expiration{expiration_ts} {}
// user_id is empty, app_id is empty: block everything.
// user_id is not empty, app_id is empty: block if user_id matches.
// user_id is empty, app_id is not empty: block if app_id matches.
@@ -21,18 +31,20 @@
std::vector<uint8_t> user_id;
std::vector<uint8_t> app_id;
+ // Revoke matching entries if |revocation| is not less than
+ // delegation timestamp.
+ base::Time revocation;
+
// Time after which to discard the rule.
base::Time expiration;
};
virtual ~AccessBlackListManager() = default;
virtual void AddEntryAddedCallback(const base::Closure& callback) = 0;
- virtual void Block(const std::vector<uint8_t>& user_id,
- const std::vector<uint8_t>& app_id,
- const base::Time& expiration,
- const DoneCallback& callback) = 0;
+ virtual void Block(const Entry& entry, const DoneCallback& callback) = 0;
virtual bool IsBlocked(const std::vector<uint8_t>& user_id,
- const std::vector<uint8_t>& app_id) const = 0;
+ const std::vector<uint8_t>& app_id,
+ base::Time timestamp) const = 0;
virtual std::vector<Entry> GetEntries() const = 0;
virtual size_t GetSize() const = 0;
virtual size_t GetCapacity() const = 0;
@@ -40,8 +52,8 @@
inline bool operator==(const AccessBlackListManager::Entry& l,
const AccessBlackListManager::Entry& r) {
- return l.user_id == r.user_id && l.app_id == r.app_id &&
- l.expiration == r.expiration;
+ return l.revocation == r.revocation && l.expiration == r.expiration &&
+ l.user_id == r.user_id && l.app_id == r.app_id;
}
inline bool operator!=(const AccessBlackListManager::Entry& l,
diff --git a/src/access_black_list_manager_impl.cc b/src/access_black_list_manager_impl.cc
index 3a54073..48c998b 100644
--- a/src/access_black_list_manager_impl.cc
+++ b/src/access_black_list_manager_impl.cc
@@ -10,6 +10,7 @@
#include "src/commands/schema_constants.h"
#include "src/data_encoding.h"
+#include "src/utils.h"
namespace weave {
@@ -19,6 +20,7 @@
const char kUser[] = "user";
const char kApp[] = "app";
const char kExpiration[] = "expiration";
+const char kRevocation[] = "revocation";
}
AccessBlackListManagerImpl::AccessBlackListManagerImpl(
@@ -34,19 +36,22 @@
return;
if (auto list = base::ListValue::From(
base::JSONReader::Read(store_->LoadSettings(kConfigFileName)))) {
- for (const auto& e : *list) {
+ for (const auto& value : *list) {
const base::DictionaryValue* entry{nullptr};
std::string user;
std::string app;
- decltype(entries_)::key_type key;
- int expiration;
- if (e->GetAsDictionary(&entry) && entry->GetString(kUser, &user) &&
- Base64Decode(user, &key.first) && entry->GetString(kApp, &app) &&
- Base64Decode(app, &key.second) &&
+ Entry e;
+ int revocation = 0;
+ int expiration = 0;
+ if (value->GetAsDictionary(&entry) && entry->GetString(kUser, &user) &&
+ Base64Decode(user, &e.user_id) && entry->GetString(kApp, &app) &&
+ Base64Decode(app, &e.app_id) &&
+ entry->GetInteger(kRevocation, &revocation) &&
entry->GetInteger(kExpiration, &expiration)) {
- base::Time expiration_time = base::Time::FromTimeT(expiration);
- if (expiration_time > clock_->Now())
- entries_[key] = expiration_time;
+ e.revocation = FromJ2000Time(revocation);
+ e.expiration = FromJ2000Time(expiration);
+ if (e.expiration > clock_->Now())
+ entries_.insert(e);
}
}
if (entries_.size() < list->GetSize()) {
@@ -66,9 +71,10 @@
base::ListValue list;
for (const auto& e : entries_) {
scoped_ptr<base::DictionaryValue> entry{new base::DictionaryValue};
- entry->SetString(kUser, Base64Encode(e.first.first));
- entry->SetString(kApp, Base64Encode(e.first.second));
- entry->SetInteger(kExpiration, e.second.ToTimeT());
+ entry->SetString(kUser, Base64Encode(e.user_id));
+ entry->SetString(kApp, Base64Encode(e.app_id));
+ entry->SetInteger(kRevocation, ToJ2000Time(e.revocation));
+ entry->SetInteger(kExpiration, ToJ2000Time(e.expiration));
list.Append(std::move(entry));
}
@@ -79,7 +85,7 @@
void AccessBlackListManagerImpl::RemoveExpired() {
for (auto i = begin(entries_); i != end(entries_);) {
- if (i->second <= clock_->Now())
+ if (i->expiration <= clock_->Now())
i = entries_.erase(i);
else
++i;
@@ -91,13 +97,11 @@
on_entry_added_callbacks_.push_back(callback);
}
-void AccessBlackListManagerImpl::Block(const std::vector<uint8_t>& user_id,
- const std::vector<uint8_t>& app_id,
- const base::Time& expiration,
+void AccessBlackListManagerImpl::Block(const Entry& entry,
const DoneCallback& callback) {
// Iterating is OK as Save below is more expensive.
RemoveExpired();
- if (expiration <= clock_->Now()) {
+ if (entry.expiration <= clock_->Now()) {
if (!callback.is_null()) {
ErrorPtr error;
Error::AddTo(&error, FROM_HERE, "aleady_expired",
@@ -116,22 +120,36 @@
return;
}
- auto& value = entries_[std::make_pair(user_id, app_id)];
- value = std::max(value, expiration);
+ auto existing = entries_.find(entry);
+ if (existing != entries_.end()) {
+ Entry new_entry = entry;
+ new_entry.expiration = std::max(entry.expiration, existing->expiration);
+ new_entry.revocation = std::max(entry.revocation, existing->revocation);
+ entries_.erase(existing);
+ entries_.insert(new_entry);
+ } else {
+ entries_.insert(entry);
+ }
+
for (const auto& cb : on_entry_added_callbacks_)
cb.Run();
Save(callback);
}
-bool AccessBlackListManagerImpl::IsBlocked(
- const std::vector<uint8_t>& user_id,
- const std::vector<uint8_t>& app_id) const {
+bool AccessBlackListManagerImpl::IsBlocked(const std::vector<uint8_t>& user_id,
+ const std::vector<uint8_t>& app_id,
+ base::Time timestamp) const {
+ Entry entry_to_find;
for (const auto& user : {{}, user_id}) {
for (const auto& app : {{}, app_id}) {
- auto both = entries_.find(std::make_pair(user, app));
- if (both != end(entries_) && both->second > clock_->Now())
+ entry_to_find.user_id = user;
+ entry_to_find.app_id = app;
+ auto match = entries_.find(entry_to_find);
+ if (match != end(entries_) && match->expiration > clock_->Now() &&
+ match->revocation >= timestamp) {
return true;
+ }
}
}
return false;
@@ -139,10 +157,7 @@
std::vector<AccessBlackListManager::Entry>
AccessBlackListManagerImpl::GetEntries() const {
- std::vector<Entry> result;
- for (const auto& e : entries_)
- result.push_back({e.first.first, e.first.second, e.second});
- return result;
+ return {begin(entries_), end(entries_)};
}
size_t AccessBlackListManagerImpl::GetSize() const {
diff --git a/src/access_black_list_manager_impl.h b/src/access_black_list_manager_impl.h
index e41a93a..ff91a8f 100644
--- a/src/access_black_list_manager_impl.h
+++ b/src/access_black_list_manager_impl.h
@@ -5,7 +5,7 @@
#ifndef LIBWEAVE_SRC_ACCESS_BLACK_LIST_IMPL_H_
#define LIBWEAVE_SRC_ACCESS_BLACK_LIST_IMPL_H_
-#include <map>
+#include <set>
#include <utility>
#include <base/time/default_clock.h>
@@ -25,12 +25,10 @@
// AccessBlackListManager implementation.
void AddEntryAddedCallback(const base::Closure& callback) override;
- void Block(const std::vector<uint8_t>& user_id,
- const std::vector<uint8_t>& app_id,
- const base::Time& expiration,
- const DoneCallback& callback) override;
+ void Block(const Entry& entry, const DoneCallback& callback) override;
bool IsBlocked(const std::vector<uint8_t>& user_id,
- const std::vector<uint8_t>& app_id) const override;
+ const std::vector<uint8_t>& app_id,
+ base::Time timestamp) const override;
std::vector<Entry> GetEntries() const override;
size_t GetSize() const override;
size_t GetCapacity() const override;
@@ -40,13 +38,22 @@
void Save(const DoneCallback& callback);
void RemoveExpired();
+ struct EntryIdsLess {
+ bool operator()(const Entry& l, const Entry& r) const {
+ if (l.user_id < r.user_id)
+ return true;
+ if (l.user_id > r.user_id)
+ return false;
+ return l.app_id < r.app_id;
+ }
+ };
+
const size_t capacity_{0};
base::DefaultClock default_clock_;
base::Clock* clock_{&default_clock_};
provider::ConfigStore* store_{nullptr};
- std::map<std::pair<std::vector<uint8_t>, std::vector<uint8_t>>, base::Time>
- entries_;
+ std::set<Entry, EntryIdsLess> entries_;
std::vector<base::Closure> on_entry_added_callbacks_;
DISALLOW_COPY_AND_ASSIGN(AccessBlackListManagerImpl);
diff --git a/src/access_black_list_manager_impl_unittest.cc b/src/access_black_list_manager_impl_unittest.cc
index 0f21c3e..3be9287 100644
--- a/src/access_black_list_manager_impl_unittest.cc
+++ b/src/access_black_list_manager_impl_unittest.cc
@@ -24,11 +24,13 @@
std::string to_load = R"([{
"user": "BQID",
"app": "BwQF",
- "expiration": 1410000000
+ "expiration": 463315200,
+ "revocation": 463314200
}, {
"user": "AQID",
"app": "AwQF",
- "expiration": 1419999999
+ "expiration": 473315199,
+ "revocation": 473313199
}])";
EXPECT_CALL(config_store_, LoadSettings("black_list"))
@@ -40,7 +42,8 @@
std::string to_save = R"([{
"user": "AQID",
"app": "AwQF",
- "expiration": 1419999999
+ "expiration": 473315199,
+ "revocation": 473313199
}])";
EXPECT_JSON_EQ(to_save, *test::CreateValue(json));
if (!callback.is_null())
@@ -60,7 +63,10 @@
EXPECT_EQ(1u, manager_->GetSize());
EXPECT_EQ(10u, manager_->GetCapacity());
EXPECT_EQ((std::vector<AccessBlackListManagerImpl::Entry>{{
- {1, 2, 3}, {3, 4, 5}, base::Time::FromTimeT(1419999999),
+ {1, 2, 3},
+ {3, 4, 5},
+ base::Time::FromTimeT(1419997999),
+ base::Time::FromTimeT(1419999999),
}}),
manager_->GetEntries());
}
@@ -75,22 +81,31 @@
std::string to_save = R"([{
"user": "AQID",
"app": "AwQF",
- "expiration": 1419999999
+ "expiration": 473315199,
+ "revocation": 473313199
}, {
"app": "CAgI",
"user": "BwcH",
- "expiration": 1419990000
+ "expiration": 473305200,
+ "revocation": 473295200
}])";
EXPECT_JSON_EQ(to_save, *test::CreateValue(json));
if (!callback.is_null())
callback.Run(nullptr);
})));
- manager_->Block({7, 7, 7}, {8, 8, 8}, base::Time::FromTimeT(1419990000), {});
+ manager_->Block({{7, 7, 7},
+ {8, 8, 8},
+ base::Time::FromTimeT(1419980000),
+ base::Time::FromTimeT(1419990000)},
+ {});
EXPECT_TRUE(callback_called);
}
TEST_F(AccessBlackListManagerImplTest, BlockExpired) {
- manager_->Block({}, {}, base::Time::FromTimeT(1400000000),
+ manager_->Block({{},
+ {},
+ base::Time::FromTimeT(1300000000),
+ base::Time::FromTimeT(1400000000)},
base::Bind([](ErrorPtr error) {
EXPECT_TRUE(error->HasError("aleady_expired"));
}));
@@ -105,18 +120,30 @@
})));
for (size_t i = manager_->GetSize(); i < manager_->GetCapacity(); ++i) {
manager_->Block(
- {99, static_cast<uint8_t>(i / 256), static_cast<uint8_t>(i % 256)},
- {8, 8, 8}, base::Time::FromTimeT(1419990000), {});
+ {{99, static_cast<uint8_t>(i / 256), static_cast<uint8_t>(i % 256)},
+ {8, 8, 8},
+ base::Time::FromTimeT(1419970000),
+ base::Time::FromTimeT(1419990000)},
+ {});
EXPECT_EQ(i + 1, manager_->GetSize());
}
- manager_->Block({99}, {8, 8, 8}, base::Time::FromTimeT(1419990000),
+ manager_->Block({{99},
+ {8, 8, 8},
+ base::Time::FromTimeT(1419970000),
+ base::Time::FromTimeT(1419990000)},
base::Bind([](ErrorPtr error) {
EXPECT_TRUE(error->HasError("blacklist_is_full"));
}));
}
-TEST_F(AccessBlackListManagerImplTest, IsBlockedFalse) {
- EXPECT_FALSE(manager_->IsBlocked({7, 7, 7}, {8, 8, 8}));
+TEST_F(AccessBlackListManagerImplTest, IsBlockedIdsNotMacth) {
+ EXPECT_FALSE(manager_->IsBlocked({7, 7, 7}, {8, 8, 8}, {}));
+}
+
+TEST_F(AccessBlackListManagerImplTest, IsBlockedRevocationIsOld) {
+ // Ids match but delegation time is newer than revocation time.
+ EXPECT_FALSE(manager_->IsBlocked({1, 2, 3}, {3, 4, 5},
+ base::Time::FromTimeT(1429997999)));
}
class AccessBlackListManagerImplIsBlockedTest
@@ -132,13 +159,16 @@
if (!callback.is_null())
callback.Run(nullptr);
})));
- manager_->Block(std::get<0>(GetParam()), std::get<1>(GetParam()),
- base::Time::FromTimeT(1419990000), {});
+ manager_->Block({std::get<0>(GetParam()),
+ std::get<1>(GetParam()),
+ {},
+ base::Time::FromTimeT(1419990000)},
+ {});
}
};
TEST_P(AccessBlackListManagerImplIsBlockedTest, IsBlocked) {
- EXPECT_TRUE(manager_->IsBlocked({7, 7, 7}, {8, 8, 8}));
+ EXPECT_TRUE(manager_->IsBlocked({7, 7, 7}, {8, 8, 8}, {}));
}
INSTANTIATE_TEST_CASE_P(
diff --git a/src/test/mock_black_list_manager.h b/src/test/mock_black_list_manager.h
index 6b55b86..2f6d7eb 100644
--- a/src/test/mock_black_list_manager.h
+++ b/src/test/mock_black_list_manager.h
@@ -16,14 +16,11 @@
class MockAccessBlackListManager : public AccessBlackListManager {
public:
MOCK_METHOD1(AddEntryAddedCallback, void(const base::Closure&));
- MOCK_METHOD4(Block,
- void(const std::vector<uint8_t>&,
- const std::vector<uint8_t>&,
- const base::Time&,
- const DoneCallback&));
- MOCK_CONST_METHOD2(IsBlocked,
+ MOCK_METHOD2(Block, void(const Entry&, const DoneCallback&));
+ MOCK_CONST_METHOD3(IsBlocked,
bool(const std::vector<uint8_t>&,
- const std::vector<uint8_t>&));
+ const std::vector<uint8_t>&,
+ base::Time));
MOCK_CONST_METHOD0(GetEntries, std::vector<Entry>());
MOCK_CONST_METHOD0(GetSize, size_t());
MOCK_CONST_METHOD0(GetCapacity, size_t());
