slbotbm commented on code in PR #3727:
URL: https://github.com/apache/iggy/pull/3727#discussion_r3651993941
##########
foreign/python/src/client.rs:
##########
@@ -264,6 +268,97 @@ impl IggyClient {
})
}
+ /// Update the permissions of a user by unique ID or username.
+ ///
+ /// This is a full replacement: the given permissions overwrite the
previous
+ /// ones, and `None` removes them entirely.
+ ///
+ /// Args:
+ /// user_id: User identifier as `str | int`.
+ /// permissions: New permissions as `Permissions | None`.
+ ///
+ /// Returns:
+ /// An awaitable that resolves to `None` when the permissions are
updated.
+ ///
+ /// Raises:
+ /// PyValueError: If a string identifier is invalid.
+ /// PyRuntimeError: If the request fails.
+ #[pyo3(signature = (user_id, permissions=None))]
Review Comment:
Let's make the permission explicit with `#[pyo3(signature = (user_id,
permissions))]` so that clearing the permissions will have to be explicitly
specified with `None`.
##########
foreign/python/src/permissions.rs:
##########
Review Comment:
Empty `streams` and `topics` dictionaries are preserved locally but cannot
be represented distinctly on the wire. Both become empty vectors and decode as
`None`, so `Permissions(streams={})` and `StreamPermissions(topics={})` fail
round-trip equality. Please normalize empty dictionaries to `None` in these
constructors and cover both cases.
##########
foreign/python/src/permissions.rs:
##########
@@ -0,0 +1,406 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use iggy::prelude::{
+ GlobalPermissions as RustGlobalPermissions, Permissions as RustPermissions,
+ StreamPermissions as RustStreamPermissions, TopicPermissions as
RustTopicPermissions,
+};
+use pyo3::prelude::*;
+use pyo3_stub_gen::derive::{gen_stub_pyclass, gen_stub_pymethods};
+use std::collections::BTreeMap;
+
+/// The permissions of a user: global permissions applied to all streams,
+/// optionally extended by per-stream permissions.
+#[derive(Debug, Clone, PartialEq)]
+#[gen_stub_pyclass]
+#[pyclass(eq, from_py_object)]
+pub struct Permissions {
+ pub(crate) inner: RustPermissions,
+}
+
+impl From<RustPermissions> for Permissions {
+ fn from(permissions: RustPermissions) -> Self {
+ Self { inner: permissions }
+ }
+}
+
+#[gen_stub_pymethods]
+#[pymethods]
+impl Permissions {
+ /// Create permissions from global permissions and optional per-stream
permissions.
+ ///
+ /// Args:
+ /// global_: Global permissions as `GlobalPermissions | None`;
defaults to all denied.
+ /// streams: Per-stream permissions keyed by stream ID as
+ /// `dict[int, StreamPermissions] | None`.
+ #[new]
+ #[pyo3(signature = (global_=None, streams=None))]
+ fn new(
+ #[gen_stub(override_type(type_repr = "GlobalPermissions | None"))]
global_: Option<
+ GlobalPermissions,
+ >,
+ #[gen_stub(override_type(type_repr = "dict[int, StreamPermissions] |
None"))]
+ streams: Option<BTreeMap<u32, StreamPermissions>>,
+ ) -> Self {
+ Self {
+ inner: RustPermissions {
+ global: global_.map(|global| global.inner).unwrap_or_default(),
+ streams: streams.map(|streams| {
+ streams
+ .into_iter()
+ .map(|(stream_id, stream)| (stream_id as usize,
stream.inner))
+ .collect()
+ }),
+ },
+ }
+ }
+
+ /// The global permissions, applied to all streams.
+ #[getter]
+ fn global_(&self) -> GlobalPermissions {
+ GlobalPermissions {
+ inner: self.inner.global.clone(),
+ }
+ }
+
+ /// The per-stream permissions keyed by stream ID, or `None` when not set.
+ #[getter]
+ #[gen_stub(override_return_type(type_repr = "dict[int, StreamPermissions]
| None"))]
+ fn streams(&self) -> Option<BTreeMap<u32, StreamPermissions>> {
+ self.inner.streams.as_ref().map(|streams| {
+ streams
+ .iter()
+ .map(|(stream_id, stream)| {
+ // IDs are u32 on the wire, so the cast cannot truncate.
+ (
+ *stream_id as u32,
+ StreamPermissions {
+ inner: stream.clone(),
+ },
+ )
+ })
+ .collect()
+ })
+ }
+}
+
+/// Global permissions, applied to all streams without specifying them one by
one.
+#[derive(Debug, Clone, PartialEq)]
+#[gen_stub_pyclass]
+#[pyclass(eq, from_py_object)]
+pub struct GlobalPermissions {
+ pub(crate) inner: RustGlobalPermissions,
+}
+
+#[gen_stub_pymethods]
+#[pymethods]
+impl GlobalPermissions {
+ /// Create global permissions. Every flag defaults to `False`.
+ ///
+ /// Args:
+ /// manage_servers: Allow managing servers; includes `read_servers`.
+ /// read_servers: Allow reading server info (stats, clients).
+ /// manage_users: Allow managing users; includes `read_users`.
+ /// read_users: Allow reading user info.
+ /// manage_streams: Allow managing all streams; includes
`manage_topics`.
+ /// read_streams: Allow reading all streams; includes `read_topics`.
+ /// manage_topics: Allow managing all topics; includes `read_topics`.
+ /// read_topics: Allow reading all topics and consumer groups.
+ /// poll_messages: Allow polling messages from all streams.
+ /// send_messages: Allow sending messages to all streams.
Review Comment:
This documentation does not actually describe permission inheritance:
- `read_topics` also permits polling message contents.
- `manage_topics` permits polling and sending messages.
- `manage_streams` inherits topic management and therefore permits
polling and sending.
- Stream `read_topics` and topic `read_topic` permit polling.
- Stream and topic management permissions permit sending.
##########
foreign/python/tests/test_permissions.py:
##########
Review Comment:
Let's also add the following tests:
- User without `manage_users` cannot change another user's password.
- User with `manage_users` can change another user's password.
- Failed unauthorized change leaves the old credentials valid.
##########
foreign/python/src/permissions.rs:
##########
Review Comment:
`GlobalPermissions`, `StreamPermissions`, and `TopicPermissions` accept
positional booleans. Ten adjacent booleans make this legal and dangerous:
```python
GlobalPermissions(False, False, True, ...)
```
A shifted argument can grant the wrong privilege without any error. These
are new security-sensitive APIs. Add `*` to their PyO3 signatures so permission
flags must be named. Existing tests already use keyword arguments.
--
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]
