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.

Sign up for GitHub

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

Jump to bottom

Return HTTP 400 status for inactive OIDC auth methods #5048

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Return HTTP 400 status for inactive OIDC auth methods #5048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion internal/daemon/controller/handlers/authmethods/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,14 @@ func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.Authenticat
}

authUrl, tokenId, err := oidc.StartAuth(ctx, s.oidcRepoFn, req.GetAuthMethodId(), opts...)
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")
Copy link
Contributor

@uruemu uruemu Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@uruemu uruemu Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

https://github.com/grpc-ecosystem/grpc-gateway/blob/93189d015a3e23872723445862dc6fe1c4502913/runtime/errors.go#L61-L62

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw that @johanbrandhorst was having a relevant discussion. This might be useful context, not sure if 400 is suitable here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that's interesting, I wasn't aware that was the mapping behaviour. I'm personally fine with a regular 400.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the mapping is 400, I think that's fine here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the commit & PR title/description to reflect this is a 400 not a 412.

case errors.Match(errors.T(errors.RecordNotFound), err):
return nil, handlers.ApiErrorWithCodeAndMessage(codes.NotFound, "Auth method %s was not found", req.GetAuthMethodId())
case errors.Match(errors.T(errors.InvalidParameter), err):
return nil, handlers.ApiErrorWithCodeAndMessage(codes.InvalidArgument, err.Error())
case err != nil:
event.WriteError(ctx, op, err, event.WithInfoMsg("error starting the oidc authentication flow"))
return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Error generating parameters for starting the OIDC flow. See the controller's log for more information.")
}
Expand Down
62 changes: 59 additions & 3 deletions internal/daemon/controller/handlers/authmethods/oidc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1624,9 +1624,10 @@ func TestAuthenticate_OIDC_Start(t *testing.T) {
s := getSetup(t)

cases := []struct {
name string
request *pbs.AuthenticateRequest
wantErr error
name string
request *pbs.AuthenticateRequest
beforeTest func(t *testing.T)
wantErr error
}{
{
name: "no command",
Expand Down Expand Up @@ -1657,6 +1658,57 @@ func TestAuthenticate_OIDC_Start(t *testing.T) {
AuthMethodId: s.authMethod.GetPublicId(),
},
},
{
name: "auth method does not exist",
request: &pbs.AuthenticateRequest{
Command: "start",
AuthMethodId: "amoidc_1234",
Attrs: &pbs.AuthenticateRequest_OidcStartAttributes{
OidcStartAttributes: &pbs.OidcStartAttributes{
RoundtripPayload: func() *structpb.Struct {
ret, err := structpb.NewStruct(map[string]any{
"foo": "bar",
"baz": true,
})
require.NoError(t, err)
return ret
}(),
},
},
},
wantErr: handlers.ApiErrorWithCode(codes.NotFound),
},
{
name: "auth method is inactive",
beforeTest: func(t *testing.T) {
r, err := s.oidcRepoFn()
require.NoError(t, err)
_, err = r.MakeInactive(s.ctx, s.authMethod.GetPublicId(), 2)
require.NoError(t, err)

t.Cleanup(func() {
_, err = r.MakePublic(s.ctx, s.authMethod.GetPublicId(), 3)
require.NoError(t, err)
})
},
request: &pbs.AuthenticateRequest{
Command: "start",
AuthMethodId: s.authMethod.GetPublicId(),
Attrs: &pbs.AuthenticateRequest_OidcStartAttributes{
OidcStartAttributes: &pbs.OidcStartAttributes{
RoundtripPayload: func() *structpb.Struct {
ret, err := structpb.NewStruct(map[string]any{
"foo": "bar",
"baz": true,
})
require.NoError(t, err)
return ret
}(),
},
},
},
wantErr: handlers.ApiErrorWithCode(codes.FailedPrecondition),
},
// NOTE: We can't really test bad roundtrip payload attributes because
// any type structpb lets us use in creation will be valid for JSON, and
// attempting to force with e.g. json.RawMessage causes structpb to
Expand Down Expand Up @@ -1692,6 +1744,10 @@ func TestAuthenticate_OIDC_Start(t *testing.T) {

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
if tc.beforeTest != nil {
tc.beforeTest(t)
}

assert, require := assert.New(t), require.New(t)
got, err := s.authMethodService.Authenticate(auth.DisabledAuthTestContext(s.iamRepoFn, s.org.GetPublicId()), tc.request)
if tc.wantErr != nil {
Expand Down
Loading