Skip to content
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

error message update #491

Merged
merged 2 commits into from
Jan 26, 2024
Merged

error message update #491

merged 2 commits into from
Jan 26, 2024

Conversation

UlfBj
Copy link
Contributor

@UlfBj UlfBj commented Jan 24, 2024

Below are my rationale for the changes.
304 not_modified

What would lead to this response? A Set that fails should have a different error code.

400 filter_invalid

This is covered by 400 bad_request or invalid_data

400 invalid_duration

This is covered by 400 invalid_data

400 invalid_value

This is replaced by 400 invalid_data

401 too_many_attempts

Authentication is validated by the AGTS, so this is not visible in this context.

401 read_only

This is covered by 401 invalid_token.

403 user_unknown

User identity is only explicit (in client context) in a token. This error would therefore be caught in the authentication step.

403 user_forbidden

User identity is only explicit (in client context) in a token. This error would therefore be caught in the authentication step.

403 device_forbidden

Device identity is only explicit (in client context) in a token. This error would therefore be caught in the authentication step.

403 device_unknown

Device identity is only explicit (in client context) in a token. This error would therefore be caught in the authentication step.

404 invalid_path

This is covered by 400 invalid_data

404 private_path

This is not a relevant scenario.

404 invalid_subscription_id

This is covered by 400 invalid_data

406 insufficient_priviledges

This is covered by 401 invalid_token

406 not_acceptable

Is this the role of the server to judge?

429 too_many_requests

This is covered by 503 service_unavailable.

502 bad_gateway

Is this a relevant scenario?

504 gateway_timeout

Is this a relevant scenario?

@UlfBj
Copy link
Contributor Author

UlfBj commented Jan 24, 2024

@@ -1677,7 +1597,8 @@ <h2>Action Definitions</h2>
<section id="error-def">
<h2>Error Definitions</h2>
<p>The error number SHOULD be a status code defined in [[RFC2616]], c. f. chapter "Status codes".
The error reason SHOULD be the corresponding reason-phrase from [[RFC2616]].
The error reason SHOULD be short. two or three words connected by underscor.
It SHOULD relate to the reason-phrase from [[RFC2616]] for the corresponding status code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful with an example here. RFC2616 only presents recommended reason phrases right (see https://datatracker.ietf.org/doc/html/rfc2616#section-6.1.1), like "Forbidden" for 403. So what does "It SHOULD relate to the reason-phrase" actually mean? Can we say that forbidden_request is related to the recommended Forbidden in RFC2616

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say that forbidden_request relates to Forbidden in RFC2616.
To use examples I think is unnecessary, but you could try to add it.

<td>private_path</td>
<td>The specified data path is private and the request is not authorized to access signals on this path.</td>
</tr>
<tr>
<td>404 (Not Found)</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see two possible error situations here.

  • One is that the server does not know about the signal, like Vehicle/Zpeed
  • Another is that the path is correct (Vehicle/Speed), but server does not yet have any value for speed.

Will 404 be returned in both cases, or will the second case result in OK (200) but with empty result?

Choose a reason for hiding this comment

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

well, its not path error so then one would lean towards 200 ...but then what is an empty result ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vehicle.Zpeed would lead to 404 unavaliable_data
Server not having any data for a correct path would also be 404.

<td>device_unknown</td>
<td>The device is unknown. Retrying does not help.</td>
<td>forbidden_request</td>
<td>The server refuses to carry out the request.</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we state that this for example might be given if token does not give access to this operation/path/signal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think that examples should be part of this text that is sent in the response.
If there is a need for giving examples I think it should be elsewhere in this chapter.

@erikbosch
Copy link
Contributor

Do we intend to cover rejection due to safety reasons in VISS. I have seen other proposals of using 409 (Conflict) if something cannot be done due to safety reasons, like open door is prevented due to vehicle moving.

@UlfBj
Copy link
Contributor Author

UlfBj commented Jan 26, 2024

Regarding the error code 409, it is specified in RFC2616, for which the spec says
The client SHOULD support any status code defined in RFC2616
So I think we have it sufficiently covered.
But if we think it should be highlighted I am fine with adding it to the list.

@UlfBj UlfBj merged commit 3a72215 into w3c:gh-pages Jan 26, 2024
3 checks passed
@tguild
Copy link
Member

tguild commented Jan 26, 2024

I am wondering if we are perhaps overloading error codes with multiple meanings too much and if there isn't a good match in HTTP and instead create additional codes. Also feel like we might have circled around this before and even requested/received advice that it was reasonable. Perhaps someone has a better recollection of that conversation @caribouW3 may recall. I can also look for old issues and minutes on that.

What I am certain would not be a misuse of response codes is:

NNN immutable_descriptor "additional free form text providing additional details"

examples:

404 Document not found "The VSS signal name requested does not exist on this server"

or since Ulf prefers considering that an invalid request

400 Bad request "The VSS signal name requested does not exist on this server"

Btw on 401 vs 403,

401 is the initial authentication needed response and can also be returned if given improper credentials, prompting the client to submit different ones. This can be a looping scenario the server needs to be careful about (stop responding to client if receiving too many requests/minute which also protects against brute force attacks). Client can break the loop too, think of the now seldom seen basic/digest authentication browser pop up for username/password (html forms, certificates etc currently more common instead on the web) and instead of providing credentials and submitting with ok, hitting cancel. At that point you get a 403.

403 is for when permissions aren't set or client provided credentials that are not authorized to access the resource.

and multiple 403s would be

403 Forbidden "Device not permitted" (btw not sure it is important to distinguish an unknown from a non-permitted device)
403 Forbidden "User not permitted"

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

@erikbosch
Copy link
Contributor

I took a second look on this PR and the result after it was merged on realized that there are some more changes that we better shall do to make things consistent. First, we should review the examples in VISSv2 Transport, where we today sometimes use error numbers and error reasons not listed in the table. Allowed, but not maybe optimal, we should better have examples with the "recommended" error reasons.

Concerning the comment from Ted, isn't the "official" reason for each status code is "hard-coded", i.e. that we see "Not Found" below is "mandatory" as the error code is 404. But for reason and message in the error body we are free to select whatever we want, but depending on implementation I assume that we not always will get an error body, for example if the error is given by the server framework rather than the VISS application.


HTTP/1.1 404 Not Found
              Content-Type: application/json; charset=utf-8
	      ...
              {
                "error": {"number": 404, "reason": "invalid_path", "message": "The specified data path does not exist."},
                "ts": "2020-04-15T13:37:00Z"
              }

Then we actually mention explicit error codes here and there in the core spec. Maybe we should take a second look at them as well.

https://raw.githack.com/w3c/automotive/gh-pages/spec/VISSv2_Core.html#access-control-server

@UlfBj
Copy link
Contributor Author

UlfBj commented Jan 30, 2024

Updating the examples is a good idea.

Regarding having an error code that explicitly addresses paths I think leads to an "unbalance" if that level of detail is not used also for other data elements e.g. in filter expressions etc.
In the current error list all data elements are addressed by either invalid_data or unavailable_data.

@UlfBj UlfBj deleted the issue474 branch January 30, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants