auth: add an endpoint for basic auth diagnosis #28
Conversation
Let's add a trivial endpoint which can be used to determine whether the current user is authenticated, along with their roles. Without this, there is no way to verify directly that the authentication reverse-proxy is working correctly, or for a user to verify that they were given the roles they requested, etc. This also gives the healthcheck and whoami endpoints the same openapi tag, so that they appear in the API docs together (as both of them are kind of "meta APIs" relating to the service itself rather than the exodus CDN.)
|
Travis is acting a bit odd today. |
|
|
||
|
|
||
| @app.get("/healthcheck") | ||
| @app.get("/healthcheck", tags=["service"]) |
There was a problem hiding this comment.
This change is unrelated to this PR
There was a problem hiding this comment.
Tags could be added in an additional commit, at least.
There was a problem hiding this comment.
It is related to the PR... the purpose of putting tags on the APIs is to make them appear grouped in some logical way in the docs.
Prior to my change here, there was only one endpoint (/healthcheck) without any tag, so there was nothing to group it with. Now in this change I'm adding one more endpoint which seems to belong in the same group, so I've put the same tag on the old endpoint and my new endpoint to group them.
There was a problem hiding this comment.
The value of the change is unquestionable. Mixing this effort with another effort is the issue here. The value of keeping PRs small and related to the PR subject+linked issue is for future trouble shooting/debugging.
There was a problem hiding this comment.
Now in this change I'm adding one more endpoint which seems to belong in the same group, so I've put the same tag on the old endpoint and my new endpoint to group them.
Makes sense to me.
I see that the explanation is also included in the commit message.
There was a problem hiding this comment.
The value of keeping PRs small and related to the PR subject+linked issue is for future trouble shooting/debugging.
I agree with the principle of keeping PRs small and focused, but I don't think we agree on how to apply that principle. I explained (already in the commit message) how I think this change is related to the purpose of the PR.
| @@ -28,3 +28,10 @@ def test_healthcheck(): | |||
| ) | |||
| def test_publish(env, expected): | |||
| assert gateway.publish(env) == expected | |||
|
|
|||
|
|
|||
| def test_whoami(): | |||
There was a problem hiding this comment.
I am not sure if this test covers all options:
a request without any payload
a request with incorrect payload
a request with correct payload
There was a problem hiding this comment.
If I understand whoami correctly, there's only one possible response; it just returns whatever context it's given.
There was a problem hiding this comment.
Right, this test is not aiming to cover those cases, it doesn't work at that level.
This test (and all the other existing tests which directly invoke some endpoint function) have assumed that the FastAPI routing, (de)serialization, dependency injection etc features all work as documented. The test scope can be summarized as "Given that fastapi works correctly, whoami does what it needs to do".
Testing that fastapi works correctly is expected to be covered elsewhere. I think we could have at least a few tests in the repo making use of TestClient https://fastapi.tiangolo.com/tutorial/testing/#using-testclient to help cover that, I don't think all tests should be written that way though.
Taking the example "a request with incorrect payload" - if there is an incorrect value in the auth header, the expected behavior is that the request is rejected before it even reaches the whoami() function, so it can't be covered by this style of test which calls whoami directly. It's instead already tested (in isolation) in tests/auth/test_call_context.py::test_bad_header.
There is no integration test currently which tests the pieces when combined. I looked into it just now and I think it might be better left to a later PR... the reason is that there is still no endpoint which requires authentication yet. This means there's no way for an integration test to realistically cover cases such as "user didn't authenticate where authentication is required" or "user authenticated but is missing needed roles".
|
Is there an issue this PR can be linked to? |
|
It seems CI is still acting strange. |
Let's add a trivial endpoint which can be used to determine whether
the current user is authenticated, along with their roles. Without
this, there is no way to verify directly that the authentication
reverse-proxy is working correctly, or for a user to verify that
they were given the roles they requested, etc.
This also gives the healthcheck and whoami endpoints the same
openapi tag, so that they appear in the API docs together (as both
of them are kind of "meta APIs" relating to the service itself
rather than the exodus CDN.)