CP-54274: Add user-specific error message for vnc connection limits #6710
Conversation
Add user name and detailed reason in http response when console connection limits are exceeded. Signed-off-by: Stephen Cheng <[email protected]>
|
Tested with XenCenter. I captured the message from XenCenter, and the message was sent correctly. |
ocaml/libs/http-lib/http_svr.ml
Outdated
|
|
||
| let response_forbidden_with_body ?req s body = | ||
| let version = Option.map get_return_version req in | ||
| response_error_html ?version s "403" "Forbidden" [] body |
There was a problem hiding this comment.
Do we want 403, not 503 as before?
There was a problem hiding this comment.
It's a client issue, not a server issue, so a 4xx response is warranted, maybe a 429 (too many requests) would be a better fit here?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/429
There was a problem hiding this comment.
If we expect more specific error responses - does it make sense to factor this into a function that takes the error code and message as arguments?
There was a problem hiding this comment.
I think it is more like a service side issue. It's server that is applying the restriction.
How about 503 + a specific error message?
There was a problem hiding this comment.
Both 429 and 503 are reasonable to me.
Yeah, it's more of a serser side issue, because the server actively reject the connection.
I'll change this to 503.
There was a problem hiding this comment.
If we expect more specific error responses - does it make sense to factor this into a function that takes the error code and message as arguments?
Yeah, the specific error responses are contained in the body. See https://github.com/xapi-project/xen-api/pull/6710/files#diff-b467704b510207bfdab55411ab1ff91aae3f19c3e0ab0409f142fd1646595a3cR236
But yes, we can make this fucion more generic taking error code and messages as arguments. Will modify it.
ocaml/xapi/console.ml
Outdated
| let user = Db.Session.get_auth_user_name ~__context ~self:session_id in | ||
| let body = | ||
| Printf.sprintf | ||
| "<html><body><h1>Connection Limit Exceeded</h1><p>User '%s': Only 1 \ |
There was a problem hiding this comment.
Why is this useful? The caller would know itself right?
There was a problem hiding this comment.
This could create invalid HTML if user has characters with special meaning in HTML that would require escaping.
!https://miro.medium.com/v2/resize:fit:720/format:webp/1*P4nj9fJjSeJ9-c0rwSZqlg.png
There was a problem hiding this comment.
Why is this useful? The caller would know itself right?
Indeed it's meaningless. Instead, we can send in the response the user name who is using the connection.
There was a problem hiding this comment.
But that may be a security issue...
ocaml/libs/http-lib/http_svr.ml
Outdated
|
|
||
| let response_forbidden_with_body ?req s body = | ||
| let version = Option.map get_return_version req in | ||
| response_error_html ?version s "403" "Forbidden" [] body |
There was a problem hiding this comment.
If we expect more specific error responses - does it make sense to factor this into a function that takes the error code and message as arguments?
ocaml/xapi/console.ml
Outdated
| let user = Db.Session.get_auth_user_name ~__context ~self:session_id in | ||
| let body = | ||
| Printf.sprintf | ||
| "<html><body><h1>Connection Limit Exceeded</h1><p>User '%s': Only 1 \ |
There was a problem hiding this comment.
This could create invalid HTML if user has characters with special meaning in HTML that would require escaping.
!https://miro.medium.com/v2/resize:fit:720/format:webp/1*P4nj9fJjSeJ9-c0rwSZqlg.png
Signed-off-by: Stephen Cheng <[email protected]>
|
Fixed the comments above and tested.
Tested: |
If we do this, we need to consider whether the console user has the right permissions to get data about other users. |
5 participants
Add user name and detailed reason in http response when console connection limits are exceeded.