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

Unauthorized GET users/{username}/testcases/{testcase_name} #35

Open
kbenne opened this issue Dec 19, 2023 · 8 comments
Open

Unauthorized GET users/{username}/testcases/{testcase_name} #35

kbenne opened this issue Dec 19, 2023 · 8 comments
Assignees

Comments

@kbenne
Copy link
Collaborator

kbenne commented Dec 19, 2023

"When I try GET users/{username}/testcases/{testcase_name} without providing any authorization I get a 404 response. But I think it should be a 401, which is what I get if I do GET users/{username}/testcases/. Basically, if I don't have permission to view all test cases with a username, I shouldn't have permission to check if a single test case is present either. "

This scenario should return a 401.

@kbenne kbenne self-assigned this Dec 19, 2023
kbenne added a commit that referenced this issue Feb 22, 2024
Previously, invalid endpoints would return a 404 in all cases. Now, if
there is an invalid Authorization header then 401 will be returned for
all generic endpoints under /users and /testcases.

ref #35
@kbenne
Copy link
Collaborator Author

kbenne commented Feb 22, 2024

@haraldwalnum and @dhblum.

I addressed part of this issue in 4dc1bd6. Now when you send an invalid Authorization header for any of the endpoints under /users or /testcases you will receive a 401 response. Addtionally, if you omit or send the wrong Authorization header for any endpoints under /users/{username} you will also receive a 401 response.

Now here is the rub. The AI (my brain) must have been hallicinating when the documentation was written, because it says that the endpoints GET /testcases/{testcase_name} and GET /users/{username}/testcases/{testcase_name} exist to check if a specific test case exists. The fact is that these do not exist. All of the BOPTEST Service endpoints are defined here and you can see that there is no such endpoint to check if a specific test case exists.

My question is what should we do about it?

  1. Update the documentation by removing these endpoints, but otherwise do nothing.
  2. Add the new endpoints.

One wrinkle which I believe has already been brought up, is what happens if a default (IBPSA) test case name matches a namespace name? In such a scenario two endpoints would be ambiguous.

Does test case named "apartment" exist in the default namespace?
GET /testcases/apartment

What test cases exist under the "apartment" namespace?
GET /testcases/apartment

Of course we could (and probbaly should) prevent this possibility with a rule in the test case upload API. Regardless, it does make me wonder if this indicates a flaw in the API and I'm interested in your thoughts. We could disambiguate the two overlapping APIs with a trailing /, but I"m not sure if this would be best practice.

Does test case named "apartment" exist in the default namespace?
GET /testcases/apartment

What test cases exist under the "apartment" namespace?
GET /testcases/apartment/

What do you all think?

@kbenne
Copy link
Collaborator Author

kbenne commented Feb 22, 2024

related. #34

@kbenne
Copy link
Collaborator Author

kbenne commented Feb 22, 2024

Another option might be to use the HTTP verb HEAD to determine the existence of a test case. https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD

@haraldwalnum
Copy link

Given that we have the endpoint that returns a list of endpoints under a namespace, I don't really see a great need for this endpoint.

@dhblum
Copy link
Contributor

dhblum commented Feb 23, 2024

Is it a better API design to do the following?

Does test case named "apartment" exist in the default namespace?
GET /testcases/apartment

What test cases exist under the "apartment" namespace?
GET /namespaces/apartment

@dhblum
Copy link
Contributor

dhblum commented Feb 23, 2024

I also agree with Harald, in that is:

Does test case named "apartment" exist in the default namespace?
GET /testcases/apartment

needed if GET /testcases gives you a list of test cases under the default namespace already?

kbenne added a commit that referenced this issue Feb 27, 2024
Remove documentation for APIs related to confirming the existence of
test cases. These endpoints were never implemented. Users can get the
list of test cases to confirm if a particular test case exists.

ref #35
@kbenne
Copy link
Collaborator Author

kbenne commented Feb 27, 2024

Thank guys. Based on your comments, I updated the documentation to remove those non-existant APIs and left it at that. I believe this issue is now resolved.

We do still have the possibility that a test case name can overlap with a namespace, but I don't think there is actually a problem with that. If we choose, we could prevent overlapping test case and namespace names on upload, but for now my suggestion is that we leave things as they are.

@dhblum
Copy link
Contributor

dhblum commented Feb 27, 2024

All sounds good, thanks Kyle.

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

No branches or pull requests

3 participants