[docs] Document Django Channels WebSocket API endpoints#691
[docs] Document Django Channels WebSocket API endpoints#691iamavichal-geek wants to merge 2 commits intoopenwisp:masterfrom
Conversation
- add docs/user/websocket-api.rst (endpoint URL, auth, message types, JS example, deployment setup) - register new page in docs/index.rst toctree - add WebSocket cross-references to generating_users.rst and importing_users.rst - add missing label + note to OPENWISP_RADIUS_BATCH_ASYNC_THRESHOLD in settings.rst Closes openwisp#682
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds documentation for a WebSocket API that streams real-time status for batch user creation/import operations. Introduces docs/user/websocket-api.rst and updates docs/index.rst, docs/user/generating_users.rst, docs/user/importing_users.rst, and docs/user/settings.rst to reference WebSocket-driven batch status delivery when batch processing runs asynchronously (via Celery) after crossing the configured OPENWISP_RADIUS_BATCH_ASYNC_THRESHOLD. Sequence Diagram(s)sequenceDiagram
participant Client
participant Channels as "Django Channels (ASGI)"
participant Session as "Django Session/Auth"
participant Celery as "Celery Worker"
participant DB as "Database"
Client->>Channels: CONNECT wss://host/ws/radius/batch/<batch-id>/
Channels->>Session: Validate session & staff permissions
Session-->>Channels: Auth result
alt Authenticated
Channels-->>Client: Accept connection
Note over Channels,Celery: Batch created/queued via REST import or generation
Celery->>DB: Process batch asynchronously
Celery-->>Channels: Publish batch_status_update (pending/processing/completed/failed)
Channels-->>Client: Push `batch_status_update` (only terminal statuses guaranteed)
Client->>Channels: Close (optional)
Channels-->>Client: Close connection
else Unauthorized
Channels-->>Client: Reject connection
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: 1 Critical Issue Found | Recommendation: Fix before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)The Current code: RadiusBatch.objects.filter(
pk=batch_id, organization__in=user.organizations_managed
).exists() # Result discarded!
return True # Always returns TrueShould be: return RadiusBatch.objects.filter(
pk=batch_id, organization__in=user.organizations_managed
).exists()This is a security vulnerability - the documentation at line 47-57 correctly describes org-scoped access control, but the implementation doesn't enforce it. Files Reviewed (5 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/websocket-api.rst`:
- Around line 91-139: The docs incorrectly guarantee "exactly one message per
connection"; update the text to state that RadiusBatchConsumer simply forwards
the single group_send emitted by RadiusBatch.process() and does not send the
current batch state on connect, so a client that subscribes after the terminal
group_send has already been published may receive zero messages; change the
wording to clarify that the server will send at most one terminal
batch_status_update (completed/failed) for a given batch and recommend clients
handle the zero-message case (e.g., by polling the REST API or re-subscribing)
instead of relying on a guaranteed message on connect.
- Around line 175-180: Update the wording to clarify that adding "daphne" as the
first entry in INSTALLED_APPS is only to override Django's runserver for local
development (to enable ASGI/WebSocket support in manage.py runserver), and does
not affect production ASGI deployment; mention that in production you run the
ASGI server directly (e.g., daphne myproject.asgi:application) and that
INSTALLED_APPS ordering is not used to choose the production server.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a59e4d6-fd0f-4cb7-a186-3156f0e4dc82
📒 Files selected for processing (5)
docs/index.rstdocs/user/generating_users.rstdocs/user/importing_users.rstdocs/user/settings.rstdocs/user/websocket-api.rst
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
docs/user/websocket-api.rst
Outdated
| After the connection is established, the client does not need to send | ||
| any messages. The server pushes exactly **one** message when batch | ||
| processing finishes (either successfully or with an error). | ||
|
|
||
| Message type: ``batch_status_update`` | ||
|
|
||
| .. code-block:: javascript | ||
|
|
||
| { | ||
| "status": "<status>" | ||
| } | ||
|
|
||
| The ``status`` field contains one of the following values: | ||
|
|
||
| .. list-table:: | ||
| :header-rows: 1 | ||
|
|
||
| * - Value | ||
| - Description | ||
| * - ``"pending"`` | ||
| - The batch has been created but processing has not yet started. | ||
| This value is not sent via WebSocket; it is visible only through | ||
| the REST API or admin interface. | ||
| * - ``"processing"`` | ||
| - The batch is currently being processed. This value is not sent | ||
| via WebSocket; it is the status visible when the admin page is | ||
| opened and the WebSocket connection is established. | ||
| * - ``"completed"`` | ||
| - Batch processing finished successfully. This is a terminal status. | ||
| * - ``"failed"`` | ||
| - Batch processing encountered an error. This is a terminal status. | ||
|
|
||
| .. note:: | ||
|
|
||
| The server sends exactly one message per connection, always with a | ||
| terminal status (``"completed"`` or ``"failed"``). The client should | ||
| close the connection after receiving it. | ||
|
|
||
| Connection Lifecycle | ||
| ++++++++++++++++++++ | ||
|
|
||
| 1. The client connects to the endpoint with the batch UUID in the URL. | ||
| 2. If the user is authorized, the connection is accepted and the client | ||
| is added to the channel group ``radius_batch_<batch-id>``. | ||
| 3. When batch processing finishes, the server sends one | ||
| ``batch_status_update`` message containing the terminal status. | ||
| 4. The client should close the connection upon receiving ``"completed"`` | ||
| or ``"failed"``. | ||
| 5. On disconnect, the client is removed from the channel group. |
There was a problem hiding this comment.
Don't promise a terminal message for every accepted connection.
RadiusBatchConsumer only forwards the single group_send emitted from RadiusBatch.process(); it does not send the current batch state on connect. A client that subscribes after that terminal event has already been published will receive zero messages, so the current “exactly one message per connection” wording can leave integrators waiting forever.
Suggested doc fix
-After the connection is established, the client does not need to send
-any messages. The server pushes exactly **one** message when batch
-processing finishes (either successfully or with an error).
+After the connection is established, the client does not need to send
+any messages. The server pushes at most **one** terminal message
+(``"completed"`` or ``"failed"``) to clients that are already connected
+when the terminal update is emitted.
@@
- The server sends exactly one message per connection, always with a
- terminal status (``"completed"`` or ``"failed"``). The client should
- close the connection after receiving it.
+ The server does not replay the last known status on connect. Clients
+ that subscribe after processing has already finished will not receive
+ a WebSocket message and should fall back to the REST API or a
+ client-side timeout.
@@
-3. When batch processing finishes, the server sends one
- ``batch_status_update`` message containing the terminal status.
+3. When batch processing finishes, the server sends one
+ ``batch_status_update`` message to clients that are already subscribed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| After the connection is established, the client does not need to send | |
| any messages. The server pushes exactly **one** message when batch | |
| processing finishes (either successfully or with an error). | |
| Message type: ``batch_status_update`` | |
| .. code-block:: javascript | |
| { | |
| "status": "<status>" | |
| } | |
| The ``status`` field contains one of the following values: | |
| .. list-table:: | |
| :header-rows: 1 | |
| * - Value | |
| - Description | |
| * - ``"pending"`` | |
| - The batch has been created but processing has not yet started. | |
| This value is not sent via WebSocket; it is visible only through | |
| the REST API or admin interface. | |
| * - ``"processing"`` | |
| - The batch is currently being processed. This value is not sent | |
| via WebSocket; it is the status visible when the admin page is | |
| opened and the WebSocket connection is established. | |
| * - ``"completed"`` | |
| - Batch processing finished successfully. This is a terminal status. | |
| * - ``"failed"`` | |
| - Batch processing encountered an error. This is a terminal status. | |
| .. note:: | |
| The server sends exactly one message per connection, always with a | |
| terminal status (``"completed"`` or ``"failed"``). The client should | |
| close the connection after receiving it. | |
| Connection Lifecycle | |
| ++++++++++++++++++++ | |
| 1. The client connects to the endpoint with the batch UUID in the URL. | |
| 2. If the user is authorized, the connection is accepted and the client | |
| is added to the channel group ``radius_batch_<batch-id>``. | |
| 3. When batch processing finishes, the server sends one | |
| ``batch_status_update`` message containing the terminal status. | |
| 4. The client should close the connection upon receiving ``"completed"`` | |
| or ``"failed"``. | |
| 5. On disconnect, the client is removed from the channel group. | |
| After the connection is established, the client does not need to send | |
| any messages. The server pushes at most **one** terminal message | |
| (``"completed"`` or ``"failed"``) to clients that are already connected | |
| when the terminal update is emitted. | |
| Message type: ``batch_status_update`` | |
| .. code-block:: javascript | |
| { | |
| "status": "<status>" | |
| } | |
| The ``status`` field contains one of the following values: | |
| .. list-table:: | |
| :header-rows: 1 | |
| * - Value | |
| - Description | |
| * - ``"pending"`` | |
| - The batch has been created but processing has not yet started. | |
| This value is not sent via WebSocket; it is visible only through | |
| the REST API or admin interface. | |
| * - ``"processing"`` | |
| - The batch is currently being processed. This value is not sent | |
| via WebSocket; it is the status visible when the admin page is | |
| opened and the WebSocket connection is established. | |
| * - ``"completed"`` | |
| - Batch processing finished successfully. This is a terminal status. | |
| * - ``"failed"`` | |
| - Batch processing encountered an error. This is a terminal status. | |
| .. note:: | |
| The server does not replay the last known status on connect. Clients | |
| that subscribe after processing has already finished will not receive | |
| a WebSocket message and should fall back to the REST API or a | |
| client-side timeout. | |
| Connection Lifecycle | |
| +++++++++++++++++++ | |
| 1. The client connects to the endpoint with the batch UUID in the URL. | |
| 2. If the user is authorized, the connection is accepted and the client | |
| is added to the channel group ``radius_batch_<batch-id>``. | |
| 3. When batch processing finishes, the server sends one | |
| ``batch_status_update`` message to clients that are already subscribed. | |
| 4. The client should close the connection upon receiving ``"completed"`` | |
| or ``"failed"``. | |
| 5. On disconnect, the client is removed from the channel group. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/user/websocket-api.rst` around lines 91 - 139, The docs incorrectly
guarantee "exactly one message per connection"; update the text to state that
RadiusBatchConsumer simply forwards the single group_send emitted by
RadiusBatch.process() and does not send the current batch state on connect, so a
client that subscribes after the terminal group_send has already been published
may receive zero messages; change the wording to clarify that the server will
send at most one terminal batch_status_update (completed/failed) for a given
batch and recommend clients handle the zero-message case (e.g., by polling the
REST API or re-subscribing) instead of relying on a guaranteed message on
connect.
docs/user/websocket-api.rst
Outdated
| Django's default WSGI server does not support WebSockets. You must use | ||
| an ASGI-compatible server such as `Daphne | ||
| <https://github.com/django/daphne>`_. | ||
|
|
||
| Install Daphne and add it as the **first entry** in ``INSTALLED_APPS`` | ||
| so that Django uses it as the ASGI server: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the official Django Channels and Daphne documentation, when and why should daphnebe added as the first entry inINSTALLED_APPS? Is that required for production ASGI deployment, or mainly for Daphne's runserver integration?
💡 Result:
Official docs say to put "daphne" at the start of INSTALLED_APPS when you want Daphne to provide/override Django’s runserver command (i.e., use Daphne as the development server for ASGI), and to ensure it’s ordered before apps like django.contrib.staticfiles that also hook runserver. In Channels 4.x this is explicitly called out because runserver moved into the daphne package and Channels adds a system check to enforce the ordering. [1], [2], [3]
This is not a general requirement for production ASGI deployment. For production you run an ASGI server directly (e.g., daphne myproject.asgi:application), and that does not depend on daphne being first in INSTALLED_APPS; the “put it first” guidance is mainly for Daphne’s runserver integration. [1], [2]
Sources:
[1] Django docs: “How to use Django with Daphne” (runserver integration and “add daphne to the start of INSTALLED_APPS”) (docs.djangoproject.com)
[2] Channels docs: “4.0.0 Release Notes” (runserver moved to daphne; ordering before django.contrib.staticfiles; system check) (channels.readthedocs.io)
[3] Channels tutorial (Daphne in installed apps replaces the standard dev server for runserver) (channels.readthedocs.io)
Citations:
- 1: https://docs.djangoproject.com/en/5.0/howto/deployment/asgi/daphne/?utm_source=openai
- 2: https://channels.readthedocs.io/en/stable/releases/4.0.0.html?utm_source=openai
- 3: https://channels.readthedocs.io/en/latest/tutorial/part_1.html?utm_source=openai
Clarify that daphne in INSTALLED_APPS is for development runserver integration, not production ASGI deployment.
The phrase "so that Django uses it as the ASGI server" is misleading. Per official Django/Channels docs, putting daphne first in INSTALLED_APPS is specifically to override Django's runserver command for development. Production ASGI deployment does not depend on INSTALLED_APPS ordering—you run the server directly (e.g., daphne myproject.asgi:application). The current wording conflates development and production concerns and should clarify the actual purpose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/user/websocket-api.rst` around lines 175 - 180, Update the wording to
clarify that adding "daphne" as the first entry in INSTALLED_APPS is only to
override Django's runserver for local development (to enable ASGI/WebSocket
support in manage.py runserver), and does not affect production ASGI deployment;
mention that in production you run the ASGI server directly (e.g., daphne
myproject.asgi:application) and that INSTALLED_APPS ordering is not used to
choose the production server.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/user/websocket-api.rst (2)
91-93:⚠️ Potential issue | 🟠 MajorDon't guarantee a terminal message on every connection.
RadiusBatchConsumerdoes not replay the current batch status on connect; it only forwards the singlegroup_sendemitted byRadiusBatch.process(). A client that subscribes after that terminal event has already been published will receive zero messages, so the repeated “exactly one message per connection” wording is inaccurate and can leave integrators waiting indefinitely.Suggested doc fix
-After the connection is established, the client does not need to send any -messages. The server pushes exactly **one** message when batch processing -finishes (either successfully or with an error). +After the connection is established, the client does not need to send any +messages. The server pushes at most **one** terminal message +(``"completed"`` or ``"failed"``) to clients that are already connected +when the terminal update is emitted. @@ - The server sends exactly one message per connection, always with a - terminal status (``"completed"`` or ``"failed"``). The client should - close the connection after receiving it. + The server does not replay the last known status on connect. Clients + that subscribe after processing has already finished may receive no + WebSocket message and should fall back to the REST API or a + client-side timeout. @@ -3. When batch processing finishes, the server sends one - ``batch_status_update`` message containing the terminal status. +3. When batch processing finishes, the server sends one + ``batch_status_update`` message to clients that are already subscribed.Also applies to: 125-127, 135-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/websocket-api.rst` around lines 91 - 93, Update the wording to remove the guarantee of a terminal message per connection: clarify that RadiusBatchConsumer forwards the single group_send emitted by RadiusBatch.process() and therefore the server may push at most one message (and possibly zero if the terminal event occurred before the client subscribed); mention RadiusBatchConsumer and RadiusBatch.process() so integrators know the consumer does not replay state on connect and may miss the event.
179-180:⚠️ Potential issue | 🟡 MinorClarify that
daphnefirst inINSTALLED_APPSis forrunserver, not production server selection.This wording implies
INSTALLED_APPSdecides the production ASGI server. In practice, puttingdaphnefirst is about Daphne'srunserverintegration for development; production runs the ASGI server directly.According to the official Django and Channels documentation, is placing "daphne" first in INSTALLED_APPS required for production ASGI deployment, or is it specifically for Daphne's manage.py runserver integration?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/websocket-api.rst` around lines 179 - 180, Update the sentence about installing "daphne" so it clearly states that placing "daphne" first in INSTALLED_APPS is required only to enable Daphne's manage.py runserver integration during development, and does not determine which ASGI server is used in production (production should run the ASGI server process directly); reference the symbols "daphne", "INSTALLED_APPS", "runserver" and "ASGI server" in the clarification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/user/websocket-api.rst`:
- Around line 91-93: Update the wording to remove the guarantee of a terminal
message per connection: clarify that RadiusBatchConsumer forwards the single
group_send emitted by RadiusBatch.process() and therefore the server may push at
most one message (and possibly zero if the terminal event occurred before the
client subscribed); mention RadiusBatchConsumer and RadiusBatch.process() so
integrators know the consumer does not replay state on connect and may miss the
event.
- Around line 179-180: Update the sentence about installing "daphne" so it
clearly states that placing "daphne" first in INSTALLED_APPS is required only to
enable Daphne's manage.py runserver integration during development, and does not
determine which ASGI server is used in production (production should run the
ASGI server process directly); reference the symbols "daphne", "INSTALLED_APPS",
"runserver" and "ASGI server" in the clarification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c87b638-5486-434b-8548-bd4789bdb51b
📒 Files selected for processing (4)
docs/user/generating_users.rstdocs/user/importing_users.rstdocs/user/settings.rstdocs/user/websocket-api.rst
iamavichal-geek
force-pushed
the
docs/websocket-api
branch
from
b4fbb6a to
9b9c193
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/user/websocket-api.rst (2)
91-93:⚠️ Potential issue | 🟠 MajorDon't promise a terminal message for every accepted connection.
The consumer forwards the single
group_sendemitted when processing finishes, but it does not replay the current batch state on connect. A client that subscribes after that terminal event has already been published will receive zero messages, so the repeated “exactly one message per connection” wording is inaccurate.Suggested doc fix
-After the connection is established, the client does not need to send any -messages. The server pushes exactly **one** message when batch processing -finishes (either successfully or with an error). +After the connection is established, the client does not need to send any +messages. The server may push **one** terminal message when batch +processing finishes, but only to clients that are already connected when +the update is emitted. @@ - The server sends exactly one message per connection, always with a - terminal status (``"completed"`` or ``"failed"``). The client should - close the connection after receiving it. + The server does not replay the last known status on connect. Clients + that subscribe after processing has already finished may receive no + WebSocket message and should fall back to the REST API or a client- + side timeout. @@ -3. When batch processing finishes, the server sends one - ``batch_status_update`` message containing the terminal status. +3. When batch processing finishes, the server sends one + ``batch_status_update`` message to clients that are already subscribed.Also applies to: 123-127, 132-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/websocket-api.rst` around lines 91 - 93, The docs currently claim the server pushes "exactly one" message per connection; update the text to clarify that the consumer emits a single group_send when processing finishes but it does not replay current batch state on new connections, so a client that connects after that terminal group_send may receive zero messages; revise all occurrences (around the paragraphs referencing consumer and group_send) to state "at most one message if the client was connected when the terminal group_send was emitted" and mention that late subscribers will not receive past terminal events.
179-180:⚠️ Potential issue | 🟡 MinorClarify that
daphneapp ordering is for developmentrunserver, not production ASGI selection.Putting
"daphne"first inINSTALLED_APPSaffects Daphne's Djangorunserverintegration; production ASGI deployment depends on the server process you run, not app ordering. The current wording conflates those two concerns.According to the official Django and Channels documentation, why should "daphne" be added first in INSTALLED_APPS, and does that affect production ASGI deployment or only Daphne's runserver integration?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/websocket-api.rst` around lines 179 - 180, The documentation currently implies adding "daphne" first in INSTALLED_APPS selects Daphne as the ASGI server; update the text to clarify that placing "daphne" first in INSTALLED_APPS is only necessary for Django's development runserver integration (so Daphne's runserver command/overrides take precedence), and does not determine which ASGI server is used in production — production ASGI deployment is chosen by the server process you run (e.g., daphne, uvicorn) and not by INSTALLED_APPS ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/websocket-api.rst`:
- Around line 47-57: The docs state org-scoped access control for websocket
batch access, but openwisp_radius/consumers.py's _user_can_access_batch()
currently discards the QuerySet.exists() result and ends up returning True for
non-superusers, so the code does not actually enforce batch ownership; fix
_user_can_access_batch() to properly evaluate and return the boolean from the
ownership check (ensure the .exists() or equivalent filter result is returned
and used in the authorization decision) or, if you prefer a docs change instead,
soften the wording in docs to remove the assertion that organization manager
ownership is enforced until _user_can_access_batch() correctly checks and
returns the ownership boolean.
---
Duplicate comments:
In `@docs/user/websocket-api.rst`:
- Around line 91-93: The docs currently claim the server pushes "exactly one"
message per connection; update the text to clarify that the consumer emits a
single group_send when processing finishes but it does not replay current batch
state on new connections, so a client that connects after that terminal
group_send may receive zero messages; revise all occurrences (around the
paragraphs referencing consumer and group_send) to state "at most one message if
the client was connected when the terminal group_send was emitted" and mention
that late subscribers will not receive past terminal events.
- Around line 179-180: The documentation currently implies adding "daphne" first
in INSTALLED_APPS selects Daphne as the ASGI server; update the text to clarify
that placing "daphne" first in INSTALLED_APPS is only necessary for Django's
development runserver integration (so Daphne's runserver command/overrides take
precedence), and does not determine which ASGI server is used in production —
production ASGI deployment is chosen by the server process you run (e.g.,
daphne, uvicorn) and not by INSTALLED_APPS ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c44fee1-8aaa-468f-b63b-f9be2822d829
📒 Files selected for processing (4)
docs/user/generating_users.rstdocs/user/importing_users.rstdocs/user/settings.rstdocs/user/websocket-api.rst
| A user is authorized if: | ||
|
|
||
| - The user is a superuser, OR | ||
| - The user: | ||
|
|
||
| - Is authenticated and marked as staff, AND | ||
| - Is an organization manager for the organization that owns the | ||
| requested batch. | ||
|
|
||
| If any check fails, the server closes the connection without sending any | ||
| message. |
There was a problem hiding this comment.
Don't document org-scoped access control unless the consumer actually enforces it.
openwisp_radius/consumers.py currently discards the .exists() result in _user_can_access_batch() and then returns True for non-superusers, so this section overstates the authorization guarantee. Please either fix the consumer before merging these docs or soften the wording until batch ownership is really checked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/user/websocket-api.rst` around lines 47 - 57, The docs state org-scoped
access control for websocket batch access, but openwisp_radius/consumers.py's
_user_can_access_batch() currently discards the QuerySet.exists() result and
ends up returning True for non-superusers, so the code does not actually enforce
batch ownership; fix _user_can_access_batch() to properly evaluate and return
the boolean from the ownership check (ensure the .exists() or equivalent filter
result is returned and used in the authorization decision) or, if you prefer a
docs change instead, soften the wording in docs to remove the assertion that
organization manager ownership is enforced until _user_can_access_batch()
correctly checks and returns the ownership boolean.
9b9c193 to
b887625
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/user/websocket-api.rst (3)
91-137:⚠️ Potential issue | 🟠 MajorAvoid guaranteeing one terminal message per connection.
The consumer only forwards the single
group_sendemitted when processing finishes; it does not replay the last-known batch state on connect. A client that subscribes after that terminal event has already been published may receive zero messages, so this section should say “at most one terminal update” and recommend a REST fallback or timeout for late subscribers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/websocket-api.rst` around lines 91 - 137, Update the documentation to avoid promising exactly one terminal message per WebSocket connection: change wording in the note and the Connection Lifecycle steps (references: message type "batch_status_update" and channel group "radius_batch_<batch-id>") to state that the server will send at most one terminal update (i.e., clients may receive zero or one terminal message) and recommend a REST fallback or client-side timeout/retry for late subscribers; also adjust the sentence in step 4 (closing on "completed") to reflect the conditional nature (close if/when a terminal message is received).
47-57:⚠️ Potential issue | 🔴 CriticalDon't document org-scoped batch access until the consumer enforces it.
openwisp_radius/consumers.py:1-47still returnsTruefor non-superusers after calling.exists()without using the result, so the current implementation does not reliably enforce “organization manager for the organization that owns the requested batch”. Either fixRadiusBatchConsumer._user_can_access_batch()first or soften this section to avoid promising an authorization check that is not actually applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/websocket-api.rst` around lines 47 - 57, The docs claim org-scoped batch access is enforced but RadiusBatchConsumer._user_can_access_batch in openwisp_radius/consumers.py currently calls .exists() and then still returns True for non-superusers; fix _user_can_access_batch so it returns the actual boolean result of the queryset.exists() check (or uses the queryset truthiness) instead of ignoring it, ensuring non-superusers only get True when they are authenticated staff and an organization manager for the batch owner; update the implementation in RadiusBatchConsumer._user_can_access_batch to short-circuit for superusers and otherwise return the exists() value (or equivalent) so the authorization described in websocket-api.rst is actually enforced.
179-180:⚠️ Potential issue | 🟡 MinorClarify that
daphnefirst inINSTALLED_APPSis forrunserver, not production server selection.This wording is misleading: putting
daphnefirst inINSTALLED_APPSis for Daphne’s Djangorunserverintegration, not what makes production “use it as the ASGI server”. Production ASGI deployment runs the server directly, so the docs should separate local development setup from production deployment.According to the official Django and Channels documentation, when and why should "daphne" be added as the first entry in INSTALLED_APPS? Is that required for production ASGI deployment, or mainly for Daphne's runserver integration?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user/websocket-api.rst` around lines 179 - 180, Update the sentence to clarify that placing "daphne" first in INSTALLED_APPS is only required to enable Daphne’s runserver integration for local development (so Django's manage.py runserver uses Daphne), and is NOT how you select the ASGI server in production — in production you start Daphne (or Uvicorn/Hypercorn) directly as the ASGI server. Replace the current line with two brief lines separating development (add "daphne" first in INSTALLED_APPS for runserver integration) and production (run the chosen ASGI server directly), and mention "daphne", "INSTALLED_APPS", and "runserver" so readers can find the exact settings to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/user/websocket-api.rst`:
- Around line 91-137: Update the documentation to avoid promising exactly one
terminal message per WebSocket connection: change wording in the note and the
Connection Lifecycle steps (references: message type "batch_status_update" and
channel group "radius_batch_<batch-id>") to state that the server will send at
most one terminal update (i.e., clients may receive zero or one terminal
message) and recommend a REST fallback or client-side timeout/retry for late
subscribers; also adjust the sentence in step 4 (closing on "completed") to
reflect the conditional nature (close if/when a terminal message is received).
- Around line 47-57: The docs claim org-scoped batch access is enforced but
RadiusBatchConsumer._user_can_access_batch in openwisp_radius/consumers.py
currently calls .exists() and then still returns True for non-superusers; fix
_user_can_access_batch so it returns the actual boolean result of the
queryset.exists() check (or uses the queryset truthiness) instead of ignoring
it, ensuring non-superusers only get True when they are authenticated staff and
an organization manager for the batch owner; update the implementation in
RadiusBatchConsumer._user_can_access_batch to short-circuit for superusers and
otherwise return the exists() value (or equivalent) so the authorization
described in websocket-api.rst is actually enforced.
- Around line 179-180: Update the sentence to clarify that placing "daphne"
first in INSTALLED_APPS is only required to enable Daphne’s runserver
integration for local development (so Django's manage.py runserver uses Daphne),
and is NOT how you select the ASGI server in production — in production you
start Daphne (or Uvicorn/Hypercorn) directly as the ASGI server. Replace the
current line with two brief lines separating development (add "daphne" first in
INSTALLED_APPS for runserver integration) and production (run the chosen ASGI
server directly), and mention "daphne", "INSTALLED_APPS", and "runserver" so
readers can find the exact settings to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a298c1da-9f66-48a3-890f-3674d7cd0a53
📒 Files selected for processing (4)
docs/user/generating_users.rstdocs/user/importing_users.rstdocs/user/settings.rstdocs/user/websocket-api.rst
- add docs/user/websocket-api.rst (endpoint URL, auth, message types, JS example, deployment setup) - register new page in docs/index.rst toctree - add WebSocket cross-references to generating_users.rst and importing_users.rst - add missing label + note to OPENWISP_RADIUS_BATCH_ASYNC_THRESHOLD in settings.rst Closes openwisp#682
iamavichal-geek
force-pushed
the
docs/websocket-api
branch
from
b887625 to
8585c2b
Compare
|
Hi @nemesifier, I've opened this PR to address #682. Modelled the structure after |
2 participants
Closes #682
Changes:
docs/user/websocket-api.rst— new page documenting the RadiusBatchWebSocket endpoint (modelled after openwisp-firmware-upgrader)
docs/index.rst— registered in toctreedocs/user/generating_users.rst— added WebSocket cross-referencedocs/user/importing_users.rst— added WebSocket cross-referencedocs/user/settings.rst— added label + note toOPENWISP_RADIUS_BATCH_ASYNC_THRESHOLD