brocaar / chirpstack-application-server

ChirpStack Application Server is an open-source LoRaWAN application-server.
https://www.chirpstack.io
MIT License
502 stars 325 forks source link

Querying a non-existing device results in different error-codes #545

Closed GinterP closed 3 years ago

GinterP commented 4 years ago

What happened?

When using the REST-API and querying a GET /api/devices/{dev_eui} with a non-existing dev_eui:

The problem could be located at validators.go: ValidateNodeAccess(devEUI lorawan.EUI64, flag Flag):

What did you expect?

I would expect getting a 404 error in both scenarios.

Steps to reproduce this issue

Steps:

  1. Create an user and an admin
  2. Use the REST-API to query a non-existing device with the user and the admin

Your Environment: Docker

Component Version
Application Server v3.12.2
brocaar commented 4 years ago

I think this applies to all endpoints. The query tries to find an user either by is_admin or related to dev_eui. Assuming the user is_admin=false, this means that when there are no results when using the dev_eui filter, this could mean that:

As the end-result is that the query does not return any records, this results always in a 401 error. I'm not sure what the best approach would be to solve this as this is how all validator functions are working.

Any ideas?

GinterP commented 4 years ago

A simple solution could be returning here a 404 error instead of 401.

To my understanding this would result in the following error codes (not tested):

admin non-admin user (with access to the resource) non-admin user (who hasn't access to the resource) non-authorized user
resource exists 200 200 404 (currently: 401) 401
resource doesn't exist 404 404 (currently: 401) 404 (currently: 401) 401

In this StackOverflow question they discuss that topic. The conclusion is that it shouldn't make a big difference if you return 404 or 401 for non-privileged users as long as they always get the same error code.

What do you mean?

brocaar commented 4 years ago

A simple solution could be returning here a 404 error instead of 401.

I partly agree with that.

When the web-interface makes an API request and gets a 401 as a response, it redirects back to the login page (https://github.com/brocaar/chirpstack-application-server/blob/master/ui/src/stores/helpers.js#L23). An use-case for this to happen is for example when the user is not logged in or when the JWT token has expired.

If we would change the 401 to 404 for all these cases, the web-interface would never be able to redirect to the login page.

However, the 401 cases would in this case always be related to the JWT token, not to the resource you are trying to access. So I'm fine with returning a 404 when a device does exist but the user does not have access to it, but if we change that line to always return a 404 code, then this would also affect the JWT token validation.

GinterP commented 4 years ago

So we have two options:

  1. Don't change anything and keep returning a 401 error.
  2. Change the behaviour of auth.ValidateNodeAccess(..), which needs to distinguish between an invalid/expired JWT token (return a 401 error) and an error which is related to non-existing/-accessible resources (return a 404 error).

But I don't think the effort is worth it. What do you think?

brocaar commented 3 years ago

But I don't think the effort is worth it.

I agree :) I'll close the issue.