From 54ccad70e74c19336204c4cce2737ec37870ca07 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 20 Nov 2023 16:40:37 -0500 Subject: [PATCH 1/8] fix(MonitoredTripController): Protect computed MonitoredTrip fields from overwrite. --- .../api/MonitoredTripController.java | 22 +++++++++++-------- .../middleware/models/MonitoredTrip.java | 2 ++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/MonitoredTripController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/MonitoredTripController.java index 3a99a45c7..6a2d14054 100644 --- a/src/main/java/org/opentripplanner/middleware/controllers/api/MonitoredTripController.java +++ b/src/main/java/org/opentripplanner/middleware/controllers/api/MonitoredTripController.java @@ -144,18 +144,22 @@ MonitoredTrip preUpdateHook(MonitoredTrip monitoredTrip, MonitoredTrip preExisti MonitoredTripLocks.lockTripForUpdating(monitoredTrip, req); try { - preCreateOrUpdateChecks(monitoredTrip, req); - // Forbid the editing of certain values that are analyzed and set during the CheckMonitoredTrip job. - // For now, accomplish this by setting whatever the previous itinerary and journey state were in the preExisting - // trip. + // These include the itinerary, journeyState, and fields initially computed via processTripQueryParams + // that we don't need to recalculate. + // Note: There is no need to re-check for monitorability because the itinerary field cannot be changed. monitoredTrip.itinerary = preExisting.itinerary; monitoredTrip.journeyState = preExisting.journeyState; - - checkTripCanBeMonitored(monitoredTrip, req); - processTripQueryParams(monitoredTrip, req); - - // TODO: Update itinerary existence record when updating a trip. + monitoredTrip.itineraryExistence = preExisting.itineraryExistence; + monitoredTrip.tripTime = preExisting.tripTime; + monitoredTrip.queryParams = preExisting.queryParams; + monitoredTrip.userId = preExisting.userId; + monitoredTrip.from = preExisting.from; + monitoredTrip.to = preExisting.to; + monitoredTrip.arriveBy = preExisting.arriveBy; + + // TODO: Update itinerary existence record when updating a trip? + // (Currently, we let web requests change the monitored days regardless of existence.) // perform the database update here before releasing the lock to be sure that the record is updated in the // database before a CheckMonitoredTripJob analyzes the data diff --git a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java index 778ea00c8..a7311173c 100644 --- a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java +++ b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java @@ -60,6 +60,7 @@ public class MonitoredTrip extends Model { * separately) to the date and time within the query parameters. The reasoning is so that it doesn't have to be * extracted every time the trip requires checking. */ + @JsonIgnore public String tripTime; /** @@ -71,6 +72,7 @@ public class MonitoredTrip extends Model { /** * whether the trip is an arriveBy trip */ + @JsonIgnore public boolean arriveBy; /** From f8cdc36c212437da48f64da3bd9c533971a96c35 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 20 Nov 2023 16:48:14 -0500 Subject: [PATCH 2/8] fix(AbstractUserController): Prevent some OtpUser fields from being written from web request. --- .../controllers/api/AbstractUserController.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/AbstractUserController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/AbstractUserController.java index d15dc274e..01860b629 100644 --- a/src/main/java/org/opentripplanner/middleware/controllers/api/AbstractUserController.java +++ b/src/main/java/org/opentripplanner/middleware/controllers/api/AbstractUserController.java @@ -139,8 +139,14 @@ U preUpdateHook(U user, U preExistingUser, Request req) { } // Include select attributes from existingOtpUser marked @JsonIgnore and - // that are not set in otpUser. + // that are not set in otpUser, and other attributes that should not be modifiable + // using web requests. otpUser.smsConsentDate = existingOtpUser.smsConsentDate; + otpUser.email = existingOtpUser.email; + otpUser.auth0UserId = existingOtpUser.auth0UserId; + otpUser.isDataToolsUser = existingOtpUser.isDataToolsUser; + otpUser.pushDevices = existingOtpUser.pushDevices; + } return user; } From f096879ec4b9ff5f4b934a0d0b4973369b6404ce Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 20 Nov 2023 17:21:59 -0500 Subject: [PATCH 3/8] test(GetMonitoredTripsTest): Add test around not overwriting fields. --- .../api/GetMonitoredTripsTest.java | 49 +++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java index 5c58fa106..c39b418b7 100644 --- a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java +++ b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java @@ -21,9 +21,9 @@ import org.opentripplanner.middleware.utils.HttpResponseValues; import org.opentripplanner.middleware.utils.JsonUtils; -import java.io.IOException; - import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assumptions.assumeTrue; import static org.opentripplanner.middleware.auth.Auth0Connection.restoreDefaultAuthDisabled; import static org.opentripplanner.middleware.auth.Auth0Connection.setAuthDisabled; @@ -55,12 +55,14 @@ public class GetMonitoredTripsTest extends OtpMiddlewareTestEnvironment { private static OtpUser multiOtpUser; private static final String MONITORED_TRIP_PATH = "api/secure/monitoredtrip"; + private static final String DUMMY_STRING = "ABCDxyz"; + /** * Create Otp and Admin user accounts. Create Auth0 account for just the Otp users. If * an Auth0 account is created for the admin user it will fail because the email address already exists. */ @BeforeAll - public static void setUp() throws IOException { + public static void setUp() { assumeTrue(IS_END_TO_END); setAuthDisabled(false); @@ -112,7 +114,7 @@ public void tearDownAfterTest() { * credentials. */ @Test - public void canGetOwnMonitoredTrips() throws Exception { + void canGetOwnMonitoredTrips() throws Exception { // Create a trip for the solo and the multi OTP user. createMonitoredTripAsUser(soloOtpUser); createMonitoredTripAsUser(multiOtpUser); @@ -138,6 +140,45 @@ public void canGetOwnMonitoredTrips() throws Exception { assertEquals(1, tripsFiltered.data.size()); } + @Test + void canPreserveTripFields() throws Exception { + // Create a trip for the solo OTP user. + createMonitoredTripAsUser(soloOtpUser); + + // Get trips for solo Otp user. + ResponseList soloTrips = getMonitoredTripsForUser(MONITORED_TRIP_PATH, soloOtpUser); + + MonitoredTrip originalTrip = soloTrips.data.get(0); + assertNotNull(originalTrip.itinerary); + assertNotNull(originalTrip.itineraryExistence); + // Can't really assert journeyState because itinerary checks will not be run for these tests. + assertNotEquals(DUMMY_STRING, originalTrip.tripTime); + assertNotEquals(DUMMY_STRING, originalTrip.queryParams); + assertNotEquals(DUMMY_STRING, originalTrip.userId); + assertNotNull(originalTrip.from); + assertNotNull(originalTrip.to); + + MonitoredTrip modifiedTrip = new MonitoredTrip(); + modifiedTrip.id = originalTrip.id; + modifiedTrip.tripTime = DUMMY_STRING; + modifiedTrip.queryParams = DUMMY_STRING; + modifiedTrip.userId = DUMMY_STRING; + + mockAuthenticatedRequest(MONITORED_TRIP_PATH, + JsonUtils.toJson(modifiedTrip), + soloOtpUser, + HttpMethod.PUT + ); + + MonitoredTrip updatedTrip = Persistence.monitoredTrips.getById(originalTrip.id); + assertEquals(updatedTrip.itinerary.startTime, originalTrip.itinerary.startTime); + assertEquals(updatedTrip.tripTime, originalTrip.tripTime); + assertEquals(updatedTrip.queryParams, originalTrip.queryParams); + assertEquals(updatedTrip.userId, originalTrip.userId); + assertEquals(updatedTrip.from.name, originalTrip.from.name); + assertEquals(updatedTrip.to.name, originalTrip.to.name); + } + /** * Helper method to get trips for user. */ From 9f1a4c1467a1e0a217ac9351af66c86c6fdf8c25 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 20 Nov 2023 17:35:54 -0500 Subject: [PATCH 4/8] test(GetNonitoredTripsTest): Delete trip after creation in tests. --- .../middleware/controllers/api/GetMonitoredTripsTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java index c39b418b7..630096260 100644 --- a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java +++ b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java @@ -138,6 +138,9 @@ void canGetOwnMonitoredTrips() throws Exception { ); // Just the trip for Otp user 2 will be returned. assertEquals(1, tripsFiltered.data.size()); + + Persistence.monitoredTrips.removeById(soloTrips.data.get(0).id); + Persistence.monitoredTrips.removeById(multiTrips.data.get(0).id); } @Test @@ -177,6 +180,8 @@ void canPreserveTripFields() throws Exception { assertEquals(updatedTrip.userId, originalTrip.userId); assertEquals(updatedTrip.from.name, originalTrip.from.name); assertEquals(updatedTrip.to.name, originalTrip.to.name); + + Persistence.monitoredTrips.removeById(originalTrip.id); } /** From 9023ba3813171dab0f1f02877dc0fb9f1f50e69d Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 20 Nov 2023 18:22:55 -0500 Subject: [PATCH 5/8] fix(MonitoredTrip): Remove JsonIgnore from tripTime, refactor tests. --- .../middleware/models/MonitoredTrip.java | 1 - .../controllers/api/GetMonitoredTripsTest.java | 16 ++++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java index a7311173c..e441907d9 100644 --- a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java +++ b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java @@ -60,7 +60,6 @@ public class MonitoredTrip extends Model { * separately) to the date and time within the query parameters. The reasoning is so that it doesn't have to be * extracted every time the trip requires checking. */ - @JsonIgnore public String tripTime; /** diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java index 630096260..6ccbc63d7 100644 --- a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java +++ b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java @@ -2,6 +2,7 @@ import com.auth0.exception.Auth0Exception; import com.auth0.json.mgmt.users.User; +import com.mongodb.BasicDBObject; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.junit.jupiter.api.AfterAll; @@ -107,6 +108,12 @@ public static void tearDown() { @AfterEach public void tearDownAfterTest() { OtpTestUtils.resetOtpMocks(); + BasicDBObject soloFilter = new BasicDBObject(); + soloFilter.put("userId", soloOtpUser.id); + Persistence.monitoredTrips.removeFiltered(soloFilter); + BasicDBObject multiFilter = new BasicDBObject(); + multiFilter.put("userId", multiOtpUser.id); + Persistence.monitoredTrips.removeFiltered(multiFilter); } /** @@ -138,9 +145,6 @@ void canGetOwnMonitoredTrips() throws Exception { ); // Just the trip for Otp user 2 will be returned. assertEquals(1, tripsFiltered.data.size()); - - Persistence.monitoredTrips.removeById(soloTrips.data.get(0).id); - Persistence.monitoredTrips.removeById(multiTrips.data.get(0).id); } @Test @@ -150,8 +154,10 @@ void canPreserveTripFields() throws Exception { // Get trips for solo Otp user. ResponseList soloTrips = getMonitoredTripsForUser(MONITORED_TRIP_PATH, soloOtpUser); + // Expect only 1 trip for solo Otp user. + assertEquals(1, soloTrips.data.size()); - MonitoredTrip originalTrip = soloTrips.data.get(0); + MonitoredTrip originalTrip = soloTrips.data.get(0); assertNotNull(originalTrip.itinerary); assertNotNull(originalTrip.itineraryExistence); // Can't really assert journeyState because itinerary checks will not be run for these tests. @@ -180,8 +186,6 @@ void canPreserveTripFields() throws Exception { assertEquals(updatedTrip.userId, originalTrip.userId); assertEquals(updatedTrip.from.name, originalTrip.from.name); assertEquals(updatedTrip.to.name, originalTrip.to.name); - - Persistence.monitoredTrips.removeById(originalTrip.id); } /** From 3ecdfa9ec701ae48e7ee16b1fd841205104da9fa Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Mon, 20 Nov 2023 18:25:25 -0500 Subject: [PATCH 6/8] fix(MonitoredTrip): Remove JsonIgnore from arriveBy. --- .../org/opentripplanner/middleware/models/MonitoredTrip.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java index e441907d9..778ea00c8 100644 --- a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java +++ b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java @@ -71,7 +71,6 @@ public class MonitoredTrip extends Model { /** * whether the trip is an arriveBy trip */ - @JsonIgnore public boolean arriveBy; /** From 18d05dbffe6e2042f0652c779e988711f21cc798 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Tue, 21 Nov 2023 10:05:56 -0500 Subject: [PATCH 7/8] refactor(MonitoredTrip): Apply back JsonIgnore to arriveBy and tripTime, update tests. --- .../opentripplanner/middleware/models/MonitoredTrip.java | 2 ++ .../controllers/api/GetMonitoredTripsTest.java | 9 +++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java index 778ea00c8..a7311173c 100644 --- a/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java +++ b/src/main/java/org/opentripplanner/middleware/models/MonitoredTrip.java @@ -60,6 +60,7 @@ public class MonitoredTrip extends Model { * separately) to the date and time within the query parameters. The reasoning is so that it doesn't have to be * extracted every time the trip requires checking. */ + @JsonIgnore public String tripTime; /** @@ -71,6 +72,7 @@ public class MonitoredTrip extends Model { /** * whether the trip is an arriveBy trip */ + @JsonIgnore public boolean arriveBy; /** diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java index 6ccbc63d7..7bdbc97b8 100644 --- a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java +++ b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java @@ -152,12 +152,13 @@ void canPreserveTripFields() throws Exception { // Create a trip for the solo OTP user. createMonitoredTripAsUser(soloOtpUser); - // Get trips for solo Otp user. - ResponseList soloTrips = getMonitoredTripsForUser(MONITORED_TRIP_PATH, soloOtpUser); + BasicDBObject filter = new BasicDBObject(); + filter.put("userId", soloOtpUser.id); + // Expect only 1 trip for solo Otp user. - assertEquals(1, soloTrips.data.size()); + assertEquals(1, Persistence.monitoredTrips.getCountFiltered(filter)); - MonitoredTrip originalTrip = soloTrips.data.get(0); + MonitoredTrip originalTrip = Persistence.monitoredTrips.getOneFiltered(filter); assertNotNull(originalTrip.itinerary); assertNotNull(originalTrip.itineraryExistence); // Can't really assert journeyState because itinerary checks will not be run for these tests. From 677076ea49027a719156dbaca779751134333796 Mon Sep 17 00:00:00 2001 From: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com> Date: Tue, 21 Nov 2023 13:03:57 -0500 Subject: [PATCH 8/8] test(GetMonitoredTripTest): Add assertion for null tripTime field. --- .../middleware/controllers/api/GetMonitoredTripsTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java index 7bdbc97b8..e84f88766 100644 --- a/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java +++ b/src/test/java/org/opentripplanner/middleware/controllers/api/GetMonitoredTripsTest.java @@ -25,6 +25,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assumptions.assumeTrue; import static org.opentripplanner.middleware.auth.Auth0Connection.restoreDefaultAuthDisabled; import static org.opentripplanner.middleware.auth.Auth0Connection.setAuthDisabled; @@ -131,6 +132,10 @@ void canGetOwnMonitoredTrips() throws Exception { // Expect only 1 trip for solo Otp user. assertEquals(1, soloTrips.data.size()); + // Certain fields such as tripTime should be null when obtained from parsing JSON responses + // because of the @JsonIgnore annotation. + assertNull(soloTrips.data.get(0).tripTime); + // Get trips for multi Otp user/admin user. ResponseList multiTrips = getMonitoredTripsForUser(MONITORED_TRIP_PATH, multiOtpUser);