Merge pull request #195 from ibi-group/prevent-overwriting-trip-user-…

…internal-fields

Prevent overwriting trip/user internal fields

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;
     }

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

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;
 
     /**

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;
@@ -21,9 +22,10 @@
 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.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;
@@ -55,12 +57,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);
 
@@ -105,14 +109,20 @@ 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);
     }
 
     /**
      * Create trips for two different Otp users and attempt to get both trips with Otp user that has 'enhanced' admin
      * 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);
@@ -122,6 +132,10 @@ public 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<MonitoredTrip> multiTrips = getMonitoredTripsForUser(MONITORED_TRIP_PATH, multiOtpUser);
 
@@ -138,6 +152,48 @@ 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);
+
+        BasicDBObject filter = new BasicDBObject();
+        filter.put("userId", soloOtpUser.id);
+
+        // Expect only 1 trip for solo Otp user.
+        assertEquals(1, Persistence.monitoredTrips.getCountFiltered(filter));
+
+        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.
+        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.
      */