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 12 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 @@ -14,6 +14,9 @@
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.ConfigUtils;
import org.opentripplanner.middleware.utils.JsonUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import spark.HaltException;
Expand All @@ -24,10 +27,6 @@

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;

/**
* This handles verifying the Auth0 token passed in the auth header (e.g., Authorization: Bearer MY_TOKEN of Spark HTTP
Expand All @@ -54,9 +53,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 +65,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,27 +74,27 @@ 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.");
JsonUtils.logMessageAndHalt(req, HttpStatus.NOT_FOUND_404, "Auth0 auth - Unknown user.");
br648 marked this conversation as resolved.
Show resolved Hide resolved
}
}
// 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);
JsonUtils.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");
JsonUtils.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) {
private static boolean isCreatingSelf(Request req, RequestingUser profile) {
String uri = req.uri();
String method = req.requestMethod();
// Check that this is a POST request.
Expand All @@ -109,7 +109,7 @@ private static boolean isCreatingSelf(Request req, Auth0UserProfile profile) {
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,17 +124,16 @@ 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);
RequestingUser user = Auth0Connection.getUserFromRequest(req);
if (!isUserAdmin(user)) {
logMessageAndHalt(
JsonUtils.logMessageAndHalt(
req,
HttpStatus.UNAUTHORIZED_401,
"User is not authorized to perform administrative action"
Expand All @@ -143,23 +142,24 @@ public static void checkUserIsAdmin(Request req, Response res) {
}

/**
* Check if the incoming user is an admin user
* Check if the incoming user is an admin user. To be classed as an admin user, the user must not be any other user
* type.
*/
public static boolean isUserAdmin(Auth0UserProfile user) {
return user != null && user.adminUser != null;
public static boolean isUserAdmin(RequestingUser user) {
return user != null && user.adminUser != null && user.apiUser == null && user.otpUser == null;
}
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 All @@ -168,19 +168,19 @@ public static Auth0UserProfile getUserFromRequest(Request req) {
*/
private static String getTokenFromRequest(Request req) {
if (!isAuthHeaderPresent(req)) {
logMessageAndHalt(req, 401, "Authorization header is missing.");
JsonUtils.logMessageAndHalt(req, 401, "Authorization header is missing.");
}

// Check that auth header is present and formatted correctly (Authorization: Bearer [token]).
final String authHeader = req.headers("Authorization");
String[] parts = authHeader.split(" ");
if (parts.length != 2 || !"bearer".equals(parts[0].toLowerCase())) {
logMessageAndHalt(req, 401, String.format("Authorization header is malformed: %s", authHeader));
JsonUtils.logMessageAndHalt(req, 401, String.format("Authorization header is malformed: %s", authHeader));
}
// Retrieve token from auth header.
String token = parts[1];
if (token == null) {
logMessageAndHalt(req, 401, "Could not find authorization token");
JsonUtils.logMessageAndHalt(req, 401, "Could not find authorization token");
}
return token;
}
Expand All @@ -192,7 +192,7 @@ private static String getTokenFromRequest(Request req) {
private static JWTVerifier getVerifier(Request req, String token) {
if (verifier == null) {
try {
final String domain = "https://" + getConfigPropertyAsText("AUTH0_DOMAIN") + "/";
final String domain = "https://" + ConfigUtils.getConfigPropertyAsText("AUTH0_DOMAIN") + "/";
br648 marked this conversation as resolved.
Show resolved Hide resolved
JwkProvider provider = new UrlJwkProvider(domain);
// Decode the token.
DecodedJWT jwt = JWT.decode(token);
Expand All @@ -209,14 +209,15 @@ private static JWTVerifier getVerifier(Request req, String token) {
.build();
} catch (IllegalStateException | NullPointerException | JwkException e) {
LOG.error("Auth0 verifier configured incorrectly.");
logMessageAndHalt(req, 500, "Server authentication configured incorrectly.", e);
JsonUtils.logMessageAndHalt(req, 500, "Server authentication configured incorrectly.", e);
}
}
return verifier;
}

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

/**
Expand All @@ -236,29 +237,38 @@ public static void setAuthDisabled(boolean authDisabled) {
/**
* 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 (otpUser != null && requestingUser.apiUser.id.equals(otpUser.applicationId)) {
return;
}
}
}
logMessageAndHalt(request, HttpStatus.FORBIDDEN_403, "Unauthorized access.");
JsonUtils.logMessageAndHalt(request, HttpStatus.FORBIDDEN_403, "Unauthorized access.");
}

}
Loading