github-actions[bot] commented on code in PR #4614:
URL: https://github.com/apache/iggy/pull/4614#discussion_r4335351611
##########
foreign/cpp/src/client.cpp:
##########
@@ -164,6 +164,13 @@ void IggyBlockingClient::UpdateUser(const Identifier &user,
});
}
+std::string IggyBlockingClient::CreatePersonalAccessToken(std::string name,
const Expiry &expiry) {
+ return RethrowAsIggyException([this, &name, &expiry] {
Review Comment:
warning: `CreatePersonalAccessToken` does not forward `ErrorInfo`, so an
uncertain create throws without `Kind()` or `OperationId()` and the documented
`ResolveOperation` recovery is unavailable. Thread `error_info` through the
bridge, record the error in `client.rs`, and add the method to the
structured-error list.
##########
foreign/cpp/src/client.rs:
##########
@@ -1254,6 +1257,23 @@ impl Client {
})
}
+ pub fn create_personal_access_token(
+ &self,
+ name: String,
+ expiry_kind: String,
+ expiry_value: u64,
+ ) -> Result<String, String> {
+ let expiry = parse_message_expiry(&expiry_kind, expiry_value)?;
Review Comment:
warning: The new call forwards any non-zero `Expiry::Duration`, so a value
near `u64::MAX` overflows the `now + micros` addition in `calculate_expiry_at`
and makes the metadata apply panic in builds with overflow checks. Use
`checked_add` and reject the request.
##########
foreign/cpp/include/iggy.hpp:
##########
@@ -3670,6 +3670,21 @@ class IggyBlockingClient final {
std::optional<UserStatus> status,
const UserUpdateOptions &options = {});
+ /**
+ * @brief Creates a personal access token for the authenticated user.
+ *
+ * The token string is only returned once upon creation and cannot be
+ * retrieved again.
+ *
+ * @param name Unique token name.
Review Comment:
nit: The token name limit depends on the transport: 1-255 bytes on binary
transports and 3-30 bytes on HTTP. Document both bounds on `name`.
##########
foreign/cpp/tests/e2e/client.cpp:
##########
@@ -3025,3 +3025,142 @@ TEST_F(LowLevelE2E_Client,
SendBinaryRequestUnknownCommandCodeThrows) {
EXPECT_FALSE(error_info.has_operation_id);
EXPECT_EQ(error_info.operation_id, 0u);
}
+
+TEST_F(E2E_Client, CreatePersonalAccessTokenBeforeLoginThrows) {
+ RecordProperty("description", "Rejects personal access token creation
without an active authenticated session.");
+ auto root_client = GetLoggedInHighLevelClient();
+ const std::string username = GetRandomName(30);
+ ASSERT_NO_THROW({ CreateUser(root_client, username, "secret123",
iggy::UserStatus::Active); });
+ auto client = GetLoggedOutHighLevelClient();
+ const std::string token_name = GetRandomName(30);
+
+ ASSERT_THROW(client.CreatePersonalAccessToken(token_name,
iggy::Expiry::ServerDefault()), iggy::IggyException);
+ ASSERT_NO_THROW(client.Connect());
+ ASSERT_THROW(client.CreatePersonalAccessToken(token_name,
iggy::Expiry::ServerDefault()), iggy::IggyException);
+ ASSERT_NO_THROW(client.Login(username, "secret123"));
+ std::string token;
+ ASSERT_NO_THROW({ token = client.CreatePersonalAccessToken(token_name,
iggy::Expiry::ServerDefault()); });
+ EXPECT_FALSE(token.empty());
+}
+
+TEST_F(E2E_Client, CreatePersonalAccessTokenReturnsTokenString) {
+ RecordProperty("description", "Creates tokens with ASCII and multibyte
UTF-8 names and returns their raw strings.");
+ auto root_client = GetLoggedInHighLevelClient();
+ const std::string username = GetRandomName(30);
+ ASSERT_NO_THROW({ CreateUser(root_client, username, "secret123",
iggy::UserStatus::Active); });
+ auto client =
GetLoggedInHighLevelClient(username, "secret123");
+ const std::vector<std::string> token_names = {
+ "ascii-token",
+ "caf\xc3\xa9",
+ "\xe6\x97\xa5\xe6\x9c\xac\xe8\xaa\x9e",
+ "token-\xf0\x9f\x8d\xb7",
+ };
+
+ for (const auto &token_name : token_names) {
+ SCOPED_TRACE(token_name);
+ std::string token;
+ ASSERT_NO_THROW({ token = client.CreatePersonalAccessToken(token_name,
iggy::Expiry::ServerDefault()); });
Review Comment:
nit: Every new test passes `Expiry::ServerDefault()`, so the `NeverExpire`
and `Duration` expiry paths through the new bridge stay untested. Add a case
that creates a token with `Expiry::Duration(...)` and logs in with it.
##########
foreign/cpp/include/iggy.hpp:
##########
@@ -3670,6 +3670,21 @@ class IggyBlockingClient final {
std::optional<UserStatus> status,
const UserUpdateOptions &options = {});
+ /**
+ * @brief Creates a personal access token for the authenticated user.
+ *
+ * The token string is only returned once upon creation and cannot be
+ * retrieved again.
+ *
+ * @param name Unique token name.
+ * @param expiry Expiry policy for the token.
Review Comment:
warning: For a personal access token, `Expiry::ServerDefault()` and
`Expiry::NeverExpire()` both create a credential that never expires, but the
parameter doc does not say so. Document that `ServerDefault()` means no expiry
for tokens.
##########
foreign/cpp/include/iggy.hpp:
##########
@@ -3670,6 +3670,21 @@ class IggyBlockingClient final {
std::optional<UserStatus> status,
const UserUpdateOptions &options = {});
+ /**
+ * @brief Creates a personal access token for the authenticated user.
+ *
+ * The token string is only returned once upon creation and cannot be
Review Comment:
warning: A lost reply can commit the token while its secret is
unrecoverable, and the C++ API exposes no delete method to remove that token.
Expose `DeletePersonalAccessToken` and document that a retry returns
`PersonalAccessTokenAlreadyExists`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
