Refactor: Cleanup legacy MQTT, username, and AGENT_USER_ID#131
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors legacy user-identification and MQTT behavior to remove AGENT_USER_ID and username usage, enforces email-based identity and strict multi-tenant behavior, and updates tests, templates, and docs accordingly. Sequence diagram for onSync user resolution without AGENT_USER_ID fallbacksequenceDiagram
actor Client
participant Routes as Routes_bp
participant ActionDevices as ActionDevices_onSync
participant UserScope as NormalizeUserScope
Client->>Routes: POST /sync
Routes->>UserScope: _resolve_smarthome_user_scope
UserScope-->>Routes: user_id None when no session
Routes->>ActionDevices: onSync(user_id None, agent_user_id None)
ActionDevices->>UserScope: _normalize_user_scope(user_id)
UserScope-->>ActionDevices: resolved_user None
ActionDevices->>ActionDevices: agent_user = str(agent_user_id or resolved_user)
ActionDevices-->>Client: agentUserId "unknown" on error
note over ActionDevices,Routes: Config AGENT_USER_ID is not used as fallback anymore
ER diagram for updated User table without usernameerDiagram
USER {
int id PK
varchar email
varchar password
}
OAUTH_TOKEN {
int id PK
int user_id FK
varchar access_token
varchar refresh_token
}
USER ||--o{ OAUTH_TOKEN : has
Class diagram for updated user and configuration modelsclassDiagram
class User {
+Integer id
+String email
+String password
}
class Config {
+Bool MQTT_TLS_ENABLED
+Int MQTT_TLS_VERSION
+String API_KEY
+String DATABASEURL
}
class OAuthToken {
+Integer id
+Integer user_id
+String access_token
+String refresh_token
}
User "1" -- "many" OAuthToken : owns
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- You now return
(None, None)from_resolve_smarthome_user_scopewhere a concrete fallback was previously guaranteed; consider double-checking all call sites to avoid accidentally propagatingNoneinto places that expect a non-emptyuser_idoragent_user_idstring. - The literal string "unknown" is now used in multiple places for
agentUserId; consider centralizing this into a constant or helper to keep behavior consistent and easier to change later. - In
onSync, when bothresolved_userandagent_user_idareNone,agent_userbecomes the string'None'whereas error handling returns'unknown'; you might want to align these code paths to avoid inconsistent identifiers for the same situation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You now return `(None, None)` from `_resolve_smarthome_user_scope` where a concrete fallback was previously guaranteed; consider double-checking all call sites to avoid accidentally propagating `None` into places that expect a non-empty `user_id` or `agent_user_id` string.
- The literal string "unknown" is now used in multiple places for `agentUserId`; consider centralizing this into a constant or helper to keep behavior consistent and easier to change later.
- In `onSync`, when both `resolved_user` and `agent_user_id` are `None`, `agent_user` becomes the string `'None'` whereas error handling returns `'unknown'`; you might want to align these code paths to avoid inconsistent identifiers for the same situation.
## Individual Comments
### Comment 1
<location path="action_devices.py" line_range="184" />
<code_context>
resolved_user = _normalize_user_scope(user_id)
- fallback = resolved_user or current_app.config.get('AGENT_USER_ID', 'test-user')
- agent_user = str(agent_user_id if agent_user_id is not None else fallback)
+ agent_user = str(agent_user_id if agent_user_id is not None else resolved_user)
return {
"agentUserId": agent_user,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid returning the literal string 'None' as agentUserId when no user is resolved.
With this change, if both `agent_user_id` and `resolved_user` are `None`, `agent_user` becomes the literal string `'None'`. That’s inconsistent with the error path (which uses `'unknown'`) and may confuse downstream consumers. Consider falling back to a stable value like `'unknown'` here (e.g. `resolved_user or 'unknown'`).
</issue_to_address>
### Comment 2
<location path="tests/test_app.py" line_range="304-306" />
<code_context>
self.assertEqual(me_resp.status_code, 200)
me_payload = me_resp.get_json()
self.assertIsInstance(me_payload, dict)
- self.assertEqual(me_payload.get('username'), 'oauth-user')
+ self.assertEqual(me_payload.get('email'), 'oauth@example.com')
def test_authorize_deny_consent_redirects_with_access_denied(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the `/me` endpoint test to assert the full response shape, not just the email value.
Instead of checking only the `email` field, assert the full payload matches the expected schema, e.g. `self.assertEqual(me_payload, {"email": "oauth@example.com"})`. This will ensure `username` is absent and prevent unnoticed additions of unexpected fields in future changes.
```suggestion
self.assertEqual(me_resp.status_code, 200)
me_payload = me_resp.get_json()
self.assertIsInstance(me_payload, dict)
self.assertEqual(me_payload, {"email": "oauth@example.com"})
```
</issue_to_address>
### Comment 3
<location path="tests/test_multi_tenant.py" line_range="17" />
<code_context>
class MultiTenantTest(unittest.TestCase):
def setUp(self):
self.app = Flask(__name__)
- self.app.config['AGENT_USER_ID'] = 'test-fallback-user'
self.ctx = self.app.app_context()
self.ctx.push()
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for behavior when no `AGENT_USER_ID` fallback is configured.
Since `_resolve_smarthome_user_scope` no longer uses `AGENT_USER_ID`, multi-tenant behavior now depends only on the request `agentUserId` and the authenticated user. Please add tests for the "no fallback" scenarios (e.g., no `agentUserId` and no authenticated user) across sync/query/execute flows and any helpers that resolve user scope, to lock in and document the new default behavior and prevent regressions if a fallback is reintroduced.
Suggested implementation:
```python
import os
os.environ['SQLALCHEMY_DATABASE_URI'] = 'sqlite:///:memory:'
os.environ['APP_ENV'] = 'testing'
import unittest
from flask import Flask
from action_devices import onSync, actions
from notifications import handle_messages, get_mqtt_logs, _append_mqtt_log
class MultiTenantTest(unittest.TestCase):
def setUp(self):
self.app = Flask(__name__)
# Explicitly ensure no AGENT_USER_ID fallback is configured for these tests
self.app.config.pop('AGENT_USER_ID', None)
self.ctx = self.app.app_context()
self.ctx.push()
@unittest.skip("Define expectations for sync behavior when no agentUserId and no authenticated user, then remove this skip.")
def test_sync_without_fallback_no_agent_or_user(self):
"""Explicitly document behavior when there is no AGENT_USER_ID, no agentUserId, and no authenticated user on SYNC."""
# TODO: Construct a SYNC request with no agentUserId and no authenticated user context
# and assert the resulting behavior (e.g., error, empty device list, etc.)
self.fail("Implement SYNC no-fallback/no-user behavior assertion")
@unittest.skip("Define expectations for query behavior when no agentUserId and no authenticated user, then remove this skip.")
def test_query_without_fallback_no_agent_or_user(self):
"""Explicitly document behavior when there is no AGENT_USER_ID, no agentUserId, and no authenticated user on QUERY."""
# TODO: Construct a QUERY request with no agentUserId and no authenticated user context
# and assert the resulting behavior (e.g., error response, empty states, etc.)
self.fail("Implement QUERY no-fallback/no-user behavior assertion")
@unittest.skip("Define expectations for execute behavior when no agentUserId and no authenticated user, then remove this skip.")
def test_execute_without_fallback_no_agent_or_user(self):
"""Explicitly document behavior when there is no AGENT_USER_ID, no agentUserId, and no authenticated user on EXECUTE."""
# TODO: Construct an EXECUTE request with no agentUserId and no authenticated user context
# and assert the resulting behavior (e.g., error response, no side effects, etc.)
self.fail("Implement EXECUTE no-fallback/no-user behavior assertion")
@unittest.skip("Wire this test to the helper that resolves user scope and assert its behavior with no fallback.")
def test_user_scope_resolution_without_fallback(self):
"""Lock in behavior of whatever helper resolves smarthome user scope when no AGENT_USER_ID, no agentUserId, and no authenticated user."""
# TODO: Call the helper used by SYNC/QUERY/EXECUTE to resolve user scope and
# assert the expected behavior (e.g., returns None/raises, etc.) in the no-fallback case.
self.fail("Implement user-scope helper no-fallback/no-user behavior assertion")
```
To fully implement the requested tests and remove the `@unittest.skip` decorators and `self.fail` placeholders, you will need to:
1. Mirror the patterns used in your existing multi-tenant tests (likely in this same file) for constructing SYNC/QUERY/EXECUTE requests:
- Reuse the same helpers or factories that build request payloads and contexts.
- Ensure *no* `agentUserId` is included in the request payload.
- Ensure there is *no* authenticated user attached (e.g., no session, no JWT, no user object in the context).
2. For `test_sync_without_fallback_no_agent_or_user`:
- Call into the same entry point your existing SYNC tests use (e.g., a Flask test client hitting the SYNC endpoint or direct invocation of `onSync`/`actions.onSync` with the appropriate arguments).
- Assert the exact expected behavior in the no-fallback/no-user case (for example: HTTP 401/403, specific error payload, or an empty device list), and replace the `self.fail` with those assertions.
3. For `test_query_without_fallback_no_agent_or_user` and `test_execute_without_fallback_no_agent_or_user`:
- Use the same entry points used by your current QUERY/EXECUTE tests (e.g., `actions.onQuery`, `actions.onExecute`, or HTTP endpoints).
- Assert the exact expected behavior (e.g., error code, empty response, or specific error structure) when no user scope can be resolved.
4. For `test_user_scope_resolution_without_fallback`:
- Import and call the helper that replaced `_resolve_smarthome_user_scope` (or `_resolve_smarthome_user_scope` if it still exists) that is responsible for resolving the user scope used by SYNC/QUERY/EXECUTE.
- Construct its inputs to represent "no `agentUserId` and no authenticated user" and assert its behavior (e.g., returns `None`, raises a specific exception, or produces a special sentinel).
- Update the test body to call this helper and assert on its return/exception instead of `self.fail`.
Once you wire these tests to the actual request/handler/helper APIs and define the assertions that match your intended "no fallback" default behavior, you can remove the `@unittest.skip` decorators so these tests actively lock in that behavior and prevent regressions if a fallback is reintroduced.
</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
304
to
306
| self.assertEqual(me_resp.status_code, 200) | ||
| me_payload = me_resp.get_json() | ||
| self.assertIsInstance(me_payload, dict) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen the /me endpoint test to assert the full response shape, not just the email value.
Instead of checking only the email field, assert the full payload matches the expected schema, e.g. self.assertEqual(me_payload, {"email": "oauth@example.com"}). This will ensure username is absent and prevent unnoticed additions of unexpected fields in future changes.
Suggested change
| self.assertEqual(me_resp.status_code, 200) | |
| me_payload = me_resp.get_json() | |
| self.assertIsInstance(me_payload, dict) | |
| self.assertEqual(me_resp.status_code, 200) | |
| me_payload = me_resp.get_json() | |
| self.assertIsInstance(me_payload, dict) | |
| self.assertEqual(me_payload, {"email": "oauth@example.com"}) |
| class MultiTenantTest(unittest.TestCase): | ||
| def setUp(self): | ||
| self.app = Flask(__name__) | ||
| self.app.config['AGENT_USER_ID'] = 'test-fallback-user' |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add explicit tests for behavior when no AGENT_USER_ID fallback is configured.
Since _resolve_smarthome_user_scope no longer uses AGENT_USER_ID, multi-tenant behavior now depends only on the request agentUserId and the authenticated user. Please add tests for the "no fallback" scenarios (e.g., no agentUserId and no authenticated user) across sync/query/execute flows and any helpers that resolve user scope, to lock in and document the new default behavior and prevent regressions if a fallback is reintroduced.
Suggested implementation:
import os
os.environ['SQLALCHEMY_DATABASE_URI'] = 'sqlite:///:memory:'
os.environ['APP_ENV'] = 'testing'
import unittest
from flask import Flask
from action_devices import onSync, actions
from notifications import handle_messages, get_mqtt_logs, _append_mqtt_log
class MultiTenantTest(unittest.TestCase):
def setUp(self):
self.app = Flask(__name__)
# Explicitly ensure no AGENT_USER_ID fallback is configured for these tests
self.app.config.pop('AGENT_USER_ID', None)
self.ctx = self.app.app_context()
self.ctx.push()
@unittest.skip("Define expectations for sync behavior when no agentUserId and no authenticated user, then remove this skip.")
def test_sync_without_fallback_no_agent_or_user(self):
"""Explicitly document behavior when there is no AGENT_USER_ID, no agentUserId, and no authenticated user on SYNC."""
# TODO: Construct a SYNC request with no agentUserId and no authenticated user context
# and assert the resulting behavior (e.g., error, empty device list, etc.)
self.fail("Implement SYNC no-fallback/no-user behavior assertion")
@unittest.skip("Define expectations for query behavior when no agentUserId and no authenticated user, then remove this skip.")
def test_query_without_fallback_no_agent_or_user(self):
"""Explicitly document behavior when there is no AGENT_USER_ID, no agentUserId, and no authenticated user on QUERY."""
# TODO: Construct a QUERY request with no agentUserId and no authenticated user context
# and assert the resulting behavior (e.g., error response, empty states, etc.)
self.fail("Implement QUERY no-fallback/no-user behavior assertion")
@unittest.skip("Define expectations for execute behavior when no agentUserId and no authenticated user, then remove this skip.")
def test_execute_without_fallback_no_agent_or_user(self):
"""Explicitly document behavior when there is no AGENT_USER_ID, no agentUserId, and no authenticated user on EXECUTE."""
# TODO: Construct an EXECUTE request with no agentUserId and no authenticated user context
# and assert the resulting behavior (e.g., error response, no side effects, etc.)
self.fail("Implement EXECUTE no-fallback/no-user behavior assertion")
@unittest.skip("Wire this test to the helper that resolves user scope and assert its behavior with no fallback.")
def test_user_scope_resolution_without_fallback(self):
"""Lock in behavior of whatever helper resolves smarthome user scope when no AGENT_USER_ID, no agentUserId, and no authenticated user."""
# TODO: Call the helper used by SYNC/QUERY/EXECUTE to resolve user scope and
# assert the expected behavior (e.g., returns None/raises, etc.) in the no-fallback case.
self.fail("Implement user-scope helper no-fallback/no-user behavior assertion")To fully implement the requested tests and remove the @unittest.skip decorators and self.fail placeholders, you will need to:
-
Mirror the patterns used in your existing multi-tenant tests (likely in this same file) for constructing SYNC/QUERY/EXECUTE requests:
- Reuse the same helpers or factories that build request payloads and contexts.
- Ensure no
agentUserIdis included in the request payload. - Ensure there is no authenticated user attached (e.g., no session, no JWT, no user object in the context).
-
For
test_sync_without_fallback_no_agent_or_user:- Call into the same entry point your existing SYNC tests use (e.g., a Flask test client hitting the SYNC endpoint or direct invocation of
onSync/actions.onSyncwith the appropriate arguments). - Assert the exact expected behavior in the no-fallback/no-user case (for example: HTTP 401/403, specific error payload, or an empty device list), and replace the
self.failwith those assertions.
- Call into the same entry point your existing SYNC tests use (e.g., a Flask test client hitting the SYNC endpoint or direct invocation of
-
For
test_query_without_fallback_no_agent_or_userandtest_execute_without_fallback_no_agent_or_user:- Use the same entry points used by your current QUERY/EXECUTE tests (e.g.,
actions.onQuery,actions.onExecute, or HTTP endpoints). - Assert the exact expected behavior (e.g., error code, empty response, or specific error structure) when no user scope can be resolved.
- Use the same entry points used by your current QUERY/EXECUTE tests (e.g.,
-
For
test_user_scope_resolution_without_fallback:- Import and call the helper that replaced
_resolve_smarthome_user_scope(or_resolve_smarthome_user_scopeif it still exists) that is responsible for resolving the user scope used by SYNC/QUERY/EXECUTE. - Construct its inputs to represent "no
agentUserIdand no authenticated user" and assert its behavior (e.g., returnsNone, raises a specific exception, or produces a special sentinel). - Update the test body to call this helper and assert on its return/exception instead of
self.fail.
- Import and call the helper that replaced
Once you wire these tests to the actual request/handler/helper APIs and define the assertions that match your intended "no fallback" default behavior, you can remove the @unittest.skip decorators so these tests actively lock in that behavior and prevent regressions if a fallback is reintroduced.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes the legacy AGENT_USER_ID configuration and the username field from the User model, transitioning to a session-based or explicit user identification system. It updates MQTT subscriptions to remove legacy topics and modifies the /me endpoint and authorization templates to use email instead of username. A review comment suggests ensuring consistency in fallback values for agentUserId in action_devices.py to avoid using the string "None" when identifiers are missing.
| resolved_user = _normalize_user_scope(user_id) | ||
| fallback = resolved_user or current_app.config.get('AGENT_USER_ID', 'test-user') | ||
| agent_user = str(agent_user_id if agent_user_id is not None else fallback) | ||
| agent_user = str(agent_user_id if agent_user_id is not None else resolved_user) |
Contributor
There was a problem hiding this comment.
When both agent_user_id and resolved_user are None, agent_user will be assigned the string value "None" due to the str() conversion. This is inconsistent with the error handling in this file (which uses "unknown" on line 191) and the report_state implementation (which uses "unknown" on line 337). Using "None" as an identifier may also cause issues with the Smart Home graph. Consider using a consistent fallback value.
Suggested change
| agent_user = str(agent_user_id if agent_user_id is not None else resolved_user) | |
| agent_user = str(agent_user_id if agent_user_id is not None else (resolved_user or "unknown")) |
Owner
Author
|
I have addressed all review comments:
|
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.
Overview
This PR removes legacy components identified during the cleanup phase to improve codebase maintainability and ensure strict multi-tenant isolation.
Changes:
+/notification,+/status) in favor of user-separated topics.usernamecolumn from theUsermodel.AGENT_USER_IDfallback logic and logging./api/meto useemailas the primary identifier.action_devices.py.Verification:
make test: All 43 tests passed.Summary by Sourcery
Remove legacy single-tenant fallbacks and identifiers in favor of user-scoped, email-based, multi-tenant behavior.
Enhancements:
/api/meresponse and authorization template to use email as the primary user identifier instead of username.usernamefield from theUsermodel and related test/fixture usage, standardizing on email.Documentation:
Tests:
Chores: