feat: support 3D-RBAC stateful token authentication#19
Open
SoukNow wants to merge 1 commit into
Open
Conversation
Reviewer's GuideAdds 3D-RBAC-compatible, stateful token authentication support to the CLI by extending client configuration and WebSocket handshake metadata, updates docs for JWT token/scopes, and introduces Docker-based deployment artifacts alongside a minor version bump. Sequence diagram for WebSocket auth handshake with 3D-RBAC metadatasequenceDiagram
participant CLI as FnsCliClient
participant Service as FastNoteSyncService
CLI->>Service: WebSocket connect /api/user/sync?lang=zh-cn&count&client&clientName&clientVersion
Service-->>CLI: [auth response]
CLI->>CLI: _on_auth_response(msg)
CLI->>Service: _raw_send(WSMessage ACTION_CLIENT_INFO {name,version,type,isDesktop,isMobile,...})
CLI->>Service: _flush_queue()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
client.pyyou added platform flags usingsys.platformbut there is no correspondingimport sysin the file, which will cause a runtime error when sendingClientInfo. - You now have both a configurable
client_typeand a hard-codedclientName=FastNodeSync-CLIin the WebSocket URL; consider derivingclientName(and possibly theClientInfo.name) from the config to avoid mismatches when users changeclient_type.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `client.py` you added platform flags using `sys.platform` but there is no corresponding `import sys` in the file, which will cause a runtime error when sending `ClientInfo`.
- You now have both a configurable `client_type` and a hard-coded `clientName=FastNodeSync-CLI` in the WebSocket URL; consider deriving `clientName` (and possibly the `ClientInfo.name`) from the config to avoid mismatches when users change `client_type`.
## Individual Comments
### Comment 1
<location path="Dockerfile" line_range="1-10" />
<code_context>
+FROM python:3.13-slim
+
+WORKDIR /app
+
+# Install dependencies first for better layer caching
+COPY requirements.txt .
+RUN pip install --no-cache-dir -r requirements.txt
+
+# Copy application code
+COPY fns_cli/ ./fns_cli/
+
+# Default config mount point
+VOLUME ["/app/config", "/app/vault"]
+
+ENV PYTHONUNBUFFERED=1
+
+ENTRYPOINT ["python", "-m", "fns_cli.main"]
+CMD ["run", "-c", "/app/config/config.yaml"]
</code_context>
<issue_to_address>
**🚨 issue (security):** The container runs as root; consider adding a non-root user for improved security.
Running the container as `root` amplifies the impact of any compromise of the CLI or its dependencies. Please add a dedicated non-root user (e.g., `fns`), `chown` `/app`, `/app/config`, and `/app/vault` to that user, and switch to it with `USER`. This usually needs no code changes and significantly reduces risk.
</issue_to_address>
### Comment 2
<location path="tests/test_client.py" line_range="24" />
<code_context>
config = MagicMock()
config.client.reconnect_base_delay = 1
config.client.reconnect_max_retries = 3
+ config.client.client_type = "fns-cli"
config.server.token = "token"
config.ws_api = "wss://example.com"
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to verify the new WebSocket URL query parameters (`client`, `clientName`, `clientVersion`).
Right now the tests only set `config.client.client_type` but never verify that `_connect` (or its public entrypoint) actually uses these values. Please add/extend a test that drives a real connection attempt and asserts that the final WebSocket URL includes:
- `client=fns-cli` (or the configured value)
- `clientName=FastNodeSync-CLI`
- `clientVersion` equal to `fns_cli.__version__`.
This will guard against regressions in URL construction for the new query parameters.
Suggested implementation:
```python
config = MagicMock()
config.client.reconnect_base_delay = 1
config.client.reconnect_max_retries = 3
config.client.client_type = "fns-cli"
config.server.token = "token"
config.ws_api = "wss://example.com"
return config
def test_websocket_url_includes_client_query_params(mocker):
from urllib.parse import urlparse, parse_qs
import fns_cli
# Arrange
config = _make_config() # adjust to the actual helper/factory name if different
client = FastNodeSyncClient(config) # adjust to the actual client class under test
# Patch the websocket connect function used by the client to initiate connections
mock_connect = mocker.patch(
"fns_cli.client.websocket.connect", autospec=True
) # adjust import path to match implementation
# Act: trigger a connection attempt (use the public entrypoint if available)
client._connect() # or client.connect(), depending on what is exposed
# Assert: inspect the URL used for the WebSocket connection
called_url = mock_connect.call_args[0][0]
parsed = urlparse(called_url)
qs = parse_qs(parsed.query)
assert qs["client"] == [config.client.client_type]
assert qs["clientName"] == ["FastNodeSync-CLI"]
assert qs["clientVersion"] == [fns_cli.__version__]
```
To integrate this test with the existing codebase, you will likely need to:
1. Replace `_make_config()` with the actual helper/fixture used to construct the `config` `MagicMock`. If this is a pytest fixture (e.g. `config`), change the test signature to accept it instead of calling a function.
2. Replace `FastNodeSyncClient` with the actual client class name being tested (for example, `WebsocketClient` or similar).
3. Adjust the `mocker.patch` target `"fns_cli.client.websocket.connect"` to point to the exact function used by your `_connect` implementation (e.g. `"fns_cli.client._websocket_connect"` or `"fns_cli.client.websocket.WebSocketApp"`), and adapt how you extract the URL from `call_args` if the signature differs.
4. Prefer calling the public entrypoint used to start a connection (e.g. `client.connect()` or `client.start()`) instead of `client._connect()` if such a method exists, so the test remains resilient to internal refactors.
5. If your code constructs the WebSocket URL in a separate helper, ensure this test exercises that same path (not a duplicated or test-only code path).
</issue_to_address>
### Comment 3
<location path="README.md" line_range="97" />
<code_context>
**How to obtain a token**
+> **Note:** Since service version with 3D-RBAC, tokens are now JWT-based and managed via the web UI.
+
1. Open the Fast Note Sync Service web UI (e.g. `https://your-server-address`)
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding an article before "service version" for grammatical correctness.
The wording "Since service version with 3D-RBAC" is ungrammatical. Consider revising to "Since the service version with 3D-RBAC" or "Since the 3D-RBAC-enabled service version" to improve clarity.
```suggestion
> **Note:** Since the 3D-RBAC-enabled service version, tokens are now JWT-based and managed via the web UI.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1
to
+10
| FROM python:3.13-slim | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| # Install dependencies first for better layer caching | ||
| COPY requirements.txt . | ||
| RUN pip install --no-cache-dir -r requirements.txt | ||
|
|
||
| # Copy application code | ||
| COPY fns_cli/ ./fns_cli/ |
There was a problem hiding this comment.
🚨 issue (security): The container runs as root; consider adding a non-root user for improved security.
Running the container as root amplifies the impact of any compromise of the CLI or its dependencies. Please add a dedicated non-root user (e.g., fns), chown /app, /app/config, and /app/vault to that user, and switch to it with USER. This usually needs no code changes and significantly reduces risk.
| config = MagicMock() | ||
| config.client.reconnect_base_delay = 1 | ||
| config.client.reconnect_max_retries = 3 | ||
| config.client.client_type = "fns-cli" |
There was a problem hiding this comment.
suggestion (testing): Add tests to verify the new WebSocket URL query parameters (client, clientName, clientVersion).
Right now the tests only set config.client.client_type but never verify that _connect (or its public entrypoint) actually uses these values. Please add/extend a test that drives a real connection attempt and asserts that the final WebSocket URL includes:
client=fns-cli(or the configured value)clientName=FastNodeSync-CLIclientVersionequal tofns_cli.__version__.
This will guard against regressions in URL construction for the new query parameters.
Suggested implementation:
config = MagicMock()
config.client.reconnect_base_delay = 1
config.client.reconnect_max_retries = 3
config.client.client_type = "fns-cli"
config.server.token = "token"
config.ws_api = "wss://example.com"
return config
def test_websocket_url_includes_client_query_params(mocker):
from urllib.parse import urlparse, parse_qs
import fns_cli
# Arrange
config = _make_config() # adjust to the actual helper/factory name if different
client = FastNodeSyncClient(config) # adjust to the actual client class under test
# Patch the websocket connect function used by the client to initiate connections
mock_connect = mocker.patch(
"fns_cli.client.websocket.connect", autospec=True
) # adjust import path to match implementation
# Act: trigger a connection attempt (use the public entrypoint if available)
client._connect() # or client.connect(), depending on what is exposed
# Assert: inspect the URL used for the WebSocket connection
called_url = mock_connect.call_args[0][0]
parsed = urlparse(called_url)
qs = parse_qs(parsed.query)
assert qs["client"] == [config.client.client_type]
assert qs["clientName"] == ["FastNodeSync-CLI"]
assert qs["clientVersion"] == [fns_cli.__version__]To integrate this test with the existing codebase, you will likely need to:
- Replace
_make_config()with the actual helper/fixture used to construct theconfigMagicMock. If this is a pytest fixture (e.g.config), change the test signature to accept it instead of calling a function. - Replace
FastNodeSyncClientwith the actual client class name being tested (for example,WebsocketClientor similar). - Adjust the
mocker.patchtarget"fns_cli.client.websocket.connect"to point to the exact function used by your_connectimplementation (e.g."fns_cli.client._websocket_connect"or"fns_cli.client.websocket.WebSocketApp"), and adapt how you extract the URL fromcall_argsif the signature differs. - Prefer calling the public entrypoint used to start a connection (e.g.
client.connect()orclient.start()) instead ofclient._connect()if such a method exists, so the test remains resilient to internal refactors. - If your code constructs the WebSocket URL in a separate helper, ensure this test exercises that same path (not a duplicated or test-only code path).
|
|
||
| **How to obtain a token** | ||
|
|
||
| > **Note:** Since service version with 3D-RBAC, tokens are now JWT-based and managed via the web UI. |
There was a problem hiding this comment.
suggestion (typo): Consider adding an article before "service version" for grammatical correctness.
The wording "Since service version with 3D-RBAC" is ungrammatical. Consider revising to "Since the service version with 3D-RBAC" or "Since the 3D-RBAC-enabled service version" to improve clarity.
Suggested change
| > **Note:** Since service version with 3D-RBAC, tokens are now JWT-based and managed via the web UI. | |
| > **Note:** Since the 3D-RBAC-enabled service version, tokens are now JWT-based and managed via the web UI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adapt CLI to support the upgraded fast-note-sync-service 3D-RBAC authentication.
Why
The service upgraded from single stateless token to stateful multi-dimensional
(protocol, client, function) validation token, breaking the old auth flow.
Changes
client/clientName/clientVersionquery params to WebSocket URLClientInfomessage with platform detection fieldsclient_typeconfig field (default:fns-cli)Testing
All 46 unit tests pass.
Summary by Sourcery
Add 3D-RBAC-aware client identification and platform metadata to the CLI authentication flow and introduce containerized deployment support.
New Features:
Enhancements:
Documentation:
Tests: