Merge branch 'stage/v7.0.0' of github.com:SpecterOps/BloodHound into …

…BED-5323
SpecterOps · Jan 29, 2025 · b4f5857 · b4f5857
2 parents 8c999df + 5c18b8b
commit b4f5857
Showing 19 changed files with 292 additions and 69 deletions.
diff --git a/cmd/api/src/api/error.go b/cmd/api/src/api/error.go
@@ -68,6 +68,7 @@ const (
 	ErrorResponseAGDuplicateTag                     = "asset group tag must be unique"
 	ErrorResponseSSOProviderDuplicateName           = "sso provider name must be unique"
 	ErrorResponseUserDuplicatePrincipal             = "principal name must be unique"
+	ErrorResponseUserDuplicateEmail                 = "email must be unique"
 	ErrorResponseDetailsUniqueViolation             = "unique constraint was violated"
 	ErrorResponseDetailsNotImplemented              = "All good things to those who wait. Not implemented."
 

diff --git a/cmd/api/src/api/v2/auth/auth.go b/cmd/api/src/api/v2/auth/auth.go
@@ -303,6 +303,8 @@ func (s ManagementResource) CreateUser(response http.ResponseWriter, request *ht
 		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, ErrResponseDetailsNumRoles, request), response)
 	} else if roles, err := s.db.GetRoles(request.Context(), createUserRequest.Roles); err != nil {
 		api.HandleDatabaseError(request, response, err)
+	} else if isNewEmail := s.db.IsNewEmail(request.Context(), createUserRequest.EmailAddress); !isNewEmail {
+		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusConflict, api.ErrorResponseUserDuplicateEmail, request), response)
 	} else {
 		userTemplate.Roles = roles
 		userTemplate.FirstName = null.StringFrom(createUserRequest.FirstName)
@@ -386,6 +388,9 @@ func (s ManagementResource) UpdateUser(response http.ResponseWriter, request *ht
 		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, "a user can only have one role", request), response)
 	} else if roles, err := s.db.GetRoles(request.Context(), updateUserRequest.Roles); err != nil {
 		api.HandleDatabaseError(request, response, err)
+	} else if !strings.EqualFold(user.EmailAddress.String, updateUserRequest.EmailAddress) && !s.db.IsNewEmail(request.Context(), updateUserRequest.EmailAddress) {
+		// Prevent duplicate emails, this can be removed once the db constraint is added
+		api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusConflict, api.ErrorResponseUserDuplicateEmail, request), response)
 	} else {
 		user.Roles = roles
 		user.FirstName = null.StringFrom(updateUserRequest.FirstName)

diff --git a/cmd/api/src/api/v2/auth/auth_test.go b/cmd/api/src/api/v2/auth/auth_test.go
@@ -1048,6 +1048,7 @@ func TestCreateUser_Failure(t *testing.T) {
 	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Not(badRole)).Return(model.Roles{}, nil).AnyTimes()
 	mockDB.EXPECT().AppendAuditLog(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 	mockDB.EXPECT().CreateUser(gomock.Any(), badUser).Return(model.User{}, fmt.Errorf("db error"))
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), gomock.Any()).Return(true).AnyTimes()
 
 	type Input struct {
 		Body v2.CreateUserRequest
@@ -1146,6 +1147,44 @@ func TestCreateUser_Failure(t *testing.T) {
 	}
 }
 
+func TestCreateUser_FailureDuplicateEmail(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	endpoint := "/api/v2/auth/users"
+
+	resources, mockDB := apitest.NewAuthManagementResource(mockCtrl)
+	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{}, nil)
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(false)
+
+	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
+	input := v2.CreateUserRequest{
+		UpdateUserRequest: v2.UpdateUserRequest{
+			Principal: "good user",
+		},
+		SetUserSecretRequest: v2.SetUserSecretRequest{
+			Secret:             "abcDEF123456$$",
+			NeedsPasswordReset: true,
+		},
+	}
+
+	if payload, err := json.Marshal(input); err != nil {
+		t.Fatal(err)
+	} else if req, err := http.NewRequestWithContext(ctx, "POST", endpoint, bytes.NewReader(payload)); err != nil {
+		t.Fatal(err)
+	} else {
+		req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+		router := mux.NewRouter()
+		router.HandleFunc(endpoint, resources.CreateUser).Methods("POST")
+
+		rr := httptest.NewRecorder()
+		router.ServeHTTP(rr, req)
+
+		require.Equal(t, rr.Code, http.StatusConflict)
+		require.Contains(t, rr.Body.String(), "email must be unique")
+	}
+}
+
 func TestCreateUser_Success(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
@@ -1162,6 +1201,7 @@ func TestCreateUser_Success(t *testing.T) {
 	}, nil)
 	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{}, nil)
 	mockDB.EXPECT().CreateUser(gomock.Any(), gomock.Any()).Return(goodUser, nil).AnyTimes()
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true)
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{
@@ -1215,6 +1255,7 @@ func TestCreateUser_ResetPassword(t *testing.T) {
 	}, nil)
 	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{}, nil)
 	mockDB.EXPECT().CreateUser(gomock.Any(), gomock.Any()).Return(goodUser, nil)
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true)
 
 	input := struct {
 		Body v2.CreateUserRequest
@@ -1288,6 +1329,7 @@ func TestManagementResource_UpdateUser_IDMalformed(t *testing.T) {
 	}, nil)
 	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{}, nil)
 	mockDB.EXPECT().CreateUser(gomock.Any(), gomock.Any()).Return(goodUser, nil).AnyTimes()
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true)
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{
@@ -1352,6 +1394,7 @@ func TestManagementResource_UpdateUser_GetUserError(t *testing.T) {
 	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{}, nil)
 	mockDB.EXPECT().CreateUser(gomock.Any(), gomock.Any()).Return(goodUser, nil).AnyTimes()
 	mockDB.EXPECT().GetUser(gomock.Any(), gomock.Any()).Return(model.User{}, fmt.Errorf("foo"))
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true)
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{
@@ -1417,6 +1460,7 @@ func TestManagementResource_UpdateUser_GetRolesError(t *testing.T) {
 	mockDB.EXPECT().CreateUser(gomock.Any(), gomock.Any()).Return(goodUser, nil).AnyTimes()
 	mockDB.EXPECT().GetUser(gomock.Any(), gomock.Any()).Return(goodUser, nil)
 	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{}, fmt.Errorf("foo"))
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true)
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{
@@ -1455,6 +1499,36 @@ func TestManagementResource_UpdateUser_GetRolesError(t *testing.T) {
 	require.Equal(t, http.StatusInternalServerError, response.Code)
 }
 
+func TestManagementResource_UpdateUser_DuplicateEmailError(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	goodUserID, err := uuid.NewV4()
+	require.Nil(t, err)
+
+	resources, mockDB := apitest.NewAuthManagementResource(mockCtrl)
+	mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{}, nil)
+	mockDB.EXPECT().GetUser(gomock.Any(), gomock.Any()).Return(model.User{EmailAddress: null.StringFrom("")}, nil)
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "different").Return(false)
+
+	reqCtx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
+
+	payload, err := json.Marshal(v2.UpdateUserRequest{EmailAddress: "different"})
+	require.Nil(t, err)
+
+	endpoint := fmt.Sprintf("/api/v2/bloodhound-users/%v", goodUserID)
+	req, err := http.NewRequestWithContext(reqCtx, "PATCH", endpoint, bytes.NewReader(payload))
+	require.Nil(t, err)
+
+	req = mux.SetURLVars(req, map[string]string{api.URIPathVariableUserID: goodUserID.String()})
+	req.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String())
+	response := httptest.NewRecorder()
+	handler := http.HandlerFunc(resources.UpdateUser)
+	handler.ServeHTTP(response, req)
+	require.Equal(t, http.StatusConflict, response.Code)
+	require.Contains(t, response.Body.String(), "email must be unique")
+}
+
 func TestManagementResource_UpdateUser_SelfDisable(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
@@ -1484,6 +1558,7 @@ func TestManagementResource_UpdateUser_SelfDisable(t *testing.T) {
 		}},
 		Serial: model.Serial{},
 	}}, nil)
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true).AnyTimes()
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{
@@ -1557,6 +1632,7 @@ func TestManagementResource_UpdateUser_UserSelfModify(t *testing.T) {
 		mockDB.EXPECT().GetRoles(gomock.Any(), gomock.Any()).Return(model.Roles{adminRole}, nil)
 		mockDB.EXPECT().GetUser(gomock.Any(), adminUser.ID).Return(adminUser, nil)
 		mockDB.EXPECT().GetSSOProviderById(gomock.Any(), int32(1)).Return(model.SSOProvider{}, nil)
+
 		test.Request(t).
 			WithContext(bhCtx).
 			WithURLPathVars(map[string]string{"user_id": adminUser.ID.String()}).
@@ -1624,6 +1700,7 @@ func TestManagementResource_UpdateUser_LookupActiveSessionsError(t *testing.T) {
 		Serial: model.Serial{},
 	}}, nil)
 	mockDB.EXPECT().LookupActiveSessionsByUser(gomock.Any(), gomock.Any()).Return([]model.UserSession{}, fmt.Errorf("foo"))
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true).AnyTimes()
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{
@@ -1705,6 +1782,7 @@ func TestManagementResource_UpdateUser_DBError(t *testing.T) {
 		Serial: model.Serial{},
 	}}, nil)
 	mockDB.EXPECT().UpdateUser(gomock.Any(), gomock.Any()).Return(fmt.Errorf("foo"))
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true).AnyTimes()
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{
@@ -1931,6 +2009,7 @@ func TestManagementResource_UpdateUser_Success(t *testing.T) {
 	}}, nil)
 	mockDB.EXPECT().LookupActiveSessionsByUser(gomock.Any(), gomock.Any()).Return([]model.UserSession{}, nil)
 	mockDB.EXPECT().UpdateUser(gomock.Any(), gomock.Any()).Return(nil)
+	mockDB.EXPECT().IsNewEmail(gomock.Any(), "").Return(true).AnyTimes()
 
 	ctx := context.WithValue(context.Background(), ctx.ValueKey, &ctx.Context{})
 	input := v2.CreateUserRequest{

diff --git a/cmd/api/src/api/v2/auth/oidc.go b/cmd/api/src/api/v2/auth/oidc.go
@@ -160,7 +160,7 @@ func (s ManagementResource) OIDCLoginHandler(response http.ResponseWriter, reque
 
 	if ssoProvider.OIDCProvider == nil {
 		// SSO misconfiguration scenario
-		api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator")
+		api.RedirectToLoginURL(response, request, "Your SSO connection failed due to misconfiguration, please contact your Administrator")
 	} else if state, err := config.GenerateRandomBase64String(77); err != nil {
 		slog.WarnContext(request.Context(), fmt.Sprintf("[OIDC] Failed to generate state: %v", err))
 		// Technical issues scenario
@@ -169,7 +169,7 @@ func (s ManagementResource) OIDCLoginHandler(response http.ResponseWriter, reque
 		slog.WarnContext(request.Context(), fmt.Sprintf("[OIDC] Failed to create OIDC provider: %v", err))
 		// SSO misconfiguration or technical issue
 		// Treat this as a misconfiguration scenario
-		api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator")
+		api.RedirectToLoginURL(response, request, "Your SSO connection failed due to misconfiguration, please contact your Administrator")
 	} else {
 		conf := &oauth2.Config{
 			ClientID:    ssoProvider.OIDCProvider.ClientID,
@@ -207,33 +207,37 @@ func (s ManagementResource) OIDCCallbackHandler(response http.ResponseWriter, re
 
 	if ssoProvider.OIDCProvider == nil {
 		// SSO misconfiguration scenario
-		api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator")
+		api.RedirectToLoginURL(response, request, "Your SSO connection failed due to misconfiguration, please contact your Administrator")
 	} else if code == "" {
-		slog.WarnContext(request.Context(), "[OIDC] auth code is missing")
-		api.RedirectToLoginURL(response, request, "We're having trouble connecting. Please check your internet and try again.")
 		// Missing authorization code implies a credentials or form issue
+		slog.WarnContext(request.Context(), "[OIDC] auth code is missing")
+		api.RedirectToLoginURL(response, request, "Invalid SSO Provider response: `code` parameter is missing")
 	} else if state == "" {
-		slog.WarnContext(request.Context(), "[OIDC] state parameter is missing")
 		// Missing state parameter - treat as technical issue
-		api.RedirectToLoginURL(response, request, "We're having trouble connecting. Please check your internet and try again.")
+		slog.WarnContext(request.Context(), "[OIDC] state parameter is missing")
+		api.RedirectToLoginURL(response, request, "Invalid SSO Provider response: `state` parameter is missing")
 	} else if pkceVerifier, err := request.Cookie(api.AuthPKCECookieName); err != nil {
-		slog.WarnContext(request.Context(), "[OIDC] pkce cookie is missing")
 		// Missing PKCE verifier - likely a technical or config issue
-		api.RedirectToLoginURL(response, request, "We're having trouble connecting. Please check your internet and try again.")
-	} else if stateCookie, err := request.Cookie(api.AuthStateCookieName); err != nil || stateCookie.Value != state {
-		slog.WarnContext(request.Context(), fmt.Sprintf("[OIDC] state cookie does not match %v", err))
-		// Invalid state - treat as technical issue or misconfiguration
-		api.RedirectToLoginURL(response, request, "We're having trouble connecting. Please check your internet and try again.")
+		slog.WarnContext(request.Context(), "[OIDC] pkce cookie is missing")
+		api.RedirectToLoginURL(response, request, "Invalid request: `pkce` is missing")
+	} else if stateCookie, err := request.Cookie(api.AuthStateCookieName); err != nil {
+		// Missing state - likely a technical or config issue
+		slog.WarnContext(request.Context(), "[OIDC] state cookie is missing")
+		api.RedirectToLoginURL(response, request, "Invalid request: `state` is missing")
+	} else if stateCookie.Value != state {
+		// State mismatch
+		slog.WarnContext(request.Context(), "[OIDC] state does not match")
+		api.RedirectToLoginURL(response, request, "Invalid: `state` do not match")
 	} else if provider, err := oidc.NewProvider(request.Context(), ssoProvider.OIDCProvider.Issuer); err != nil {
-		slog.WarnContext(request.Context(), fmt.Sprintf("[OIDC] Failed to create OIDC provider: %v", err))
 		// SSO misconfiguration scenario
-		api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator")
+		slog.WarnContext(request.Context(), fmt.Sprintf("[OIDC] Failed to create OIDC provider: %v", err))
+		api.RedirectToLoginURL(response, request, "Your SSO connection failed due to misconfiguration, please contact your Administrator")
 	} else if claims, err := getOIDCClaims(request.Context(), provider, ssoProvider, pkceVerifier, code); err != nil {
 		slog.WarnContext(request.Context(), fmt.Sprintf("[OIDC] %v", err))
-		api.RedirectToLoginURL(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator")
+		api.RedirectToLoginURL(response, request, fmt.Sprintf("Exchange failed: %s", err.Error()))
 	} else if email, err := getEmailFromOIDCClaims(claims); errors.Is(err, ErrEmailMissing) { // Note email claims are not always present so we will check different claim keys for possible email
 		slog.WarnContext(request.Context(), "[OIDC] Claims did not contain any valid email address")
-		api.RedirectToLoginURL(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator")
+		api.RedirectToLoginURL(response, request, "Claims invalid: no valid email address found")
 	} else {
 		if ssoProvider.Config.AutoProvision.Enabled {
 			if err := jitOIDCUserCreation(request.Context(), ssoProvider, email, claims, s.db); err != nil {