From 3f5f7349fb1354b33c321ec510585e7fe9db65d5 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Fri, 8 Nov 2024 09:55:05 +0000 Subject: [PATCH] refactor(PR feedback): Addressed issues with auth and dependent user reference --- .../controllers/api/OtpUserController.java | 18 ++++-------------- .../middleware/models/OtpUser.java | 2 +- .../tripmonitor/TrustedCompanion.java | 17 ++++++++++------- .../latest-spark-swagger-output.yaml | 9 ++------- .../api/OtpUserControllerTest.java | 19 +++++++++++++------ 5 files changed, 30 insertions(+), 35 deletions(-) diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java index c1445e513..ff3e43168 100644 --- a/src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java +++ b/src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java @@ -26,8 +26,7 @@ import static io.github.manusant.ss.descriptor.MethodDescriptor.path; import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.ACCEPT_KEY; -import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.EMAIL_PARAM; -import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.GET_DEPENDENT_MOBILITY_PROFILE_PATH; +import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.DEPENDENT_USER_ID; import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.USER_LOCALE; import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.ensureRelatedUserIntegrity; import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.manageAcceptDependentEmail; @@ -103,13 +102,12 @@ protected void buildEndpoint(ApiEndpoint baseEndpoint) { .withResponseType(OtpUser.class), TrustedCompanion::acceptDependent ) - .get(path(ROOT_ROUTE + String.format(GET_DEPENDENT_MOBILITY_PROFILE_PATH, ID_PARAM)) + .get(path(ROOT_ROUTE + "/getdependentmobilityprofile") .withDescription("Retrieve a dependent's mobility profile.") .withResponses(SwaggerUtils.createStandardResponses(MobilityProfile.class)) - .withPathParam().withName(ID_PARAM).withRequired(true).withDescription("The id of the related user.").and() - .withPathParam().withName(EMAIL_PARAM).withRequired(true).withDescription("The dependent's email address.").and() + .withPathParam().withName(DEPENDENT_USER_ID).withRequired(true).withDescription("A dependent's user id.").and() .withResponseType(MobilityProfile.class), - this::getDependentMobilityProfile, JsonUtils::toJson + TrustedCompanion::getDependentMobilityProfile, JsonUtils::toJson ) .get(path(ROOT_ROUTE + String.format(VERIFY_ROUTE_TEMPLATE, ID_PARAM, VERIFY_PATH, PHONE_PARAM)) .withDescription("Request an SMS verification to be sent to an OtpUser's phone number.") @@ -139,14 +137,6 @@ protected OtpUser getUserProfile(RequestingUser profile) { return otpUser; } - /** - * Extract the related user from the request and return a dependent's mobility profile if the right conditions - * are met. - */ - private MobilityProfile getDependentMobilityProfile(Request request, Response response) { - return TrustedCompanion.getDependentMobilityProfile(request, response, getEntityForId(request, response)); - } - /** * HTTP endpoint to send an SMS text to an {@link OtpUser}'s phone number with a verification code. This is used * during user signup (or if a user wishes to change their notification preferences to use a new un-verified phone diff --git a/src/main/java/org/opentripplanner/middleware/models/OtpUser.java b/src/main/java/org/opentripplanner/middleware/models/OtpUser.java index 79de4104c..d9d798ecd 100644 --- a/src/main/java/org/opentripplanner/middleware/models/OtpUser.java +++ b/src/main/java/org/opentripplanner/middleware/models/OtpUser.java @@ -90,7 +90,7 @@ public enum Notification { /** Companions and observers of this user. */ public List relatedUsers = new ArrayList<>(); - /** Users that are dependent on this user. */ + /** A list of users (their ids only) that are dependent on this user. */ public List dependents = new ArrayList<>(); /** This user's name */ diff --git a/src/main/java/org/opentripplanner/middleware/tripmonitor/TrustedCompanion.java b/src/main/java/org/opentripplanner/middleware/tripmonitor/TrustedCompanion.java index e722edb8c..2b1285b53 100644 --- a/src/main/java/org/opentripplanner/middleware/tripmonitor/TrustedCompanion.java +++ b/src/main/java/org/opentripplanner/middleware/tripmonitor/TrustedCompanion.java @@ -4,6 +4,7 @@ import org.apache.logging.log4j.util.Strings; import org.eclipse.jetty.http.HttpStatus; import org.opentripplanner.middleware.OtpMiddlewareMain; +import org.opentripplanner.middleware.auth.Auth0Connection; import org.opentripplanner.middleware.i18n.Message; import org.opentripplanner.middleware.models.MobilityProfile; import org.opentripplanner.middleware.models.OtpUser; @@ -46,10 +47,10 @@ private TrustedCompanion() { public static final String ACCEPT_KEY = "acceptKey"; public static final String USER_LOCALE = "userLocale"; public static final String EMAIL_PARAM = "email"; + public static final String DEPENDENT_USER_ID = "dependentuserid"; /** Note: This path is excluded from security checks, see {@link OtpMiddlewareMain#initializeHttpEndpoints()}. */ public static final String ACCEPT_DEPENDENT_PATH = "api/secure/user/acceptdependent"; - public static final String GET_DEPENDENT_MOBILITY_PROFILE_PATH = "/:%s/getdependentmobilityprofile"; /** * Accept a request from another user to be their dependent. This will include both companions and observers. If @@ -243,17 +244,19 @@ public static void removeDependent(OtpUser dependent, RelatedUser relatedUser) { /** * Retrieve the mobility profile for a dependent providing the requesting user is a trusted companion. */ - public static MobilityProfile getDependentMobilityProfile(Request request, Response response, OtpUser relatedUser) { + public static MobilityProfile getDependentMobilityProfile(Request request, Response response) { + OtpUser relatedUser = Auth0Connection.getUserFromRequest(request).otpUser; + if (relatedUser == null) { logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, "Related user not provided or unknown."); } - var dependentEmail = HttpUtils.getQueryParamFromRequest(request, EMAIL_PARAM, false); - if (dependentEmail == null) { - logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, "Required dependent email address not provided."); + var dependentUserId = HttpUtils.getQueryParamFromRequest(request, DEPENDENT_USER_ID, false); + if (dependentUserId == null) { + logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, "Required dependent id not provided."); } - var dependentUser = Persistence.otpUsers.getOneFiltered(eq(EMAIL_PARAM, dependentEmail)); + var dependentUser = Persistence.otpUsers.getById(dependentUserId); if (dependentUser == null) { logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, "Dependent user unknown."); } @@ -263,7 +266,7 @@ public static MobilityProfile getDependentMobilityProfile(Request request, Respo logMessageAndHalt( request, HttpStatus.FORBIDDEN_403, - String.format("Related user is not a trusted companion for %s!", dependentEmail) + String.format("Related user is not a trusted companion for dependent with id: %s!", dependentUserId) ); } else { return dependentUser.mobilityProfile; diff --git a/src/main/resources/latest-spark-swagger-output.yaml b/src/main/resources/latest-spark-swagger-output.yaml index a229bb08e..1e91844c1 100644 --- a/src/main/resources/latest-spark-swagger-output.yaml +++ b/src/main/resources/latest-spark-swagger-output.yaml @@ -1675,17 +1675,12 @@ paths: description: "An error occurred while performing the request. Contact an\ \ API administrator for more information." examples: {} - /api/secure/user/{id}/getdependentmobilityprofile: + /api/secure/user/getdependentmobilityprofile: get: tags: - "api/secure/user" description: "Retrieve a dependent's mobility profile." - parameters: - - name: "id" - in: "path" - description: "The id of the related user." - required: true - type: "string" + parameters: [] responses: "200": description: "Successful operation" diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java index 689e42c73..e6589f861 100644 --- a/src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java +++ b/src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java @@ -1,5 +1,6 @@ package org.opentripplanner.middleware.controllers.api; +import com.auth0.json.mgmt.users.User; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.junit.jupiter.api.AfterAll; @@ -33,11 +34,14 @@ import static org.junit.jupiter.api.Assumptions.assumeTrue; import static org.opentripplanner.middleware.auth.Auth0Connection.restoreDefaultAuthDisabled; import static org.opentripplanner.middleware.auth.Auth0Connection.setAuthDisabled; +import static org.opentripplanner.middleware.auth.Auth0Users.createAuth0UserForEmail; +import static org.opentripplanner.middleware.testutils.ApiTestUtils.TEMP_AUTH0_USER_PASSWORD; import static org.opentripplanner.middleware.testutils.ApiTestUtils.getMockHeaders; import static org.opentripplanner.middleware.testutils.ApiTestUtils.makeGetRequest; import static org.opentripplanner.middleware.testutils.ApiTestUtils.makeRequest; import static org.opentripplanner.middleware.testutils.ApiTestUtils.mockAuthenticatedGet; import static org.opentripplanner.middleware.testutils.PersistenceTestUtils.deleteOtpUser; +import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.DEPENDENT_USER_ID; public class OtpUserControllerTest extends OtpMiddlewareTestEnvironment { private static final String INITIAL_PHONE_NUMBER = "+15555550222"; // Fake US 555 number. @@ -73,7 +77,13 @@ public static void setUp() throws Exception { dependentUserTwo = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("dependent-two")); relatedUserThree = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("related-user-three")); dependentUserThree = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("dependent-three")); + relatedUserFour = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("related-user-four")); + + User auth0User = createAuth0UserForEmail(relatedUserFour.email, TEMP_AUTH0_USER_PASSWORD); + relatedUserFour.auth0UserId = auth0User.getId(); + Persistence.otpUsers.replace(relatedUserFour.id, relatedUserFour); + dependentUserFour = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("dependent-four")); } @@ -273,12 +283,10 @@ void canRemoveUserFromRelatedUsersList() throws Exception { @Test void canGetDependentMobilityProfile() throws Exception { - setAuthDisabled(true); - String path = String.format( - "api/secure/user/%s/getdependentmobilityprofile?email=%s", - relatedUserFour.id, - dependentUserFour.email + "api/secure/user/getdependentmobilityprofile?%s=%s", + DEPENDENT_USER_ID, + dependentUserFour.id ); HttpResponseValues responseValues = makeGetRequest(path, getMockHeaders(relatedUserFour)); @@ -295,7 +303,6 @@ void canGetDependentMobilityProfile() throws Exception { assertEquals(HttpStatus.OK_200, responseValues.status); assertEquals(JsonUtils.toJson(mobilityProfile), responseValues.responseBody); - setAuthDisabled(false); } /**