github-actions[bot] commented on code in PR #4550:
URL: https://github.com/apache/iggy/pull/4550#discussion_r4286586443
##########
core/server/src/http/handlers.rs:
##########
@@ -2061,11 +2101,17 @@ pub(in crate::http) async fn delete_pat(
Ok(StatusCode::NO_CONTENT)
}
-/// Issue a fresh access token for `user_id` and wrap it in the exact
-/// `IdentityInfo` shape the SDKs pin: numeric `user_id` plus an `access_token`
-/// carrying the token string and its unix-seconds expiry.
-fn issue_identity(inner: &HttpInner, user_id: u32) ->
Result<Json<IdentityInfo>, CustomError> {
- let generated = inner.jwt.generate(user_id)?;
+/// Mint a token bound to the active account at `user_id`. A refresh passes the
+/// `created_at` its token names, so it never re-issues for a newer account
that
+/// took a deleted user's id. A login passes `None`, because it just verified
Review Comment:
nit: The comment says that a login passes `None`, but `login_user` and
`login_with_personal_access_token` both pass `Some(created_at)`. Rewrite the
sentence to match, since every caller passes a verified creation stamp.
##########
core/server/config.toml:
##########
@@ -165,9 +165,9 @@ use_base64_secret = false
# Trusted issuers for A2A (Application-to-Application) authentication. Opt-in:
# with none configured the listener accepts only self-issued HMAC tokens.
-# `issuer`, `audience` and `jwks_url` are required per entry; `user_id` is
-# optional but defaults to 0 (root), which is rejected - set it to the non-zero
-# iggy user every token from that issuer is remapped onto.
+# `issuer`, `audience`, `jwks_url` and `username` are required per entry. Every
Review Comment:
nit: `HttpJwt::build` rejects an empty `issuer`, `jwks_url` or `username` at
startup, but not an empty `audience`, so the comment above overstates the
check. Add the matching check: an empty audience cannot match a real token's
`aud` claim, so the misconfiguration surfaces as 401s instead of a startup
error.
##########
core/metadata/src/stm/result.rs:
##########
@@ -216,6 +231,8 @@ result_enum!(TruncatePartitionResult {
result_enum!(CreateUserResult {
InvalidUsername = 43,
UserAlreadyExists = 46,
+ StreamNotFound = 1009,
Review Comment:
nit: `CreateUserResult` and `UpdatePermissionsResult` gained
`StreamNotFound` and `TopicNotFound`, but the test that claims to pin every
discriminant omits them. Add the four assertions, so a renumber still fails
here.
##########
core/server/src/http/handlers.rs:
##########
@@ -2061,11 +2101,17 @@ pub(in crate::http) async fn delete_pat(
Ok(StatusCode::NO_CONTENT)
}
-/// Issue a fresh access token for `user_id` and wrap it in the exact
-/// `IdentityInfo` shape the SDKs pin: numeric `user_id` plus an `access_token`
-/// carrying the token string and its unix-seconds expiry.
-fn issue_identity(inner: &HttpInner, user_id: u32) ->
Result<Json<IdentityInfo>, CustomError> {
- let generated = inner.jwt.generate(user_id)?;
+/// Mint a token bound to the active account at `user_id`. A refresh passes the
+/// `created_at` its token names, so it never re-issues for a newer account
that
+/// took a deleted user's id. A login passes `None`, because it just verified
+/// the account's own credentials.
+fn issue_identity(
+ inner: &HttpInner,
+ user_id: u32,
+ expected_created_at: Option<IggyTimestamp>,
Review Comment:
simplification: All three callers pass `Some`, so the `Option` on
`issue_identity`'s `expected_created_at` only carries an always-present value.
Take `IggyTimestamp` directly and drop the option.
##########
core/simulator/src/client.rs:
##########
@@ -241,12 +241,19 @@ impl SimClient {
#[allow(clippy::cast_possible_truncation)]
pub fn register(&self) -> Message<RoutedRequestHeader> {
let header_size = std::mem::size_of::<RoutedRequestHeader>();
- let verifier =
- consensus::client_table::bind_verifier(self.client_id, 0,
&self.bind_secret());
+ let registration =
iggy_binary_protocol::requests::users::login_register::RegisterRequest {
+ user_created_at: 1,
Review Comment:
nit: This register hard-codes `user_created_at: 1`, which must equal the
fixed stamp `ensure_root_user` gives the root user, or the commit gate refuses
it. Reuse one named sentinel here, so a change to the root bootstrap cannot
break the simulator.
##########
core/shard/src/poll.rs:
##########
@@ -418,16 +446,40 @@ where
if reply.is_disconnected() {
return;
}
- if attachment.is_some_and(|attachment| {
- !attachment.session.is_valid()
- || !attachment
- .metadata
- .is_valid(self.plane.metadata().mux_stm.streams(),
namespace)
- }) {
- let _ = reply.try_send(PartitionReadReply::Rejected(
- IggyError::TransientNotAccepted,
- ));
- return;
+ if let Some(attachment) = &attachment {
+ let users = self.plane.metadata().mux_stm.users();
+ let (user_id, metadata, account_valid) = match attachment {
+ PollAttachment::Bound(attachment) => (
+ attachment.session.user_id(),
+ &attachment.metadata,
+ attachment.session.is_valid(),
+ ),
+ PollAttachment::Unbound {
+ user_id,
+ user_created_at,
+ metadata,
+ } => (
+ *user_id,
+ metadata,
+ users.is_active_account(*user_id, *user_created_at),
Review Comment:
simplification: The unbound poll completion reads the users metadata twice,
once for `is_active_account` and once for `authorize`. Fold both checks into
one read closure, so one snapshot supplies the account and its grants. Also at
`core/server/src/http/reads.rs:66`.
--
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]
