Skip to content

[WIP] Fix convert_called_station_id command to properly convert field#728

Draft
pandafy with Claude wants to merge 2 commits into
masterfrom
claude/bug-fix-convert-called-station-id
Draft

[WIP] Fix convert_called_station_id command to properly convert field#728
pandafy with Claude wants to merge 2 commits into
masterfrom
claude/bug-fix-convert-called-station-id

Conversation

@Claude

@Claude Claude AI commented May 27, 2026

Copy link
Copy Markdown

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.


This section details on the original issue you should resolve

<issue_title>[bug] convert_called_station_id command fails silently, called_station_id remains unconverted</issue_title>
<issue_description>## Describe the bug

The convert_called_station_id management command does not convert the called_station_id
field in RADIUS accounting sessions. The called_station_id remains in its original
unconverted format (e.g., 00:27:22:F3:FA:F1:gw1.openwisp.org) after the command runs.

Root Cause

The file openwisp_radius/management/commands/base/convert_called_station_id.py uses
Exscript.protocols.telnetlib.Telnet as a context manager:

with telnetlib.Telnet(host, port, timeout=TELNET_CONNECTION_TIMEOUT) as tn:

However, Exscript.protocols.telnetlib.Telnet is a custom fork of Python's standard
telnetlib and does not implement the __enter__ and __exit__ dunder methods required
for context manager (with statement) support.

As a result:

  • The Telnet connection to the OpenVPN management interface is never properly established
  • The command exits silently without performing any conversion
  • No exception is raised, making the failure difficult to detect

Note: Python's standard telnetlib.Telnet does support context managers, but
Exscript.protocols.telnetlib does not inherit this behavior as it is a separate
re-implementation.

Steps to Reproduce

  1. Configure OPENWISP_RADIUS_CALLED_STATION_IDS in settings.py:
OPENWISP_RADIUS_CALLED_STATION_IDS = {
    "<organization_uuid>": {
        "openvpn_config": [
            {
                "host": "<openvpn_host>",
                "port": 7505,
            }
        ],
        "unconverted_ids": ["<called_station_id>"],
    }
}
  1. Run the management command:
python manage.py convert_called_station_id
  1. Check radacctcalled_station_id remains unchanged.

Expected Behavior

The command should connect to the OpenVPN management interface via Telnet, retrieve the
correct called station ID, and update the called_station_id field in the RADIUS
accounting records accordingly.

Suggested Fix

Replace the with block with an explicit try/finally to ensure the Telnet connection is
properly opened and closed:

tn = telnetlib.Telnet(host, port, timeout=TELNET_CONNECTION_TIMEOUT)
try:
    # existing logic using tn
finally:
    tn.close()

</issue_description>

<agent_instructions>1. Add a failing test case to ensure that this bug never re-surfaces
2. We need better logging so we can catch this kind of bugs easily.
3. Follow openwisp conventions. Read the ci.yml file to understand how to install the project and run tests. Ensure that you have properly formatted the code, use the helper scripts openwisp-qa-format and ./run-qa-checks to ensure the code is formatted properly.
5. You can also refer to docs/developer/installation.rst</agent_instructions>

Comments on the Issue (you are @claude[agent] in this section)

…ly due to Exscript telnetlib not supporting context managers

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Agent-Logs-Url: https://github.com/openwisp/openwisp-radius/sessions/255f0e6a-1df5-4341-8d61-6d47e27d270c

Co-authored-by: pandafy <32094433+pandafy@users.noreply.github.com>
@Claude
Claude AI requested a review from pandafy May 27, 2026 21:10
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 97.579% (+0.2%) from 97.42% — claude/bug-fix-convert-called-station-id into master

@nemesifier nemesifier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, see my comments below.

# Mock telnetlib.Telnet to verify it's properly opened and closed
from unittest.mock import MagicMock

from Exscript.protocols import telnetlib

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the imports to the top


# Verify that Telnet was instantiated with correct parameters
mock_telnet_class.assert_called_once_with("127.0.0.1", 7505, timeout=30)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove the extra blank lines as usual

with telnetlib.Telnet(host, port, timeout=TELNET_CONNECTION_TIMEOUT) as tn:
tn = telnetlib.Telnet(host, port, timeout=TELNET_CONNECTION_TIMEOUT)
try:
logger.info(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.info(
logger.debug(

)
finally:
tn.close()
logger.info(f"Closed telnet connection to {host}:{port}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.info(f"Closed telnet connection to {host}:{port}")
logger.debug(f"Closed telnet connection to {host}:{port}")

@openwisp-companion

Copy link
Copy Markdown

Hi @claude 👋,

This is a friendly reminder that this pull request has had no activity for 7 days since changes were requested.

We'd love to see this contribution merged! Please take a moment to:

  • Address the review feedback
  • Push your changes
  • Let us know if you have any questions or need clarification

If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it.

Note: within 7 more days, the linked issue will be unassigned to allow other contributors to work on it.

Thank you for your contribution! 🙏

@openwisp-companion

Copy link
Copy Markdown

Hi @claude 👋,

This pull request has been marked as stale due to 14 days of inactivity after changes were requested.

As a result, any linked issues are being unassigned from you so other contributors can pick them up.

However, you can still continue working on this PR! If you push new commits or respond to the review feedback:

  • The issue will be reassigned to you
  • Your contribution is still very welcome

If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] convert_called_station_id command fails silently, called_station_id remains unconverted

4 participants