From 3cf9611cc4a03dad43d3a2ef521b7af5216d1e64 Mon Sep 17 00:00:00 2001
From: David Bond <davidsbond93@gmail.com>
Date: Tue, 27 Aug 2024 17:59:40 +0100
Subject: [PATCH] Return HTTP 400 status for inactive OIDC auth methods

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>
---
 .../controller/handlers/authmethods/oidc.go   |  9 ++-
 .../handlers/authmethods/oidc_test.go         | 62 ++++++++++++++++++-
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/internal/daemon/controller/handlers/authmethods/oidc.go b/internal/daemon/controller/handlers/authmethods/oidc.go
index 1285c28332..853fc743b9 100644
--- a/internal/daemon/controller/handlers/authmethods/oidc.go
+++ b/internal/daemon/controller/handlers/authmethods/oidc.go
@@ -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")
+	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.")
 	}
diff --git a/internal/daemon/controller/handlers/authmethods/oidc_test.go b/internal/daemon/controller/handlers/authmethods/oidc_test.go
index d75e0bdb61..fc79db52a1 100644
--- a/internal/daemon/controller/handlers/authmethods/oidc_test.go
+++ b/internal/daemon/controller/handlers/authmethods/oidc_test.go
@@ -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",
@@ -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
@@ -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 {