Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cascade delete #64

Merged
merged 21 commits into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0a79dd6
fix(delete): add cascade delete for collections
landonreed Aug 24, 2020
c94a52b
test(e2e): add api user flow test to simulate 3rd party apps
landonreed Aug 24, 2020
3fbc850
refactor(Fixed merge conflict): Merged dev and fixed conflicts
Aug 25, 2020
24acb0b
refactor(ApiUserFlowTest): Api and otp related tests using Auth0 auth…
Aug 27, 2020
3d70aa2
refactor(Merge conflict): Fixed merge conflict with dev branch
Aug 27, 2020
993a7ba
refactor(PR updates): Addressed PR feedback
Aug 28, 2020
1fa79e7
refactor(ApiUserController): Corrected issue introduced fixing merge …
Aug 28, 2020
ab88847
refactor(Auth0): do not check valid user if creating self
landonreed Sep 1, 2020
6ff1fd1
refactor: update tests and simplify mock otp
landonreed Sep 1, 2020
6e6e3a0
refactor: remove unused imports
landonreed Sep 1, 2020
39bfee4
refactor(OtpUserController): handle missing API user on OtpUser#create
landonreed Sep 1, 2020
84608b7
refactor: minor tweaks, add more setup docs
landonreed Sep 1, 2020
942c804
refactor(ApiController): change delete return type
landonreed Sep 1, 2020
4ae3945
refactor(user#delete): address PR comments
landonreed Sep 1, 2020
869713d
refactor(Auth0): fix isCreatingSelf check; handle null throwable in b…
landonreed Sep 2, 2020
e4cdb9a
Merge branch 'dev' into cascade-delete
landonreed Sep 2, 2020
d4ecd7a
refactor(DateTimeUtils): use currentTimeMillis util
landonreed Sep 2, 2020
167af9b
test(trip-history): clean up otpuser after each test
landonreed Sep 2, 2020
32f0340
Merge branch 'dev' into cascade-delete
landonreed Sep 3, 2020
69c562f
test: evaluate authDisabled after config is loaded
landonreed Sep 3, 2020
4f43685
Merge branch 'dev' into cascade-delete
landonreed Sep 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,26 @@ mvn package
java -jar target/otp-middleware.jar configurations/default/env.yml
```

## OTP Server Proxy Setup
## Configuration

### Auth0

TODO: Add Auth0 setup instructions.

### OTP Server Proxy Setup
The follow parameters are used to interact with an OTP server.

| Parameter | Description | Example |
| --- | --- | --- |
| OTP_API_ROOT | This is the address of the OTP server, including the root path to the OTP API, to which all OTP related requests will be sent to. | http://otp-server.example.com/otp |
| OTP_PLAN_ENDPOINT | This defines the plan endpoint part of the requesting URL. If a request is made to this, the assumption is that a plan request has been made and that the response should be processed accordingly. | /plan |

## Bugsnag Configuration Parameters
### Bugsnag

Bugsnag is used to report error events that occur within the otp-middleware application or
OpenTripPlanner components that it is monitoring.

#### Bugsnag Configuration Parameters

These values can used as defined here (were applicable), or commented out so the default values are used. Parameters
that don't have default values (N/A) can be obtained my following the steps in the next section.
Expand All @@ -61,8 +72,8 @@ that don't have default values (N/A) can be obtained my following the steps in t
| BUGSNAG_REPORTING_WINDOW_IN_DAYS | 14 | The number of days in the past to start retrieving event information. |


## Bugsnag Setup
Where default parameters can not be used, these steps describe how to obtain each compulsory parameter.
#### Bugsnag Setup
Where default parameters cannot be used, these steps describe how to obtain each compulsory parameter.

##### BUGSNAG_API_KEY
A bugsnag API key is a key that is unique to an individual Bugsnag user. This key can be obtained by logging into
Expand All @@ -79,3 +90,26 @@ From here, click on the organization name and then copy the name from the pop-up
A Bugsnag project identifier key is unique to a Bugsnag project and allows errors to be saved against it. This key can
be obtained by logging into Bugsnag (https://app.bugsnag.com), clicking on Projects (left side menu) and selecting the
required project. Once selected, the notifier API key is presented.

## Testing

### End-to-end (E2E)

In order to run E2E tests, specific configuration and environment variables must be used.

#### Auth0
The standard Auth0 configuration can be used for the server settings. However, some additional settings must be applied
in order for the server to get an oath token from Auth0 for creating authenticated requests. A private application
should be created for use in Auth0 following these instructions: https://auth0.com/docs/flows/call-your-api-using-resource-owner-password-flow

One special note is that the default directory must be set at the Account/tenant level. Otherwise, authorization will not
be successful. See this StackOverflow answer for more info: https://stackoverflow.com/a/43835563/915811

The special E2E client settings should be defined in `env.yml`:

| Parameter | Default | Description |
| --- | --- | --- |
| AUTH0_CLIENT_ID | N/A | Special E2E application client ID. |
| AUTH0_CLIENT_SECRET | N/A | Special E2E application client secret. |
Comment on lines +112 to +113
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new variables should appear in env.yml.tmp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that's the case if they're only intended for e2e.


**Note:** Just to reiterate, these are different from the server application settings and are only needed for E2E testing.
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
import org.opentripplanner.middleware.controllers.api.AdminUserController;
import org.opentripplanner.middleware.controllers.api.ApiUserController;
import org.opentripplanner.middleware.controllers.api.BugsnagController;
import org.opentripplanner.middleware.controllers.api.OtpUserController;
import org.opentripplanner.middleware.controllers.api.LogController;
import org.opentripplanner.middleware.controllers.api.MonitoredTripController;
import org.opentripplanner.middleware.controllers.api.OtpUserController;
import org.opentripplanner.middleware.controllers.api.TripHistoryController;
import org.opentripplanner.middleware.otp.OtpRequestProcessor;
import org.opentripplanner.middleware.persistence.Persistence;
Expand All @@ -36,7 +36,7 @@ public class OtpMiddlewareMain {
private static final Logger LOG = LoggerFactory.getLogger(OtpMiddlewareMain.class);
private static final String DEFAULT_ENV = "configurations/default/env.yml";
private static JsonNode envConfig;
private static final String API_PREFIX = "/api/";
public static final String API_PREFIX = "/api/";
public static boolean inTestEnvironment = false;

public static void main(String[] args) throws IOException {
Expand Down Expand Up @@ -117,8 +117,8 @@ private static void initializeHttpEndpoints() throws IOException {
}

/**
* Load config files from either program arguments or (if no args specified) from
* default configuration file locations. Config fields are retrieved with getConfigProperty.
* Load config files from either program arguments or (if no args specified) from default configuration file
* locations. Config fields are retrieved with getConfigProperty.
*/
private static void loadConfig(String[] args) throws IOException {
FileInputStream envConfigStream;
Expand Down Expand Up @@ -203,16 +203,23 @@ public static String getConfigPropertyAsText(String name, String defaultValue) {
* value if the config value is not defined (null) or cannot be converted to an int.
*/
public static int getConfigPropertyAsInt(String name, int defaultValue) {

int value = defaultValue;

try {
JsonNode node = getConfigProperty(name);
value = Integer.parseInt(node.asText());
if (node != null) {
value = Integer.parseInt(node.asText());
}
} catch (NumberFormatException | NullPointerException e) {
LOG.warn("Unable to parse {}. Using default: {}", name, defaultValue, e);
}
return value;
}

/**
* Returns true only if an environment variable exists and is set to "true".
*/
public static boolean getBooleanEnvVar(String var) {
String variable = System.getenv(var);
return variable != null && variable.equals("true");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,22 @@
import com.auth0.jwt.algorithms.Algorithm;
import com.auth0.jwt.exceptions.JWTVerificationException;
import com.auth0.jwt.interfaces.DecodedJWT;
import com.fasterxml.jackson.core.JsonProcessingException;
import org.eclipse.jetty.http.HttpStatus;
import org.opentripplanner.middleware.models.AbstractUser;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import spark.HaltException;
import spark.Request;
import spark.Response;

import java.security.interfaces.RSAPublicKey;

import static org.opentripplanner.middleware.OtpMiddlewareMain.getConfigPropertyAsText;
import static org.opentripplanner.middleware.OtpMiddlewareMain.hasConfigProperty;
import static org.opentripplanner.middleware.controllers.api.ApiUserController.API_USER_PATH;
import static org.opentripplanner.middleware.controllers.api.OtpUserController.OTP_USER_PATH;
import static org.opentripplanner.middleware.utils.JsonUtils.getPOJOFromRequestBody;
import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;

/**
Expand Down Expand Up @@ -53,18 +59,48 @@ public static void checkUser(Request req) {
try {
DecodedJWT jwt = verifier.verify(token);
Auth0UserProfile profile = new Auth0UserProfile(jwt);
if (!isValidUser(profile)) {
landonreed marked this conversation as resolved.
Show resolved Hide resolved
// If not a valid user (and not creating an OTP/API user for self), halt the request. Note: creating an
// admin user requires that the requester is an admin (checkUserIsAdmin must be passed), so this is not
// a concern for that method/controller.
if (!isCreatingSelf(req, profile)) {
logMessageAndHalt(req, HttpStatus.NOT_FOUND_404, "Unknown user.");
}
}
// The user attribute is used on the server side to check user permissions and does not have all of the
// fields that the raw Auth0 profile string does.
addUserToRequest(req, profile);
} catch (JWTVerificationException e) {
// Invalid signature/claims
logMessageAndHalt(req, 401, "Login failed to verify with our authorization provider.", e);
} catch (HaltException e) {
throw e;
} catch (Exception e) {
LOG.warn("Login failed to verify with our authorization provider.", e);
logMessageAndHalt(req, 401, "Could not verify user's token");
}
}

/**
* Check for POST requests that are creating an {@link AbstractUser} (a proxy for OTP/API users).
*/
private static boolean isCreatingSelf(Request req, Auth0UserProfile profile) {
// First, check the request method/path.
String uri = req.uri();
String method = req.requestMethod();
if (method.equalsIgnoreCase("POST") && (uri.endsWith(OTP_USER_PATH) || uri.endsWith(API_USER_PATH))) {
// Next, get the user object from the request body, verifying that the Auth0UserId matches.
try {
AbstractUser user = getPOJOFromRequestBody(req, AbstractUser.class);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A piece of code might still be missing here.
For a new signup from MOD UI, an exception is thrown:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `org.opentripplanner.middleware.models.AbstractUser` (no Creators, like default construct, exist): abstract types either need to be mapped to concrete types, have custom deserializer, or contain additional type information
 at [Source: (String)"{"auth0UserId":"auth0|5f****************",
<req.body() content below>}"; line: 1, column: 1]

req.body() returns something such as

{
"auth0UserId":"auth0|5f**************",
"email":"[email protected]",
"hasConsentedToTerms":true,
"notificationChannel":"email",
"phoneNumber":"",
"savedLocations":[],
"storeTripHistory":false
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easiest workaround is probably to pass ApiUser.class or OtpUser.class to the get POJO method depending on the path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fixed and tested. 869713d

return profile.auth0UserId.equals(user.auth0UserId);
} catch (JsonProcessingException e) {
LOG.warn("Could not parse user object from request.");
return false;
}
}
return false;
}

public static boolean isAuthHeaderPresent(Request req) {
final String authHeader = req.headers("Authorization");
return authHeader != null;
Expand Down Expand Up @@ -106,7 +142,7 @@ public static void addUserToRequest(Request req, Auth0UserProfile user) {
* Get user profile from Spark Request object
*/
public static Auth0UserProfile getUserFromRequest(Request req) {
return (Auth0UserProfile) req.attribute("user");
return req.attribute("user");
}

/**
Expand Down Expand Up @@ -169,40 +205,30 @@ public static boolean authDisabled() {
}

/**
* Confirm that the user exists
* Confirm that the user exists in at least one of the MongoDB user collections.
*/
private static Auth0UserProfile isValidUser(Request request) {

Auth0UserProfile profile = getUserFromRequest(request);
if (profile == null || (profile.adminUser == null && profile.otpUser == null && profile.apiUser == null)) {
logMessageAndHalt(request, HttpStatus.NOT_FOUND_404, "Unknown user.");
}

return profile;
private static boolean isValidUser(Auth0UserProfile profile) {
return profile != null && (profile.adminUser != null || profile.otpUser != null || profile.apiUser != null);
}

/**
* Confirm that the user's actions are on their items if not admin.
*/
public static void isAuthorized(String userId, Request request) {

Auth0UserProfile profile = isValidUser(request);

Auth0UserProfile profile = getUserFromRequest(request);
// let admin do anything
if (profile.adminUser != null) {
return;
}

// If userId is defined, it must be set to a value associated with the user.
if (userId != null) {
if (profile.otpUser != null && profile.otpUser.id.equals(userId)) {
return;
}

if (profile.apiUser != null && profile.apiUser.id.equals(userId)) {
return;
}
}

logMessageAndHalt(request, HttpStatus.FORBIDDEN_403, "Unauthorized access.");
}

Expand Down
Loading