-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Revert "fix(api): authorize owner_ids for list chats and search apps (#14775) #15698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -84,31 +84,15 @@ def list_searches(): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| owner_ids = request.args.getlist("owner_ids") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tenants = TenantService.get_joined_tenants_by_user_id(current_user.id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authorized_owner_ids = {member["tenant_id"] for member in tenants} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| authorized_owner_ids.add(current_user.id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if owner_ids: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| requested_owner_ids = set(owner_ids) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unauthorized_owner_ids = requested_owner_ids - authorized_owner_ids | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if unauthorized_owner_ids: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logging.warning( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Rejected list_searches request: user=%s attempted unauthorized owner_ids=%s", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| current_user.id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sorted(unauthorized_owner_ids), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return get_json_result( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| data=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| message="Only authorized owner_ids can be queried.", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| code=RetCode.OPERATING_ERROR, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| effective_owner_ids = list(requested_owner_ids) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not owner_ids: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tenants = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| search_apps, total = SearchService.get_by_tenant_ids(tenants, current_user.id, page_number, items_per_page, orderby, desc, keywords) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| effective_owner_ids = list(authorized_owner_ids) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| search_apps, total = SearchService.get_by_tenant_ids( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| effective_owner_ids, current_user.id, page_number, items_per_page, orderby, desc, keywords | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| search_apps, total = SearchService.get_by_tenant_ids(owner_ids, current_user.id, 0, 0, orderby, desc, keywords) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| search_apps = [s for s in search_apps if s["tenant_id"] in owner_ids] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| total = len(search_apps) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if page_number and items_per_page: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| search_apps = search_apps[(page_number - 1) * items_per_page: page_number * items_per_page] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+87
to
+95
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not treat request
🔒 Proposed fix try:
- if not owner_ids:
- tenants = []
- search_apps, total = SearchService.get_by_tenant_ids(tenants, current_user.id, page_number, items_per_page, orderby, desc, keywords)
- else:
- search_apps, total = SearchService.get_by_tenant_ids(owner_ids, current_user.id, 0, 0, orderby, desc, keywords)
- search_apps = [s for s in search_apps if s["tenant_id"] in owner_ids]
- total = len(search_apps)
- if page_number and items_per_page:
- search_apps = search_apps[(page_number - 1) * items_per_page: page_number * items_per_page]
+ joined_tenants = UserTenantService.query(user_id=current_user.id)
+ authorized_owner_ids = {current_user.id}
+ authorized_owner_ids.update(tenant.tenant_id for tenant in joined_tenants)
+ if owner_ids:
+ unauthorized_owner_ids = sorted(set(owner_ids) - authorized_owner_ids)
+ if unauthorized_owner_ids:
+ logging.warning(
+ "Rejecting unauthorized owner_ids on list_searches: user_id=%s owner_ids=%s",
+ current_user.id,
+ unauthorized_owner_ids,
+ )
+ return get_json_result(
+ data=False,
+ message="Please specify authorized owner_ids only.",
+ code=RetCode.OPERATING_ERROR,
+ )
+ effective_owner_ids = owner_ids
+ else:
+ effective_owner_ids = list(authorized_owner_ids)
+
+ search_apps, total = SearchService.get_by_tenant_ids(
+ effective_owner_ids,
+ current_user.id,
+ page_number,
+ items_per_page,
+ orderby,
+ desc,
+ keywords,
+ )
return get_json_result(data={"search_apps": search_apps, "total": total})As per coding guidelines, "Add logging for new flows". 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return get_json_result(data={"search_apps": search_apps, "total": total}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return server_error_response(e) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1251,7 +1251,7 @@ def _save(**kwargs): | |
|
|
||
|
|
||
| @pytest.mark.p2 | ||
| def test_list_chats_defaults_to_authorized_owner_ids_when_omitted_unit(monkeypatch): | ||
| def test_list_chats_passes_empty_owner_ids_when_omitted_unit(monkeypatch): | ||
| module = _load_chat_routes_unit_module(monkeypatch) | ||
| captured = {} | ||
| monkeypatch.setattr( | ||
|
|
@@ -1280,33 +1280,44 @@ def _get_by_tenant_ids(owner_ids, *_args, **_kwargs): | |
| monkeypatch.setattr(module.DialogService, "get_by_tenant_ids", _get_by_tenant_ids) | ||
| res = _run(module.list_chats.__wrapped__()) | ||
| assert res["code"] == 0 | ||
| assert set(captured["owner_ids"]) == {"tenant-1", "team-tenant-2"} | ||
| assert captured["owner_ids"] == [] | ||
|
|
||
|
|
||
| @pytest.mark.p2 | ||
| def test_list_chats_rejects_unauthorized_owner_ids_unit(monkeypatch): | ||
| def test_list_chats_filters_by_requested_owner_ids_unit(monkeypatch): | ||
| module = _load_chat_routes_unit_module(monkeypatch) | ||
| captured = {} | ||
| monkeypatch.setattr( | ||
| module, | ||
| "request", | ||
| SimpleNamespace( | ||
| args=SimpleNamespace( | ||
| get=lambda key, default=None: { | ||
| "keywords": "", | ||
| "page": "0", | ||
| "page_size": "0", | ||
| "page": "1", | ||
| "page_size": "10", | ||
| "orderby": "create_time", | ||
| "desc": "true", | ||
| "id": None, | ||
| "name": None, | ||
| }.get(key, default), | ||
| getlist=lambda key: ["foreign-tenant-id"] if key == "owner_ids" else [], | ||
| getlist=lambda key: ["team-tenant-2"] if key == "owner_ids" else [], | ||
| ) | ||
| ), | ||
| ) | ||
|
|
||
| def _get_by_tenant_ids(owner_ids, *_args, **_kwargs): | ||
| captured["owner_ids"] = owner_ids | ||
| team_chat = _DummyDialogRecord({"id": "team-chat", "tenant_id": "team-tenant-2", "name": "team"}).to_dict() | ||
| own_chat = _DummyDialogRecord({"id": "own-chat", "tenant_id": "tenant-1", "name": "own"}).to_dict() | ||
| return ([team_chat, own_chat], 2) | ||
|
|
||
| monkeypatch.setattr(module.DialogService, "get_by_tenant_ids", _get_by_tenant_ids) | ||
| res = _run(module.list_chats.__wrapped__()) | ||
| assert res["code"] == module.RetCode.OPERATING_ERROR | ||
| assert "authorized owner_ids" in res["message"] | ||
| assert res["code"] == 0 | ||
| assert captured["owner_ids"] == ["team-tenant-2"] | ||
| assert [chat["id"] for chat in res["data"]["chats"]] == ["team-chat"] | ||
| assert res["data"]["total"] == 1 | ||
|
Comment on lines
+1287
to
+1320
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cover the new When Suggested test tightening get=lambda key, default=None: {
"keywords": "",
- "page": "1",
- "page_size": "10",
+ "page": "2",
+ "page_size": "1",
"orderby": "create_time",
"desc": "true",
"id": None,
"name": None,
}.get(key, default),
getlist=lambda key: ["team-tenant-2"] if key == "owner_ids" else [],
)
),
)
- def _get_by_tenant_ids(owner_ids, *_args, **_kwargs):
+ def _get_by_tenant_ids(owner_ids, _user_id, page_number, items_per_page, *_args, **_kwargs):
captured["owner_ids"] = owner_ids
- team_chat = _DummyDialogRecord({"id": "team-chat", "tenant_id": "team-tenant-2", "name": "team"}).to_dict()
+ captured["page_number"] = page_number
+ captured["items_per_page"] = items_per_page
+ team_chat_1 = _DummyDialogRecord({"id": "team-chat-1", "tenant_id": "team-tenant-2", "name": "team-1"}).to_dict()
own_chat = _DummyDialogRecord({"id": "own-chat", "tenant_id": "tenant-1", "name": "own"}).to_dict()
- return ([team_chat, own_chat], 2)
+ team_chat_2 = _DummyDialogRecord({"id": "team-chat-2", "tenant_id": "team-tenant-2", "name": "team-2"}).to_dict()
+ return ([team_chat_1, own_chat, team_chat_2], 3)
monkeypatch.setattr(module.DialogService, "get_by_tenant_ids", _get_by_tenant_ids)
res = _run(module.list_chats.__wrapped__())
assert res["code"] == 0
assert captured["owner_ids"] == ["team-tenant-2"]
- assert [chat["id"] for chat in res["data"]["chats"]] == ["team-chat"]
- assert res["data"]["total"] == 1
+ assert captured["page_number"] == 0
+ assert captured["items_per_page"] == 0
+ assert [chat["id"] for chat in res["data"]["chats"]] == ["team-chat-2"]
+ assert res["data"]["total"] == 2As per coding guidelines, "Add/adjust tests for behavior changes." 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| @pytest.mark.p2 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore server-side owner authorization before querying.
DialogService.get_by_tenant_ids()treats its first argument as the authorized tenant scope (tenant_id IN joined_tenant_ids OR tenant_id == user_idinapi/db/services/dialog_service.py:194-255). Passing request-suppliedowner_idsstraight through lets any authenticated caller enumerate chats for arbitrary tenants, and theelsebranch now regresses the default listing to onlycurrent_user.idchats even thoughget_chatstill authorizes acrossUserTenantService.query()at Lines 489-497. Recompute the caller's joined-tenant set first, reject out-of-scopeowner_ids, and use that authorized set for both filtered and unfiltered listing.🔒 Proposed fix
try: page_number = int(request.args.get("page", 0)) items_per_page = validate_rest_api_page_size(int(request.args.get("page_size", 0))) + joined_tenants = await thread_pool_exec(UserTenantService.query, user_id=current_user.id) + authorized_owner_ids = {current_user.id} + authorized_owner_ids.update(tenant.tenant_id for tenant in joined_tenants) if owner_ids: - chats, total = await thread_pool_exec( - DialogService.get_by_tenant_ids, - owner_ids, current_user.id, 0, 0, orderby, desc, keywords, **exact_filters, - ) - chats = [chat for chat in chats if chat["tenant_id"] in owner_ids] - total = len(chats) - if page_number and items_per_page: - start = (page_number - 1) * items_per_page - chats = chats[start : start + items_per_page] + unauthorized_owner_ids = sorted(set(owner_ids) - authorized_owner_ids) + if unauthorized_owner_ids: + logging.warning( + "Rejecting unauthorized owner_ids on list_chats: user_id=%s owner_ids=%s", + current_user.id, + unauthorized_owner_ids, + ) + return get_json_result( + data=False, + message="Please specify authorized owner_ids only.", + code=RetCode.OPERATING_ERROR, + ) + effective_owner_ids = owner_ids else: - chats, total = await thread_pool_exec( - DialogService.get_by_tenant_ids, - [], current_user.id, page_number, items_per_page, orderby, desc, keywords, **exact_filters, - ) + effective_owner_ids = list(authorized_owner_ids) + + chats, total = await thread_pool_exec( + DialogService.get_by_tenant_ids, + effective_owner_ids, + current_user.id, + page_number, + items_per_page, + orderby, + desc, + keywords, + **exact_filters, + )As per coding guidelines, "Add logging for new flows".
🤖 Prompt for AI Agents