Return HTTP 400 status for inactive OIDC auth methods #5048
Conversation
|
@johanbrandhorst I went with failed precondition as the situation we're in seems to line up with the grpc definition:
In this case, the state of the system (i.e the status of the OIDC auth method) is not in a state where we can start the auth flow. So the OIDC auth method being public feels like a precondition to its use. Ultimately for HCP's purposes any 4xx status code is fine. |
I agree that the error code makes sense, but generally in Boundary we care more about using the correct HTTP status code than the correct gRPC status code. I just want to make sure that we're not going against some existing pattern. I did a search and it seems we already use this code for various errors, so I think this is fine :) |
| if err != nil { | ||
| switch { | ||
| case errors.Match(errors.T(errors.AuthMethodInactive), err): | ||
| return nil, handlers.ApiErrorWithCodeAndMessage(codes.FailedPrecondition, "Cannot start authentication against an inactive OIDC auth method") |
There was a problem hiding this comment.
Note that this returns an HTTP 400 error and not a 412 from the GRPC gateway. I assume this would still make sense since it is on the client to activate the OIDC auth method. Does that sound correct?
There was a problem hiding this comment.
Alternatively, could the GRPC Aborted error could be more suitable here? It maps to the HTTP.StatusConflict, which is more descriptive to the case but not sure Aborted is more suitable than FailedPrecondition here
There was a problem hiding this comment.
I just saw that @johanbrandhorst was having a relevant discussion. This might be useful context, not sure if 400 is suitable here
There was a problem hiding this comment.
Oh that's interesting, I wasn't aware that was the mapping behaviour. I'm personally fine with a regular 400.
There was a problem hiding this comment.
Yes, the mapping is 400, I think that's fine here.
There was a problem hiding this comment.
I've updated the commit & PR title/description to reflect this is a 400 not a 412.
This commit modifies the OIDC authentication start method to return a 400 (Bad Request) error if a client attempts to start the authentication flow against an OIDC auth method that is set to inactive. This is something we noticed in our error logs in HCP Boundary that is impacting our SLOs. Previously this returned its own error code which is mapped to a 500 response. Tests have been updated to catch this new scenario. Signed-off-by: David Bond <davidsbond93@gmail.com>
davidsbond
force-pushed
the
4xx-for-inactive-auth
branch
from
91168d2
to
3cf9611
Compare
davidsbond
changed the title
|
Going to merge, the tests that are failing seem unrelated and they are failing across all other PRs |
This commit modifies the OIDC authentication start method to return a 400 (Bad Request) error if a client attempts to start the authentication flow against an OIDC auth method that is set to inactive. This is something we noticed in our error logs in HCP Boundary that is impacting our SLOs.
Previously this returned its own error code which is mapped to a 500 response. Tests have been updated to catch this new scenario.