From af13deeab8815d78aa9a06798b6c119d3ca09c89 Mon Sep 17 00:00:00 2001 From: kevwhitt-hee Date: Wed, 6 Mar 2024 08:51:23 +0000 Subject: [PATCH 1/3] TD3838 Reworks query to use coalesce to avoid throwing exception --- .../DataServices/CourseContentService.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs b/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs index b6e861a054..62f7c8adfd 100644 --- a/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs +++ b/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs @@ -270,18 +270,18 @@ FROM Sessions AS S1 WITH (NOLOCK) try { return connection.QueryFirst( - @"SELECT ProgressId - FROM Progress + @"SELECT COALESCE((SELECT ProgressID + FROM Progress WHERE CandidateID = @candidateId AND CustomisationID = @customisationId AND SystemRefreshed = 0 - AND RemovedDate IS NULL", + AND RemovedDate IS NULL), 0) AS ProgressId", new { candidateId, customisationId } ); } catch (InvalidOperationException) { - return null; + return 0; } } } From 1f359b62a77c60eee6c55af671e3570161f3a488 Mon Sep 17 00:00:00 2001 From: kevwhitt-hee Date: Wed, 6 Mar 2024 11:47:58 +0000 Subject: [PATCH 2/3] TD-3838 Returns null int instead of zero and comments id manipulation code --- .../DataServices/CourseContentService.cs | 4 ++-- .../LearningMenuController/LearningMenuController.cs | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs b/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs index 62f7c8adfd..c2c464a531 100644 --- a/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs +++ b/DigitalLearningSolutions.Data/DataServices/CourseContentService.cs @@ -269,13 +269,13 @@ FROM Sessions AS S1 WITH (NOLOCK) { try { - return connection.QueryFirst( + return connection.QueryFirst( @"SELECT COALESCE((SELECT ProgressID FROM Progress WHERE CandidateID = @candidateId AND CustomisationID = @customisationId AND SystemRefreshed = 0 - AND RemovedDate IS NULL), 0) AS ProgressId", + AND RemovedDate IS NULL), NULL) AS ProgressId", new { candidateId, customisationId } ); } diff --git a/DigitalLearningSolutions.Web/Controllers/LearningMenuController/LearningMenuController.cs b/DigitalLearningSolutions.Web/Controllers/LearningMenuController/LearningMenuController.cs index 453c5b0e97..ff6f5c0df9 100644 --- a/DigitalLearningSolutions.Web/Controllers/LearningMenuController/LearningMenuController.cs +++ b/DigitalLearningSolutions.Web/Controllers/LearningMenuController/LearningMenuController.cs @@ -85,10 +85,11 @@ public IActionResult Index(int customisationId) var sectionId = courseContent.Sections.First().Id; return RedirectToAction("Section", "LearningMenu", new { customisationId, sectionId }); } - if (UniqueIdManipulationDetected(candidateId, customisationId)) - { - return RedirectToAction("StatusCode", "LearningSolutions", new { code = 404 }); - } + // Unique Id Manipulation Detection is being disabled as part of work on TD-3838 - a bug created by its introduction + //if (UniqueIdManipulationDetected(candidateId, customisationId)) + //{ + // return RedirectToAction("StatusCode", "LearningSolutions", new { code = 404 }); + //} var progressId = courseContentService.GetOrCreateProgressId(candidateId, customisationId, centreId); if (progressId == null) { @@ -97,6 +98,7 @@ public IActionResult Index(int customisationId) $"Candidate id: {candidateId}, customisation id: {customisationId}, centre id: {centreId}"); return RedirectToAction("StatusCode", "LearningSolutions", new { code = 404 }); } + if (sessionService.StartOrUpdateDelegateSession(candidateId, customisationId, HttpContext.Session) > 0) { courseContentService.UpdateProgress(progressId.Value); From 95ec05a304f4cb66bdf82ecaa7618ad958fef2c1 Mon Sep 17 00:00:00 2001 From: kevwhitt-hee Date: Wed, 6 Mar 2024 14:23:06 +0000 Subject: [PATCH 3/3] Deprecates id manipulation tests --- .../Controllers/LearningMenu/IndexTests.cs | 87 ++++++++++--------- 1 file changed, 44 insertions(+), 43 deletions(-) diff --git a/DigitalLearningSolutions.Web.Tests/Controllers/LearningMenu/IndexTests.cs b/DigitalLearningSolutions.Web.Tests/Controllers/LearningMenu/IndexTests.cs index e0a5af7581..64681e6042 100644 --- a/DigitalLearningSolutions.Web.Tests/Controllers/LearningMenu/IndexTests.cs +++ b/DigitalLearningSolutions.Web.Tests/Controllers/LearningMenu/IndexTests.cs @@ -241,49 +241,50 @@ public void Index_unable_to_enrol_should_not_StartOrUpdate_course_sessions() // Then A.CallTo(() => sessionService.StartOrUpdateDelegateSession(A._, A._, A._)).MustNotHaveHappened(); } - - [Test] - public void Index_detects_id_manipulation_no_progress_id() - { - // Given - var expectedCourseContent = CourseContentHelper.CreateDefaultCourseContent(CustomisationId); - A.CallTo(() => courseContentService.GetCourseContent(CandidateId, CustomisationId)) - .Returns(expectedCourseContent); - A.CallTo(() => courseContentService.GetOrCreateProgressId(CandidateId, CustomisationId, CentreId)).Returns(10); - A.CallTo(() => courseContentService.GetProgressId(CandidateId, CustomisationId)).Returns(null); - - // When - var result = controller.Index(CustomisationId); - - // Then - result.Should() - .BeRedirectToActionResult() - .WithControllerName("LearningSolutions") - .WithActionName("StatusCode") - .WithRouteValue("code", 404); - } - - [Test] - public void Index_detects_id_manipulation_self_register_false() - { - // Given - var expectedCourseContent = CourseContentHelper.CreateDefaultCourseContent(CustomisationId); - A.CallTo(() => courseContentService.GetCourseContent(CandidateId, CustomisationId)) - .Returns(expectedCourseContent); - A.CallTo(() => courseContentService.GetOrCreateProgressId(CandidateId, CustomisationId, CentreId)).Returns(10); - A.CallTo(() => courseContentService.GetProgressId(CandidateId, CustomisationId)).Returns(null); - A.CallTo(() => courseDataService.GetSelfRegister(CustomisationId)).Returns(false); - - // When - var result = controller.Index(CustomisationId); - - // Then - result.Should() - .BeRedirectToActionResult() - .WithControllerName("LearningSolutions") - .WithActionName("StatusCode") - .WithRouteValue("code", 404); - } + //Deprecated in response to TD-3838 - a bug caused by this id manipulation detection functionality + + //[Test] + //public void Index_detects_id_manipulation_no_progress_id() + //{ + // // Given + // var expectedCourseContent = CourseContentHelper.CreateDefaultCourseContent(CustomisationId); + // A.CallTo(() => courseContentService.GetCourseContent(CandidateId, CustomisationId)) + // .Returns(expectedCourseContent); + // A.CallTo(() => courseContentService.GetOrCreateProgressId(CandidateId, CustomisationId, CentreId)).Returns(10); + // A.CallTo(() => courseContentService.GetProgressId(CandidateId, CustomisationId)).Returns(null); + + // // When + // var result = controller.Index(CustomisationId); + + // // Then + // result.Should() + // .BeRedirectToActionResult() + // .WithControllerName("LearningSolutions") + // .WithActionName("StatusCode") + // .WithRouteValue("code", 404); + //} + + //[Test] + //public void Index_detects_id_manipulation_self_register_false() + //{ + // // Given + // var expectedCourseContent = CourseContentHelper.CreateDefaultCourseContent(CustomisationId); + // A.CallTo(() => courseContentService.GetCourseContent(CandidateId, CustomisationId)) + // .Returns(expectedCourseContent); + // A.CallTo(() => courseContentService.GetOrCreateProgressId(CandidateId, CustomisationId, CentreId)).Returns(10); + // A.CallTo(() => courseContentService.GetProgressId(CandidateId, CustomisationId)).Returns(null); + // A.CallTo(() => courseDataService.GetSelfRegister(CustomisationId)).Returns(false); + + // // When + // var result = controller.Index(CustomisationId); + + // // Then + // result.Should() + // .BeRedirectToActionResult() + // .WithControllerName("LearningSolutions") + // .WithActionName("StatusCode") + // .WithRouteValue("code", 404); + //} [Test] public void Index_not_detects_id_manipulation_self_register_true()