Skip to content

Commit

Permalink
Adding extra verification method to support validating requests that …
Browse files Browse the repository at this point in the history 
…may contain arrays, solves #33
  • Loading branch information
@pfgray
pfgray committed Sep 18, 2017
1 parent f79187c commit 83cabd1
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 16 deletions.
11 changes: 11 additions & 0 deletions src/main/java/org/imsglobal/lti/launch/LtiLaunch.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.imsglobal.lti.launch;

import javax.servlet.http.HttpServletRequest;
import java.util.Collection;
import java.util.Map;

/**
Expand Down Expand Up @@ -38,6 +39,16 @@ public LtiLaunch(Map<String, String> parameters) {
this.toolConsumerInstanceGuid = parameters.get("tool_consumer_instance_guid");
}

public LtiLaunch(Collection<? extends Map.Entry> parameters) {
this.user = new LtiUser(parameters);
this.version = LtiOauthVerifier.getKey(parameters, "lti_version");
this.messageType = LtiOauthVerifier.getKey(parameters, "lti_message_type");
this.resourceLinkId = LtiOauthVerifier.getKey(parameters, "resource_link_id");
this.contextId = LtiOauthVerifier.getKey(parameters, "context_id");
this.launchPresentationReturnUrl = LtiOauthVerifier.getKey(parameters, "launch_presentation_return_url");
this.toolConsumerInstanceGuid = LtiOauthVerifier.getKey(parameters, "tool_consumer_instance_guid");
}

public LtiUser getUser() {
return user;
}
Expand Down
46 changes: 34 additions & 12 deletions src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
import net.oauth.server.OAuthServlet;

import javax.servlet.http.HttpServletRequest;
import java.util.Arrays;
import java.util.Map;
import java.util.*;
import java.util.logging.Logger;

/**
Expand All @@ -15,7 +14,7 @@
*/
public class LtiOauthVerifier implements LtiVerifier {

public static final String OAUTH_KEY_PARAMETER= "oauth_consumer_key";
public static final String OAUTH_KEY_PARAMETER = "oauth_consumer_key";

private final static Logger logger = Logger.getLogger(LtiOauthVerifier.class.getName());

Expand Down Expand Up @@ -60,16 +59,39 @@ public LtiVerificationResult verify(HttpServletRequest request, String secret) t
*/
@Override
public LtiVerificationResult verifyParameters(Map<String, String> parameters, String url, String method, String secret) throws LtiVerificationException {
OAuthMessage oam = new OAuthMessage(method, url, parameters.entrySet());
OAuthConsumer cons = new OAuthConsumer(null, parameters.get(OAUTH_KEY_PARAMETER), secret, null);
OAuthValidator oav = new SimpleOAuthValidator();
OAuthAccessor acc = new OAuthAccessor(cons);
return verifyParameters(parameters.entrySet(), url, method, secret);
}

try {
oav.validateMessage(oam, acc);
} catch (Exception e) {
return new LtiVerificationResult(false, LtiError.BAD_REQUEST, "Failed to validate: " + e.getLocalizedMessage() + ", Parameters: " + Arrays.toString(parameters.entrySet().toArray()));
@Override
public LtiVerificationResult verifyParameters(Collection<? extends Map.Entry> parameters, String url, String method, String secret) throws LtiVerificationException {
OAuthMessage oam = new OAuthMessage(method, url, parameters);
String key = getKey(parameters, OAUTH_KEY_PARAMETER);
if(key == null) {
return new LtiVerificationResult(false, LtiError.BAD_REQUEST, "No key found in LTI request with parameters: " + Arrays.toString(parameters.toArray()));
} else {
OAuthConsumer cons = new OAuthConsumer(null, key, secret, null);
OAuthValidator oav = new SimpleOAuthValidator();
OAuthAccessor acc = new OAuthAccessor(cons);

try {
oav.validateMessage(oam, acc);
} catch (Exception e) {
return new LtiVerificationResult(false, LtiError.BAD_REQUEST, "Failed to validate: " + e.getLocalizedMessage() + ", Parameters: " + Arrays.toString(parameters.toArray()));
}
return new LtiVerificationResult(true, new LtiLaunch(parameters));
}
}

/**
* Given a collection of parameters, return the first value for the given key.
* returns null if no entry is found with the given key.
*/
public static String getKey(Collection<? extends Map.Entry> parameters, String parameterName) {
for(Map.Entry<String, String> entry: parameters) {
if(entry.getKey().equals(parameterName)) {
return entry.getValue();
}
}
return new LtiVerificationResult(true, new LtiLaunch(parameters));
return null;
}
}
17 changes: 17 additions & 0 deletions src/main/java/org/imsglobal/lti/launch/LtiUser.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.imsglobal.lti.launch;

import javax.servlet.http.HttpServletRequest;
import java.util.Collection;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -33,6 +34,22 @@ public LtiUser(Map<String, String> parameters) {
}
}

public LtiUser(Collection<? extends Map.Entry> parameters) {
this.id = LtiOauthVerifier.getKey(parameters, "user_id");
this.roles = new LinkedList<>();
String parameterRoles = LtiOauthVerifier.getKey(parameters, "roles");
if(parameterRoles != null) {
for (String role : parameterRoles.split(",")) {
this.roles.add(role.trim());
}
}
}

public LtiUser(String id, List<String> roles) {
this.id = id;
this.roles = roles;
}

public String getId() {
return id;
}
Expand Down
26 changes: 22 additions & 4 deletions src/main/java/org/imsglobal/lti/launch/LtiVerifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@


import javax.servlet.http.HttpServletRequest;
import java.util.Collection;
import java.util.Map;

/**
Expand All @@ -24,13 +25,13 @@ public interface LtiVerifier {
* information about the request).
* @throws LtiVerificationException
*/
public LtiVerificationResult verify(HttpServletRequest request, String secret) throws LtiVerificationException;
LtiVerificationResult verify(HttpServletRequest request, String secret) throws LtiVerificationException;

/**
* This method will verify a list of properties (mapped
* by key &amp; value).
* @param parameters the parameters that will be verified. mapped by key &amp; value
* @param url the url this request was made at
* @param parameters the parameters that will be verified. mapped by key &amp; value. This should only include parameters explicitly included in the body (not the url).
* @param url The url this request was made at. The url passed should be the same as sent for the request (along with any parameters).
* @param method the method this url was requested with
* @param secret the secret to verify the propertihes with
* @return an LtiVerificationResult which will
Expand All @@ -39,6 +40,23 @@ public interface LtiVerifier {
* information about the request).
* @throws LtiVerificationException
*/
public LtiVerificationResult verifyParameters(Map<String, String> parameters, String url, String method, String secret) throws LtiVerificationException;
LtiVerificationResult verifyParameters(Map<String, String> parameters, String url, String method, String secret) throws LtiVerificationException;

/**
* This method will verify a list of properties (mapped
* by key &amp; value).
* @param parameters the parameters that will be verified. mapped by key &amp; value. This should only include parameters explicitly included in the body (not the url).
* The entries must be of type `Entry<String,String>`. If a specific key has multiple values (i.e. an array), each value must be in its own entry, each
* with the same key.
* @param url The url this request was made at. The url passed should be the same as sent for the request (along with any parameters).
* @param method the method this url was requested with
* @param secret the secret to verify the propertihes with
* @return an LtiVerificationResult which will
* contain information about the request (whether or
* not it is valid, and if it is valid, contextual
* information about the request).
* @throws LtiVerificationException
*/
LtiVerificationResult verifyParameters(Collection<? extends Map.Entry> parameters, String url, String method, String secret) throws LtiVerificationException;

}

5 comments on commit 83cabd1

@cayhorstmann
Copy link

Choose a reason for hiding this comment

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

The untyped Map.Entry might give callers uncomfortable warnings. I see no harm in accepting a Collection<Map.Entry<String, String>> or perhaps Collection<? extends Map.Entry<String, String>>.

More importantly, are you sure that the URL can contain query string params? I tried passing the entire URL to OAuthMessage, and validation failed. When I instead stripped off the query part and added the query string parameters to the map, it passed. I read through the source code in http://grepcode.com/file/repo1.maven.org/maven2/net.oauth.core/oauth-provider/20100527/net/oauth/SimpleOAuthValidator.java#SimpleOAuthValidator.validateSignature%28net.oauth.OAuthMessage%2Cnet.oauth.OAuthAccessor%29 and was unable to find any part where the query string parameters were stripped off. Of course I could be wrong--that code is not easy to follow.

This is all super frustrating to implementors who want to get their pedagogy into an LMS and are less interested in OAuth, so it would be good to get it right.

@pfgray
Copy link
Contributor Author

@pfgray pfgray commented on 83cabd1 Sep 28, 2017

Choose a reason for hiding this comment

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

Thanks for your input, I've updated the signature to be: Collection<? extends Map.Entry<String, String>>.

As far as the url containing query string parameters... I've done some preliminary testing and it turns out it works both ways! (in the sense that you can send the url with parameters to LtiOauthVerifier, or extract the url parameters and include them in the Collection<Entry<String, String>> params).

I'll update the javadoc in this interface to reflect this, and if I get some time tomorrow, I'll see if I can add some unit tests that reflect the above. I'm also going to create a PR for these changes that you can comment further on if you wish

@cayhorstmann
Copy link

Choose a reason for hiding this comment

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

Thanks, that code change looks good.

As for the URL parameters, I can give you a specific set of POST parameters and a URL with a request parameter that I get from Canvas. It does not validate when I pass it as-is to the OAuthValidator, but it does validate when I move the request parameter from the URL to the params collection. I don't want to post the shared secret here; contact me if you'd like to see that.

@pfgray
Copy link
Contributor Author

@pfgray pfgray commented on 83cabd1 Sep 30, 2017

Choose a reason for hiding this comment

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

Okay, I've added two unit tests, one where the url parameters are passed in via the url, and one where the url parameters are passed in via the collection of parameters.

Testing LTI signatures is always tricky because they are time-based (i.e. if you have a valid signature from a week ago, it is no longer valid since it should be considered "stale"). The only reliable way I've found is to create a signature via our Signer, and validate it with our Validator, but then again, this solution is a bit tautological... I'm open to ideas on how to make this better, but I fear a better solution would require changes to signpost.

Would you be able to construct a unit test (maybe with a different secret) that demonstrates what you're seeing, or possibly post the code that you're using to test?

@cayhorstmann
Copy link

Choose a reason for hiding this comment

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

I don't think this part uses signpost. You can specify what time interval is acceptable to you--just set it to 100 years. That should be ok for unit tests.

I sent you a program that shows the issue.

Please sign in to comment.