Skip to content

Commit

Permalink
Addressed codereview suggestions:
Browse files Browse the repository at this point in the history
- updated comments
- removed forgotten include
- used base::StrCat
- redone TimeLimitedWords::Validate => TimeLimitedWords::Parse to avoid using of output string* parameter
- fixed ret.pure_words initialization
- removed forgotten log
- from js and html changes combined variables isInvalidSyncCode and syncCodeValidationResult into single syncCodeValidationError

Adjusted words epoch and v1 sunset dates
- epoch to Tue, 10 May 2022
- sunset to Fri, 1 Jul
  • Loading branch information
AlexeyBarabash committed May 13, 2022
1 parent b37c70d commit bf67782
Show file tree
Hide file tree
Showing 9 changed files with 156 additions and 126 deletions.
20 changes: 12 additions & 8 deletions browser/android/brave_sync_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,11 @@ int JNI_BraveSyncWorker_GetWordsValidationResult(
std::string str_time_limited_words =
base::android::ConvertJavaStringToUTF8(time_limited_words);
DCHECK(!str_time_limited_words.empty());
std::string pure_words_stub;
return static_cast<int>(brave_sync::TimeLimitedWords::Validate(
str_time_limited_words, &pure_words_stub));

auto pure_words_with_status =
brave_sync::TimeLimitedWords::Parse(str_time_limited_words);

return static_cast<int>(pure_words_with_status.status);
}

static base::android::ScopedJavaLocalRef<jstring>
Expand All @@ -371,12 +373,14 @@ JNI_BraveSyncWorker_GetPureWordsFromTimeLimited(
base::android::ConvertJavaStringToUTF8(time_limited_words);
DCHECK(!str_time_limited_words.empty());

std::string pure_words;
auto validation_result = brave_sync::TimeLimitedWords::Validate(
str_time_limited_words, &pure_words);
DCHECK_EQ(validation_result, brave_sync::WordsValidationResult::kValid);
auto pure_words_with_status =
brave_sync::TimeLimitedWords::Parse(str_time_limited_words);
DCHECK_EQ(pure_words_with_status.status,
brave_sync::WordsValidationStatus::kValid);
DCHECK(pure_words_with_status.pure_words.has_value());

return base::android::ConvertUTF8ToJavaString(env, pure_words);
return base::android::ConvertUTF8ToJavaString(
env, pure_words_with_status.pure_words.value());
}

static base::android::ScopedJavaLocalRef<jstring>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,9 @@ <h2 class="device-title">$i18n{braveSyncChooseDeviceComputerTitle}</h2>
<div class="sync-code-footer">
[[i18n('braveSyncWordCount', syncCodeWordCount_)]]
</div>
<template is="dom-if" if="[[isInvalidSyncCode]]">
<template is="dom-if" if="[[syncCodeValidationError]]">
<div class="invalid-sync-code">
[[syncCodeValidationResult]]
[[syncCodeValidationError]]
</div>
</template>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,7 @@ Polymer({
value: 'choose',
notify: true
},
isInvalidSyncCode: {
type: Boolean,
value: false,
notify: true
},
syncCodeValidationResult: {
syncCodeValidationError: {
type: String,
value: '',
notify: true
Expand All @@ -69,7 +64,6 @@ Polymer({
},

observers: [
'updateSyncCodeValidity_(syncCode)',
'getQRCode_(syncCode, codeType)',
],

Expand All @@ -80,10 +74,6 @@ Polymer({
this.syncBrowserProxy_ = BraveSyncBrowserProxy.getInstance();
},

updateSyncCodeValidity_: function() {
this.isInvalidSyncCode = false
},

computeSyncCodeWordCount_: function() {
if (!this.syncCode) {
return 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ <h1>$i18n{braveSyncSetupTitle}</h1>
<settings-brave-sync-code-dialog
code-type="{{syncCodeDialogType_}}"
sync-code="{{syncCode}}"
is-invalid-sync-code="{{isInvalidSyncCode_}}"
sync-code-validation-result="{{syncCodeValidationResult_}}"
sync-code-validation-error="{{syncCodeValidationError_}}"
on-done="handleSyncCodeDialogDone_"
></settings-brave-sync-code-dialog>
</template>
13 changes: 3 additions & 10 deletions browser/resources/settings/brave_sync_page/brave_sync_setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,7 @@ Polymer({
type: Boolean,
value: false,
},
isInvalidSyncCode_: {
type: Boolean,
value: false,
},
syncCodeValidationResult_: {
syncCodeValidationError_: {
type: String,
value: '',
}
Expand Down Expand Up @@ -86,20 +82,17 @@ Polymer({
},

submitSyncCode_: async function () {
console.log('submitSyncCode_ 000')
this.isSubmittingSyncCode_ = true
const syncCodeToSubmit = this.syncCode || ''
let success = false
try {
success = await this.syncBrowserProxy_.setSyncCode(syncCodeToSubmit)
} catch (e) {
this.syncCodeValidationResult_ = e
this.syncCodeValidationError_ = e
success = false
}
this.isSubmittingSyncCode_ = false
if (!success) {
this.isInvalidSyncCode_ = true
} else {
if (success) {
this.syncCodeDialogType_ = undefined
this.fire('setup-success')
}
Expand Down
53 changes: 27 additions & 26 deletions browser/ui/webui/settings/brave_sync_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,24 @@
#include "ui/base/webui/web_ui_util.h"

using brave_sync::TimeLimitedWords;
using brave_sync::WordsValidationResult;
using brave_sync::WordsValidationStatus;

namespace {

std::string GetSyncCodeValidationString(
WordsValidationResult validation_result) {
WordsValidationStatus validation_result) {
switch (validation_result) {
case WordsValidationResult::kValid:
case WordsValidationStatus::kValid:
return "";
case WordsValidationResult::kWrongWordsNumber:
case WordsValidationResult::kNotValidPureWords:
case WordsValidationStatus::kWrongWordsNumber:
case WordsValidationStatus::kNotValidPureWords:
return l10n_util::GetStringUTF8(IDS_BRAVE_SYNC_CODE_INVALID);
case WordsValidationResult::kVersionDeprecated:
case WordsValidationStatus::kVersionDeprecated:
return l10n_util::GetStringUTF8(
IDS_BRAVE_SYNC_CODE_FROM_DEPRECATED_VERSION);
case WordsValidationResult::kExpired:
case WordsValidationStatus::kExpired:
return l10n_util::GetStringUTF8(IDS_BRAVE_SYNC_CODE_EXPIRED);
case WordsValidationResult::kValidForTooLong:
case WordsValidationStatus::kValidForTooLong:
return l10n_util::GetStringUTF8(IDS_BRAVE_SYNC_CODE_VALID_FOR_TOO_LONG);
default:
NOTREACHED();
Expand Down Expand Up @@ -152,15 +152,15 @@ void BraveSyncHandler::HandleGetQRCode(const base::Value::List& args) {
const std::string time_limited_sync_code = args[1].GetString();

// Sync code arrives here with time-limit 25th word, remove it to get proper
// pure seed for QR generation
std::string pure_sync_code;
auto validation_result =
TimeLimitedWords::Validate(time_limited_sync_code, &pure_sync_code);
CHECK_EQ(validation_result, WordsValidationResult::kValid);
CHECK_NE(pure_sync_code.size(), 0u);
// pure seed for QR generation (QR codes have their own expiry)
auto pure_words_with_status = TimeLimitedWords::Parse(time_limited_sync_code);
CHECK_EQ(pure_words_with_status.status, WordsValidationStatus::kValid);
CHECK(pure_words_with_status.pure_words);
CHECK_NE(pure_words_with_status.pure_words.value().size(), 0u);

std::vector<uint8_t> seed;
if (!brave_sync::crypto::PassphraseToBytes32(pure_sync_code, &seed)) {
if (!brave_sync::crypto::PassphraseToBytes32(
pure_words_with_status.pure_words.value(), &seed)) {
LOG(ERROR) << "invalid sync code when generating qr code";
RejectJavascriptCallback(args[0].Clone(), base::Value("invalid sync code"));
return;
Expand Down Expand Up @@ -211,23 +211,24 @@ void BraveSyncHandler::HandleSetSyncCode(const base::Value::List& args) {
return;
}

std::string pure_sync_code;
WordsValidationResult validation_result =
TimeLimitedWords::Validate(time_limited_sync_code, &pure_sync_code);
if (validation_result != WordsValidationResult::kValid) {
auto pure_words_with_status = TimeLimitedWords::Parse(time_limited_sync_code);

if (pure_words_with_status.status != WordsValidationStatus::kValid) {
LOG(ERROR) << "Could not validate a sync code, validation_result="
<< static_cast<int>(validation_result) << " "
<< GetSyncCodeValidationString(validation_result);
RejectJavascriptCallback(
args[0].Clone(),
base::Value(GetSyncCodeValidationString(validation_result)));
<< static_cast<int>(pure_words_with_status.status) << " "
<< GetSyncCodeValidationString(pure_words_with_status.status);
RejectJavascriptCallback(args[0].Clone(),
base::Value(GetSyncCodeValidationString(
pure_words_with_status.status)));
return;
}

CHECK(!pure_sync_code.empty());
CHECK(pure_words_with_status.pure_words);
CHECK(!pure_words_with_status.pure_words.value().empty());

auto* sync_service = GetSyncService();
if (!sync_service || !sync_service->SetSyncCode(pure_sync_code)) {
if (!sync_service ||
!sync_service->SetSyncCode(pure_words_with_status.pure_words.value())) {
RejectJavascriptCallback(args[0].Clone(), base::Value(false));
return;
}
Expand Down
55 changes: 40 additions & 15 deletions components/brave_sync/time_limited_words.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "base/containers/span.h"
#include "base/logging.h"
#include "base/notreached.h"
#include "base/strings/strcat.h"
#include "base/strings/string_split.h"
#include "base/strings/string_util.h"
#include "base/time/time.h"
Expand All @@ -23,13 +24,22 @@ namespace brave_sync {

namespace {

// TODO(alexeybarabash): subject to change
static constexpr char kWordsv1SunsetDate[] = "Wed, 1 Jun 2022 00:00:00 GMT";

static constexpr char kWordsv2Epoch[] = "Fri, 15 Apr 2022 00:00:00 GMT";
static constexpr char kWordsv1SunsetDate[] = "Fri, 1 Jul 2022 00:00:00 GMT";
static constexpr char kWordsv2Epoch[] = "Tue, 10 May 2022 00:00:00 GMT";

} // namespace

TimeLimitedWords::PureWordsWithStatus::PureWordsWithStatus() = default;

TimeLimitedWords::PureWordsWithStatus::PureWordsWithStatus(
PureWordsWithStatus&& other) = default;

TimeLimitedWords::PureWordsWithStatus::~PureWordsWithStatus() = default;

TimeLimitedWords::PureWordsWithStatus&
TimeLimitedWords::PureWordsWithStatus::operator=(PureWordsWithStatus&& other) =
default;

using base::Time;
using base::TimeDelta;

Expand Down Expand Up @@ -117,11 +127,11 @@ std::string TimeLimitedWords::GenerateForDate(const std::string& pure_words,

std::string last_word = GetWordByIndex(days_since_words_v2_epoch);

std::string time_limited_code = pure_words + " " + last_word;
std::string time_limited_code = base::StrCat({pure_words, " ", last_word});
return time_limited_code;
}

WordsValidationResult TimeLimitedWords::Validate(
WordsValidationStatus TimeLimitedWords::Validate(
const std::string& time_limited_words,
std::string* pure_words) {
CHECK_NE(pure_words, nullptr);
Expand All @@ -144,12 +154,12 @@ WordsValidationResult TimeLimitedWords::Validate(
base::span<std::string>(words.begin(), kPureWordsCount), " ");
if (crypto::IsPassphraseValid(recombined_pure_words)) {
*pure_words = recombined_pure_words;
return WordsValidationResult::kValid;
return WordsValidationStatus::kValid;
} else {
return WordsValidationResult::kNotValidPureWords;
return WordsValidationStatus::kNotValidPureWords;
}
} else {
return WordsValidationResult::kVersionDeprecated;
return WordsValidationStatus::kVersionDeprecated;
}
} else if (num_words == kWordsV2Count) {
std::string recombined_pure_words = base::JoinString(
Expand All @@ -164,21 +174,36 @@ WordsValidationResult TimeLimitedWords::Validate(
int days_abs_diff = std::abs(days_actual - days_encoded);
if (days_abs_diff <= 1) {
*pure_words = recombined_pure_words;
return WordsValidationResult::kValid;
return WordsValidationStatus::kValid;
} else if (days_actual > days_encoded) {
return WordsValidationResult::kExpired;
return WordsValidationStatus::kExpired;
} else if (days_encoded > days_actual) {
return WordsValidationResult::kValidForTooLong;
return WordsValidationStatus::kValidForTooLong;
}
} else {
return WordsValidationResult::kNotValidPureWords;
return WordsValidationStatus::kNotValidPureWords;
}
} else {
return WordsValidationResult::kWrongWordsNumber;
return WordsValidationStatus::kWrongWordsNumber;
}

NOTREACHED();
return WordsValidationResult::kNotValidPureWords;
return WordsValidationStatus::kNotValidPureWords;
}

TimeLimitedWords::PureWordsWithStatus TimeLimitedWords::Parse(
const std::string& time_limited_words) {
PureWordsWithStatus ret;
std::string pure_words;
ret.status = Validate(time_limited_words, &pure_words);

if (ret.status == WordsValidationStatus::kValid) {
ret.pure_words = pure_words;
} else {
ret.pure_words = absl::nullopt;
}

return ret;
}

} // namespace brave_sync
28 changes: 22 additions & 6 deletions components/brave_sync/time_limited_words.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@

#include "base/gtest_prod_util.h"
#include "base/time/time.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace brave_sync {

enum class WordsValidationResult {
enum class WordsValidationStatus {
kValid = 0,
kNotValidPureWords = 1,
kVersionDeprecated = 2,
Expand All @@ -27,14 +28,26 @@ FORWARD_DECLARE_TEST(TimeLimitedWordsTest, GenerateForDate);
FORWARD_DECLARE_TEST(TimeLimitedWordsTest, GetIndexByWord);
FORWARD_DECLARE_TEST(TimeLimitedWordsTest, GetRoundedDaysDiff);
FORWARD_DECLARE_TEST(TimeLimitedWordsTest, GetWordByIndex);
FORWARD_DECLARE_TEST(TimeLimitedWordsTest, Validate);
FORWARD_DECLARE_TEST(TimeLimitedWordsTest, Parse);

class TimeLimitedWords {
public:
static std::string GenerateForNow(const std::string& pure_words);
struct PureWordsWithStatus {
PureWordsWithStatus();
PureWordsWithStatus(PureWordsWithStatus&& other);
PureWordsWithStatus& operator=(PureWordsWithStatus&& other);

static WordsValidationResult Validate(const std::string& time_limited_words,
std::string* pure_words);
PureWordsWithStatus(const PureWordsWithStatus&) = delete;
PureWordsWithStatus& operator=(const PureWordsWithStatus&) = delete;

~PureWordsWithStatus();

absl::optional<std::string> pure_words;
WordsValidationStatus status;
};

static std::string GenerateForNow(const std::string& pure_words);
static PureWordsWithStatus Parse(const std::string& time_limited_words);

static base::Time GetWordsV1SunsetDay();
static base::Time GetWordsV2Epoch();
Expand All @@ -44,7 +57,10 @@ class TimeLimitedWords {
FRIEND_TEST_ALL_PREFIXES(TimeLimitedWordsTest, GetIndexByWord);
FRIEND_TEST_ALL_PREFIXES(TimeLimitedWordsTest, GetRoundedDaysDiff);
FRIEND_TEST_ALL_PREFIXES(TimeLimitedWordsTest, GetWordByIndex);
FRIEND_TEST_ALL_PREFIXES(TimeLimitedWordsTest, Validate);
FRIEND_TEST_ALL_PREFIXES(TimeLimitedWordsTest, Parse);

static WordsValidationStatus Validate(const std::string& time_limited_words,
std::string* pure_words);

static std::string GenerateForDate(const std::string& pure_words,
const base::Time& not_after);
Expand Down
Loading

0 comments on commit bf67782

Please sign in to comment.