[18.0] [MIG] ai_oca_mcp#76
Conversation
Currently translated at 100.0% (43 of 43 strings) Translation: ai-16.0/ai-16.0-ai_oca_mcp Translate-URL: https://translation.odoo-community.org/projects/ai-16-0/ai-16-0-ai_oca_mcp/it/
e54dedd to
5ac6fe0
Compare
|
Hello, I've been reviewing this PR and noticed some behavior that might be worth looking into when an MCP key expires. In that case, the exception It seems to be related to line 31 of the file Since I’m not yet a member of the organization, I can’t contribute directly. To avoid holding things up, I’d really appreciate it if you could take a look at it when you have a moment. Of course, if you’d rather wait, that’s no problem: as soon as I have permissions, I’ll be happy to push the change and help out. Thank you very much for your time. |
petrus-v
left a comment
There was a problem hiding this comment.
The authentication mechanisms sees pretty coupled to the mcp endpoint that I've the feeling it will be hard to overload it to implement an other one
| def _hash_key(self, key): | ||
| return sha256(key.encode()).hexdigest() | ||
|
|
||
| @tools.ormcache("key", "security_key") |
There was a problem hiding this comment.
should we invalidate cache when expiration_date change?
| .sudo() | ||
| ._get_mcp_server_by_key(key, security_key) | ||
| ) | ||
| if expiration_date and expiration_date < fields.Datetime.now(): |
There was a problem hiding this comment.
I don't see the interest of this condition because _get_mcp_server_by_key already filter expired server key. Do I miss a case? or is it because the cache is not invalidated when expiration_date changed?
|
|
||
| class McpController(http.Controller): | ||
| @http.route( | ||
| "/mcp/<string:key>", type="http", auth="none", methods=["POST"], csrf=False |
There was a problem hiding this comment.
I'm would suggested to create a dedicated auth="ai_mcp" and de-coupled authentication and business logic even it generate an extra request.
| ._get_mcp_server_by_key(key, security_key) | ||
| ) | ||
| if expiration_date and expiration_date < fields.Datetime.now(): | ||
| request.env["mcp.server.key"].sudo().browse(server_id).expire_key() |
There was a problem hiding this comment.
I think will never been called because the filter, I would separate the way how the expire state is changed, using ir.cron ?
There was a problem hiding this comment.
probably called if the key come from cache
| "error": {"code": -32000, "message": "Connection failed"}, | ||
| } | ||
| ) | ||
| server = request.env["mcp.server.key"].sudo().browse(server_id) |
There was a problem hiding this comment.
could you rename this variable to something like mcp_server_key as you have mcp.server it's a bit confusing
| server = request.env["mcp.server.key"].sudo().browse(server_id) | |
| mcp_server_key = request.env["mcp.server.key"].sudo().browse(server_id) |
There was a problem hiding this comment.
done in this commit: 0ef5ad3 (in migratoin PR to v17)
| "id": payload.get("id"), | ||
| "result": { | ||
| "protocolVersion": "2025-03-26", | ||
| "capabilities": {"tools": {"listChanged": True}}, |
There was a problem hiding this comment.
I haven't seen any call to notified clients, do I miss something or mis understand th protocal, As far I understand this listChanded capabilities indicate the client the it will be notified when tools list result would changed
| "capabilities": {"tools": {"listChanged": True}}, | |
| "capabilities": {"tools": {}}, |
There was a problem hiding this comment.
Done by d621868 (in migratoin PR to v17)
| name = fields.Char() | ||
| description = fields.Text() | ||
| active = fields.Boolean(default=True) | ||
| key = fields.Char( |
There was a problem hiding this comment.
non locking point: Idea of improvement, add api.constraints to ensure it's a valid url path
| "result": { | ||
| "structuredContent": result, | ||
| "content": [{"type": "text", "text": json.dumps(result)}], | ||
| }, |
There was a problem hiding this comment.
it seems resultType is required in result
| "result": { | |
| "structuredContent": result, | |
| "content": [{"type": "text", "text": json.dumps(result)}], | |
| }, | |
| "result": { | |
| "resultType": "complete", | |
| "structuredContent": result, | |
| "content": [{"type": "text", "text": json.dumps(result)}], | |
| }, |
There was a problem hiding this comment.
you can ignore this message, I've linked the draft version of the protocol
|
just realize this is a migration PR so I guess those comment are valid in version 16.0 too. I'm on the way to migrate this module from v16.0 to v17.0 I'll probably create the PR on top of it to make easier back/forward ports |
| hashed_key = fields.Char(copy=False) | ||
| state = fields.Selection( | ||
| [("active", "Active"), ("expired", "Expired")], default="active" | ||
| ) |
There was a problem hiding this comment.
Using field selection for those 2 states I'm wondering why we are not rely on Odoo active bool feature ? that we could use those state later for an other purpose different than active or archvied
| key = fields.Char( | ||
| required=True, | ||
| copy=False, | ||
| default=lambda self: uuid4(), | ||
| groups="base.group_system", | ||
| ) |
There was a problem hiding this comment.
the name of this field is a bit confusing, mainly because of mcp.server.key model... I would suggested to named it slug / slug path / server path
Migration to 18.0
No major changes.
Depends on #69