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

Refactor api user check auth0 #84

Merged
merged 36 commits into from
Nov 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e3f5ea9
refactor(auth): WIP modify auth check for API users
landonreed Sep 22, 2020
385e20b
refactor(Merged dev branch): Merged dev branch and fixed merge conflicts
Oct 1, 2020
07b78c2
refactor(Refactor of Api user auth): Various updates to auth Api user…
Oct 2, 2020
1f60f1e
refactor(Additional auth check): Auth0 account will only be deleted i…
Oct 2, 2020
cd7a668
refactor(Authenticate Api users with Auth0. New endpoint for Api user…
Oct 9, 2020
c469e2c
refactor(latest-spark-swagger-output.yaml): Updated spark swagger out…
Oct 9, 2020
0bf0e18
refactor(latest-spark-swagger-output.yaml): Correctly updated swagger…
Oct 9, 2020
36855ca
refactor(Fixed merge conflicts): Merged dev branch and fixed conflicts
Oct 12, 2020
ca3e5ce
refactor(Addressed PR feedback): Addressed PR feedback
Oct 13, 2020
b7758a8
refactor(Updated spark swagger output): Updated spark swagger output …
Oct 13, 2020
777e218
refactor(Addressed PR feedback and included update for issue #81): Ad…
Oct 15, 2020
4bd181a
refactor(Fixed merge conflicts): Merged dev and fixed conflicts
Oct 15, 2020
7685977
refactor(Addressed PR feedback): Addressed PR feedback
Oct 16, 2020
d94f0e5
refactor(Addressed PR feedback): Minor chanages to GetMonitoredTripsT…
Oct 16, 2020
7e54d28
refactor(Created issue with API key): An API key was added to the abs…
Oct 16, 2020
6e2df15
refactor(Addressed PR feedback): Addressed PR feedback
Oct 19, 2020
efad45f
refactor(Addressed PR feedback): Addressed PR feedback
Oct 23, 2020
d989ff9
refactor(Auth0Users): Replaced Auth0 vars
Oct 23, 2020
befa0c1
refactor(Auth0Users): Added end to end check for auth0 vars
Oct 23, 2020
ce30456
refactor(Addressed PR feedback): Addressed PR feedback
Oct 29, 2020
382c977
Merge branch 'dev' into refactor-api-user-check-auth0
Oct 29, 2020
790e3fe
refactor(OtpUserControllerTest): Fixed issue with mock auth request
Oct 29, 2020
ae7bc47
refactor(Comment update and explict use of auth path): Comment update…
Oct 30, 2020
3954147
refactor(Addressed PR feedback): Addressed PR feedback
Nov 2, 2020
86581bb
refactor(Addressed PR feedback): Addressed PR feedback
Nov 3, 2020
feef7f5
refactor(Addressed edge case with matching otp and api users when pro…
Nov 5, 2020
6b504c2
refactor(OtpRequestProcessor): Added additional OTP user check and TODO
Nov 5, 2020
7729deb
refactor(OtpRequestProcessor): Minor update so ApiUser can still make…
Nov 5, 2020
bb80a15
refactor(TestUtils): rename request methods, remove mockHeaders arg
landonreed Nov 6, 2020
1b57450
refactor: fix tests and add restoreDefaultAuthDisabled
landonreed Nov 6, 2020
6edd37d
test(JsonUtils): fix responseList parser method
landonreed Nov 6, 2020
ec7d369
Merge pull request #96 from ibi-group/refactor-api-user-check-auth0-ltr
Nov 9, 2020
b16534a
refactor: Addressed PR feedback
Nov 9, 2020
31aea84
refactor: Addressed PR feedback
Nov 10, 2020
1c00d7a
refactor(Fixed merge conflicts): Fixed merge conflicts with dev
Nov 10, 2020
9072f1e
refactor(Fixed failed unit tests): Fixed failed unit tests as a resul…
Nov 10, 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
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import org.opentripplanner.middleware.docs.PublicApiDocGenerator;
import org.opentripplanner.middleware.persistence.Persistence;
import org.opentripplanner.middleware.tripMonitor.jobs.MonitorAllTripsJob;
import org.opentripplanner.middleware.utils.ConfigUtils;
import org.opentripplanner.middleware.utils.HttpUtils;
import org.opentripplanner.middleware.utils.Scheduler;
import org.slf4j.Logger;
Expand All @@ -30,6 +29,9 @@
import java.util.concurrent.TimeUnit;

import static javax.ws.rs.core.MediaType.APPLICATION_JSON;
import static org.opentripplanner.middleware.controllers.api.ApiUserController.API_USER_PATH;
import static org.opentripplanner.middleware.controllers.api.ApiUserController.AUTHENTICATE_PATH;
import static org.opentripplanner.middleware.utils.ConfigUtils.loadConfig;
import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;

/**
Expand All @@ -43,7 +45,7 @@ public class OtpMiddlewareMain {

public static void main(String[] args) throws IOException, InterruptedException {
// Load configuration.
ConfigUtils.loadConfig(args);
loadConfig(args);

// Connect to MongoDB.
Persistence.initialize();
Expand Down Expand Up @@ -131,9 +133,12 @@ private static void initializeHttpEndpoints() throws IOException, InterruptedExc
return "OK";
});

// Security checks for admin and /secure/ endpoints.
// Security checks for admin and /secure/ endpoints. Excluding /authenticate so that API users can obtain a
// bearer token to authenticate against all other /secure/ endpoints.
spark.before(API_PREFIX + "/secure/*", ((request, response) -> {
if (!request.requestMethod().equals("OPTIONS")) Auth0Connection.checkUser(request);
if (!request.requestMethod().equals("OPTIONS") && !request.pathInfo().endsWith(API_USER_PATH + AUTHENTICATE_PATH)) {
Auth0Connection.checkUser(request);
}
}));
spark.before(API_PREFIX + "admin/*", ((request, response) -> {
if (!request.requestMethod().equals("OPTIONS")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,13 @@
import com.auth0.jwt.interfaces.DecodedJWT;
import com.fasterxml.jackson.core.JsonProcessingException;
import org.eclipse.jetty.http.HttpStatus;
import org.opentripplanner.middleware.controllers.api.ApiUserController;
import org.opentripplanner.middleware.controllers.api.OtpUserController;
import org.opentripplanner.middleware.models.AbstractUser;
import org.opentripplanner.middleware.models.ApiUser;
import org.opentripplanner.middleware.models.OtpUser;
import org.opentripplanner.middleware.persistence.Persistence;
import org.opentripplanner.middleware.utils.JsonUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import spark.HaltException;
Expand All @@ -22,11 +26,8 @@

import java.security.interfaces.RSAPublicKey;

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.ConfigUtils.getConfigPropertyAsText;
import static org.opentripplanner.middleware.utils.ConfigUtils.hasConfigProperty;
import static org.opentripplanner.middleware.utils.JsonUtils.getPOJOFromRequestBody;
import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;

/**
Expand Down Expand Up @@ -54,9 +55,10 @@ public static void checkUser(Request req) {
if (isAuthDisabled()) {
// If in a development or testing environment, assign a mock profile of an admin user to the request
// attribute and skip authentication.
addUserToRequest(req, Auth0UserProfile.createTestUser(req));
addUserToRequest(req, RequestingUser.createTestUser(req));
return;
}
// Admin and OTP users authenticated by Bearer token
String token = getTokenFromRequest(req);
// Handle getting the verifier outside of the below verification try/catch, which is intended to catch issues
// with the client request. (getVerifier has its own exception/halt handling).
Expand All @@ -65,7 +67,7 @@ public static void checkUser(Request req) {
// for downstream controllers to check permissions.
try {
DecodedJWT jwt = verifier.verify(token);
Auth0UserProfile profile = new Auth0UserProfile(jwt);
RequestingUser profile = new RequestingUser(jwt);
if (!isValidUser(profile)) {
if (isCreatingSelf(req, profile)) {
// If creating self, no user account is required (it does not exist yet!). Note: creating an
Expand All @@ -74,7 +76,7 @@ public static void checkUser(Request req) {
LOG.info("New user is creating self. OK to proceed without existing user object for auth0UserId");
} else {
// Otherwise, if no valid user is found, halt the request.
logMessageAndHalt(req, HttpStatus.NOT_FOUND_404, "Unknown user.");
logMessageAndHalt(req, HttpStatus.NOT_FOUND_404, "No user found in database associated with the provided auth token.");
}
}
// The user attribute is used on the server side to check user permissions and does not have all of the
Expand All @@ -94,22 +96,22 @@ public static void checkUser(Request req) {
/**
* Check for POST requests that are creating an {@link AbstractUser} (a proxy for OTP/API users).
*/
private static boolean isCreatingSelf(Request req, Auth0UserProfile profile) {
private static boolean isCreatingSelf(Request req, RequestingUser profile) {
String uri = req.uri();
String method = req.requestMethod();
// Check that this is a POST request.
if (method.equalsIgnoreCase("POST")) {
// Next, check that an OtpUser or ApiUser is being created (an admin must rely on another admin to create
// them).
boolean creatingOtpUser = uri.endsWith(OTP_USER_PATH);
boolean creatingApiUser = uri.endsWith(API_USER_PATH);
boolean creatingOtpUser = uri.endsWith(OtpUserController.OTP_USER_PATH);
boolean creatingApiUser = uri.endsWith(ApiUserController.API_USER_PATH);
if (creatingApiUser || creatingOtpUser) {
// Get the correct user class depending on request path.
Class<? extends AbstractUser> userClass = creatingApiUser ? ApiUser.class : OtpUser.class;
try {
// Next, get the user object from the request body, verifying that the Auth0UserId matches between
// requester and the new user object.
AbstractUser user = getPOJOFromRequestBody(req, userClass);
AbstractUser user = JsonUtils.getPOJOFromRequestBody(req, userClass);
return profile.auth0UserId.equals(user.auth0UserId);
} catch (JsonProcessingException e) {
LOG.warn("Could not parse user object from request.", e);
Expand All @@ -124,16 +126,15 @@ public static boolean isAuthHeaderPresent(Request req) {
return authHeader != null;
}


/**
* Assign user to request and check that the user is an admin.
*/
public static void checkUserIsAdmin(Request req, Response res) {
// Check auth token in request (and add user object to request).
checkUser(req);
// Check that user object is present and is admin.
Auth0UserProfile user = Auth0Connection.getUserFromRequest(req);
if (!isUserAdmin(user)) {
RequestingUser user = Auth0Connection.getUserFromRequest(req);
if (!user.isAdmin()) {
logMessageAndHalt(
req,
HttpStatus.UNAUTHORIZED_401,
Expand All @@ -143,23 +144,36 @@ public static void checkUserIsAdmin(Request req, Response res) {
}

/**
* Check if the incoming user is an admin user
* Check that the API key used in the incoming request is associated with the matching {@link ApiUser} (which is
* determined from the Authorization header).
*/
public static boolean isUserAdmin(Auth0UserProfile user) {
return user != null && user.adminUser != null;
//FIXME: Move this check into existing auth checks so it would be carried out automatically prior to any
// business logic. Consider edge cases where a user can be both an API user and OTP user.
public static void ensureApiUserHasApiKey(Request req) {
RequestingUser requestingUser = getUserFromRequest(req);
String apiKeyValueFromHeader = req.headers("x-api-key");
if (requestingUser.apiUser == null ||
apiKeyValueFromHeader == null ||
!requestingUser.apiUser.hasApiKeyValue(apiKeyValueFromHeader)) {
// If API user not found, log message and halt.
logMessageAndHalt(
req,
HttpStatus.FORBIDDEN_403,
"API key not linked to an API user.");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This check appears to have been replaced with RequestingUser#isAdmin. Are we certain that the user is never null (i.e., are we avoiding NPEs when user.isAdmin() is invoked)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fine. All API calls (except to /auth) call Auth0Connection.checkUser which in-turn calls Auth0Connection.isValidUser. This does a null pointer check on the requesting user object.


/**
* Add user profile to Spark Request object
*/
public static void addUserToRequest(Request req, Auth0UserProfile user) {
public static void addUserToRequest(Request req, RequestingUser user) {
req.attribute("user", user);
}

/**
* Get user profile from Spark Request object
*/
public static Auth0UserProfile getUserFromRequest(Request req) {
public static RequestingUser getUserFromRequest(Request req) {
return req.attribute("user");
}

Expand Down Expand Up @@ -216,7 +230,8 @@ private static JWTVerifier getVerifier(Request req, String token) {
}

public static boolean getDefaultAuthDisabled() {
return hasConfigProperty("DISABLE_AUTH") && "true".equals(getConfigPropertyAsText("DISABLE_AUTH"));
return hasConfigProperty("DISABLE_AUTH") &&
"true".equals(getConfigPropertyAsText("DISABLE_AUTH"));
}

/**
Expand All @@ -227,38 +242,56 @@ public static boolean isAuthDisabled() {
}

/**
* Override the current {@link #authDisabled} value.
* Override the current {@link #authDisabled} value. This is used principally for setting up test environments that
* require auth to be disabled.
*/
public static void setAuthDisabled(boolean authDisabled) {
Auth0Connection.authDisabled = authDisabled;
}

/**
* Restore default {@link #authDisabled} value. This is used principally for tearing down test environments that
* require auth to be disabled.
*/
public static void restoreDefaultAuthDisabled() {
setAuthDisabled(getDefaultAuthDisabled());
}

/**
* Confirm that the user exists in at least one of the MongoDB user collections.
*/
private static boolean isValidUser(Auth0UserProfile profile) {
private static boolean isValidUser(RequestingUser 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.
* Confirm that the user's actions are on their items if not admin. In the case of an Api user confirm that the
* user's actions, on Otp users, are Otp users they created initially.
*/
public static void isAuthorized(String userId, Request request) {
Auth0UserProfile profile = getUserFromRequest(request);
RequestingUser requestingUser = getUserFromRequest(request);
// let admin do anything
if (profile.adminUser != null) {
if (requestingUser.adminUser != null) {
return;
}
// If userId is defined, it must be set to a value associated with the user.
// If userId is defined, it must be set to a value associated with a user.
if (userId != null) {
if (profile.otpUser != null && profile.otpUser.id.equals(userId)) {
if (requestingUser.otpUser != null && requestingUser.otpUser.id.equals(userId)) {
// Otp user requesting their item.
return;
}
if (profile.apiUser != null && profile.apiUser.id.equals(userId)) {
if (requestingUser.isThirdPartyUser() && requestingUser.apiUser.id.equals(userId)) {
// Api user requesting their item.
return;
}
if (requestingUser.isThirdPartyUser()) {
// Api user potentially requesting an item on behalf of an Otp user they created.
OtpUser otpUser = Persistence.otpUsers.getById(userId);
if (requestingUser.canManageEntity(otpUser)) {
return;
}
}
}
logMessageAndHalt(request, HttpStatus.FORBIDDEN_403, "Unauthorized access.");
}

}
42 changes: 23 additions & 19 deletions src/main/java/org/opentripplanner/middleware/auth/Auth0Users.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,17 @@
import com.auth0.client.auth.AuthAPI;
import com.auth0.client.mgmt.ManagementAPI;
import com.auth0.exception.Auth0Exception;
import com.auth0.json.auth.TokenHolder;
import com.auth0.json.mgmt.jobs.Job;
import com.auth0.json.mgmt.users.User;
import com.auth0.net.AuthRequest;
import com.fasterxml.jackson.core.JsonProcessingException;
import org.apache.commons.validator.routines.EmailValidator;
import org.eclipse.jetty.http.HttpStatus;
import org.opentripplanner.middleware.bugsnag.BugsnagReporter;
import org.opentripplanner.middleware.models.AbstractUser;
import org.opentripplanner.middleware.persistence.TypedPersistence;
import org.opentripplanner.middleware.utils.HttpUtils;
import org.opentripplanner.middleware.utils.JsonUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import spark.Request;
Expand All @@ -26,8 +27,6 @@

import static com.mongodb.client.model.Filters.eq;
import static org.opentripplanner.middleware.utils.ConfigUtils.getConfigPropertyAsText;
import static org.opentripplanner.middleware.utils.HttpUtils.httpRequestRawResponse;
import static org.opentripplanner.middleware.utils.JsonUtils.getSingleNodeValueFromJSON;
import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;

/**
Expand All @@ -39,12 +38,11 @@ public class Auth0Users {
// This client/secret pair is for making requests for an API access token used with the Management API.
private static final String AUTH0_API_CLIENT = getConfigPropertyAsText("AUTH0_API_CLIENT");
private static final String AUTH0_API_SECRET = getConfigPropertyAsText("AUTH0_API_SECRET");
private static final String AUTH0_CLIENT_ID = getConfigPropertyAsText("AUTH0_CLIENT_ID");
private static final String AUTH0_CLIENT_SECRET = getConfigPropertyAsText("AUTH0_CLIENT_SECRET");
private static final String DEFAULT_CONNECTION_TYPE = "Username-Password-Authentication";
private static final String DEFAULT_AUDIENCE = "https://otp-middleware";
private static final String MANAGEMENT_API_VERSION = "v2";
private static final String SEARCH_API_VERSION = "v3";
public static final String API_PATH = "/api/" + MANAGEMENT_API_VERSION;

/**
* Cached API token so that we do not have to request a new one each time a Management API request is made.
*/
Expand All @@ -53,8 +51,8 @@ public class Auth0Users {
private static final AuthAPI authAPI = new AuthAPI(AUTH0_DOMAIN, AUTH0_API_CLIENT, AUTH0_API_SECRET);

/**
* Creates a standard user for the provided email address. Defaults to a random UUID password and connection type of
* {@link #DEFAULT_CONNECTION_TYPE}.
* Creates a standard user for the provided email address, password (Defaulted to a random UUID) and connection type
* of {@link #DEFAULT_CONNECTION_TYPE}.
*/
public static User createAuth0UserForEmail(String email) throws Auth0Exception {
return createAuth0UserForEmail(email, UUID.randomUUID().toString());
Expand Down Expand Up @@ -242,33 +240,39 @@ private static String getAuth0Url() {
}

/**
* Get an Auth0 oauth token for use in mocking user requests by using the Auth0 'Call Your API Using Resource Owner
* Password Flow' approach. Auth0 setup can be reviewed here: https://auth0.com/docs/flows/call-your-api-using-resource-owner-password-flow.
* If the user is successfully validated by Auth0 a bearer access token is returned, which is extracted and returned
* to the caller. In all other cases, null is returned.
* Get an Auth0 oauth token response for use in mocking user requests by using the Auth0 'Call Your API Using Resource
* Owner Password Flow' approach. Auth0 setup can be reviewed here: https://auth0.com/docs/flows/call-your-api-using-resource-owner-password-flow.
* If token response is returned to calling methods for evaluation.
*/
public static String getAuth0Token(String username, String password) throws JsonProcessingException {
public static HttpResponse<String> getCompleteAuth0TokenResponse(String username, String password) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Both downstream methods calling this method simply parse it into a TokenHolder. Why not just handle that within this method and change its return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because `ApiUserController.authenticateAuth0User' will provide the status code (produced by Auth0) to the user in the event of failure. I could update this to a fixed status code (403?) and then make your suggested update?

if (Auth0Connection.isAuthDisabled()) return null;
String body = String.format(
"grant_type=password&username=%s&password=%s&audience=%s&scope=&client_id=%s&client_secret=%s",
username,
password,
"https://otp-middleware", // must match an API identifier
AUTH0_CLIENT_ID, // Auth0 application client ID
AUTH0_CLIENT_SECRET // Auth0 application client secret
DEFAULT_AUDIENCE, // must match an API identifier
AUTH0_API_CLIENT, // Auth0 application client ID
AUTH0_API_SECRET // Auth0 application client secret
);

HttpResponse<String> response = httpRequestRawResponse(
return HttpUtils.httpRequestRawResponse(
URI.create(String.format("https://%s/oauth/token", AUTH0_DOMAIN)),
1000,
HttpUtils.REQUEST_METHOD.POST,
Collections.singletonMap("content-type", "application/x-www-form-urlencoded"),
body
);
}

/**
* Extract from a complete Auth0 token just the access token. If the token is not available, return null instead.
*/
public static String getAuth0AccessToken(String username, String password) {
HttpResponse<String> response = getCompleteAuth0TokenResponse(username, password);
if (response == null || response.statusCode() != HttpStatus.OK_200) {
LOG.error("Cannot obtain Auth0 token for user {}. response: {} - {}", username, response.statusCode(), response.body());
return null;
}
return getSingleNodeValueFromJSON("access_token", response.body());
TokenHolder token = JsonUtils.getPOJOFromJSON(response.body(), TokenHolder.class);
return (token == null) ? null : token.getAccessToken();
}
}
Loading