From 83cabd1c00e751abdb2f633bf2080c31d1ca875b Mon Sep 17 00:00:00 2001 From: Paul Gray Date: Mon, 18 Sep 2017 13:49:55 -0400 Subject: [PATCH 1/3] Adding extra verification method to support validating requests that may contain arrays, solves #33 --- .../org/imsglobal/lti/launch/LtiLaunch.java | 11 +++++ .../lti/launch/LtiOauthVerifier.java | 46 ++++++++++++++----- .../org/imsglobal/lti/launch/LtiUser.java | 17 +++++++ .../org/imsglobal/lti/launch/LtiVerifier.java | 26 +++++++++-- 4 files changed, 84 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/imsglobal/lti/launch/LtiLaunch.java b/src/main/java/org/imsglobal/lti/launch/LtiLaunch.java index 4e322f1..4784da4 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiLaunch.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiLaunch.java @@ -1,6 +1,7 @@ package org.imsglobal.lti.launch; import javax.servlet.http.HttpServletRequest; +import java.util.Collection; import java.util.Map; /** @@ -38,6 +39,16 @@ public LtiLaunch(Map parameters) { this.toolConsumerInstanceGuid = parameters.get("tool_consumer_instance_guid"); } + public LtiLaunch(Collection 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; } diff --git a/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java b/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java index 09027f6..bdb0a45 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java @@ -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; /** @@ -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()); @@ -60,16 +59,39 @@ public LtiVerificationResult verify(HttpServletRequest request, String secret) t */ @Override public LtiVerificationResult verifyParameters(Map 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 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 parameters, String parameterName) { + for(Map.Entry entry: parameters) { + if(entry.getKey().equals(parameterName)) { + return entry.getValue(); + } } - return new LtiVerificationResult(true, new LtiLaunch(parameters)); + return null; } } diff --git a/src/main/java/org/imsglobal/lti/launch/LtiUser.java b/src/main/java/org/imsglobal/lti/launch/LtiUser.java index 8131459..bb35a46 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiUser.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiUser.java @@ -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; @@ -33,6 +34,22 @@ public LtiUser(Map parameters) { } } + public LtiUser(Collection 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 roles) { + this.id = id; + this.roles = roles; + } + public String getId() { return id; } diff --git a/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java b/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java index 1a21c23..1c13dd9 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java @@ -2,6 +2,7 @@ import javax.servlet.http.HttpServletRequest; +import java.util.Collection; import java.util.Map; /** @@ -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 & value). - * @param parameters the parameters that will be verified. mapped by key & value - * @param url the url this request was made at + * @param parameters the parameters that will be verified. mapped by key & 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 @@ -39,6 +40,23 @@ public interface LtiVerifier { * information about the request). * @throws LtiVerificationException */ - public LtiVerificationResult verifyParameters(Map parameters, String url, String method, String secret) throws LtiVerificationException; + LtiVerificationResult verifyParameters(Map parameters, String url, String method, String secret) throws LtiVerificationException; + + /** + * This method will verify a list of properties (mapped + * by key & value). + * @param parameters the parameters that will be verified. mapped by key & value. This should only include parameters explicitly included in the body (not the url). + * The entries must be of type `Entry`. 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 parameters, String url, String method, String secret) throws LtiVerificationException; } From bfae21807d7a8a8681869845956e73d7db331218 Mon Sep 17 00:00:00 2001 From: Paul Gray Date: Mon, 25 Sep 2017 21:40:42 -0400 Subject: [PATCH 2/3] Adding better typed sign & verify params signature --- .../imsglobal/lti/launch/LtiOauthSigner.java | 19 ++++++++++++------- .../lti/launch/LtiOauthVerifier.java | 2 +- .../org/imsglobal/lti/launch/LtiSigner.java | 16 ++++++++++++++++ .../org/imsglobal/lti/launch/LtiVerifier.java | 2 +- .../lti/launch/LtiVerifierAndSignerTest.java | 16 +++++++++++----- 5 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/imsglobal/lti/launch/LtiOauthSigner.java b/src/main/java/org/imsglobal/lti/launch/LtiOauthSigner.java index 2884f26..7e21177 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiOauthSigner.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiOauthSigner.java @@ -20,6 +20,7 @@ import java.net.URLEncoder; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.util.Collection; import java.util.HashMap; import java.util.Map; @@ -64,17 +65,21 @@ public HttpRequest sign(HttpRequest request, String key, String secret) throws L @Override public Map signParameters(Map parameters, String key, String secret, String url, String method) throws LtiSigningException { - OAuthMessage oam = new OAuthMessage(method, url, parameters.entrySet()); + Map signedParameters = new HashMap<>(); + for(Map.Entry param : signParameters(parameters.entrySet(), key, secret, url, method)){ + signedParameters.put(param.getKey(), param.getValue()); + } + return signedParameters; + } + + @Override + public Collection> signParameters(Collection> parameters, String key, String secret, String url, String method) throws LtiSigningException { + OAuthMessage oam = new OAuthMessage(method, url, parameters); OAuthConsumer cons = new OAuthConsumer(null, key, secret, null); OAuthAccessor acc = new OAuthAccessor(cons); try { oam.addRequiredParameters(acc); - - Map signedParameters = new HashMap<>(); - for(Map.Entry param : oam.getParameters()){ - signedParameters.put(param.getKey(), param.getValue()); - } - return signedParameters; + return oam.getParameters(); } catch (OAuthException |IOException |URISyntaxException e) { throw new LtiSigningException("Error signing LTI request.", e); } diff --git a/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java b/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java index bdb0a45..203f1e7 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiOauthVerifier.java @@ -63,7 +63,7 @@ public LtiVerificationResult verifyParameters(Map parameters, St } @Override - public LtiVerificationResult verifyParameters(Collection parameters, String url, String method, String secret) throws LtiVerificationException { + public LtiVerificationResult verifyParameters(Collection> 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) { diff --git a/src/main/java/org/imsglobal/lti/launch/LtiSigner.java b/src/main/java/org/imsglobal/lti/launch/LtiSigner.java index 14ee829..d3e623a 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiSigner.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiSigner.java @@ -2,6 +2,7 @@ import org.apache.http.HttpRequest; +import java.util.Collection; import java.util.Map; /** @@ -44,4 +45,19 @@ public interface LtiSigner { */ public Map signParameters(Map parameters, String key, String secret, String url, String method) throws LtiSigningException; + /** + * This method will return a list of signed parameters. + * Once returned, adding new parameters or changing the + * body will invalidate the signature. This method will + * overwrite reserved parameters from the underlying + * specification. For example, if you are using the Oauth + * implementation, oauth_signature will be removed + * & replaced with the generated signature from the properties. + * @param parameters the parameters that will be signed. mapped by key & value + * @param key the key that will be added to the request. + * @param secret the secret to be sign the parameters with + * @return a map of signed parameters (including the signature) + * @throws LtiSigningException + */ + public Collection> signParameters(Collection> parameters, String key, String secret, String url, String method) throws LtiSigningException; } diff --git a/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java b/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java index 1c13dd9..0b91dab 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java @@ -57,6 +57,6 @@ public interface LtiVerifier { * information about the request). * @throws LtiVerificationException */ - LtiVerificationResult verifyParameters(Collection parameters, String url, String method, String secret) throws LtiVerificationException; + LtiVerificationResult verifyParameters(Collection> parameters, String url, String method, String secret) throws LtiVerificationException; } diff --git a/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java b/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java index f7500aa..614e0f0 100644 --- a/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java +++ b/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java @@ -8,6 +8,7 @@ import static org.junit.Assert.*; import java.net.URI; +import java.util.*; /** @@ -58,16 +59,21 @@ public void verifierShouldVerifyCorrectlySignedLtiGetServiceRequests() throws Ex } @Test - public void verifierShouldRejectIncorrectlySignedLtiGetServiceRequests() throws Exception { + public void verifierShouldDoStuff() throws Exception { String key = "key"; String secret = "secret"; - HttpGet ltiServiceGetRequest = new HttpGet(new URI("http://example.com/test")); - signer.sign(ltiServiceGetRequest, key, secret); - LtiVerificationResult result = verifier.verify(new MockHttpGet(ltiServiceGetRequest), "anotherWrongSecret"); + Collection> myEntries = Arrays.asList( + new AbstractMap.SimpleEntry("hm", "okasy"), + new AbstractMap.SimpleEntry("hm", "asdf"), + new AbstractMap.SimpleEntry("wat", "asdfasdff") + ); - assertFalse(result.getSuccess()); + Collection signedEntries = signer.signParameters(myEntries, key, secret, "http://example.com/test", "GET"); + LtiVerificationResult result = verifier.verifyParameters(signedEntries, "http://example.com/test", "GET", secret); + + assertTrue(result.getSuccess()); } } From 1856cea0df43546541a7f082d3da3626718ca2ab Mon Sep 17 00:00:00 2001 From: Paul Gray Date: Wed, 27 Sep 2017 20:45:00 -0400 Subject: [PATCH 3/3] Adding unit tests for LtiVerifier and url parameters and updating documentation --- .../org/imsglobal/lti/launch/LtiVerifier.java | 21 +++-- .../lti/launch/LtiOauthVerifierTest.java | 77 +++++++++++++++++++ .../lti/launch/LtiVerifierAndSignerTest.java | 18 ----- 3 files changed, 92 insertions(+), 24 deletions(-) create mode 100644 src/test/java/org/imsglobal/lti/launch/LtiOauthVerifierTest.java diff --git a/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java b/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java index 0b91dab..3c4f49e 100644 --- a/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java +++ b/src/main/java/org/imsglobal/lti/launch/LtiVerifier.java @@ -31,9 +31,16 @@ public interface LtiVerifier { * This method will verify a list of properties (mapped * by key & value). * @param parameters the parameters that will be verified. mapped by key & 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 parameters the parameters that will be verified. mapped by key & value. This should include parameters explicitly included in the body + * (if this collection also includes parameters from the url, those parameters must be omitted in the `url` parameter to this method). + * 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 if the request includes parameters, + * they may be optionally included here, or in the collection passed to parameters. * @param method the method this url was requested with - * @param secret the secret to verify the propertihes with + * @param secret the secret to verify the properties with * @return an LtiVerificationResult which will * contain information about the request (whether or * not it is valid, and if it is valid, contextual @@ -45,12 +52,14 @@ public interface LtiVerifier { /** * This method will verify a list of properties (mapped * by key & value). - * @param parameters the parameters that will be verified. mapped by key & value. This should only include parameters explicitly included in the body (not the url). - * The entries must be of type `Entry`. If a specific key has multiple values (i.e. an array), each value must be in its own entry, each + * @param parameters the parameters that will be verified. mapped by key & value. This should include parameters explicitly included in the body + * (if this collection also includes parameters from the url, those parameters must be omitted in the `url` parameter to this method). + * 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 url The url this request was made at. The url passed should be the same as sent for the request if the request includes parameters, + * they may be optionally included here, or in the collection passed to parameters. * @param method the method this url was requested with - * @param secret the secret to verify the propertihes with + * @param secret the secret to verify the properties with * @return an LtiVerificationResult which will * contain information about the request (whether or * not it is valid, and if it is valid, contextual diff --git a/src/test/java/org/imsglobal/lti/launch/LtiOauthVerifierTest.java b/src/test/java/org/imsglobal/lti/launch/LtiOauthVerifierTest.java new file mode 100644 index 0000000..17f0dab --- /dev/null +++ b/src/test/java/org/imsglobal/lti/launch/LtiOauthVerifierTest.java @@ -0,0 +1,77 @@ +package org.imsglobal.lti.launch; + +import org.apache.http.client.methods.HttpPost; +import org.codehaus.jackson.map.ser.std.IterableSerializer; +import org.junit.Test; + +import java.net.URI; +import java.util.*; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * Created by paul on 9/27/17. + */ +public class LtiOauthVerifierTest { + + + LtiVerifier verifier = new LtiOauthVerifier(); + LtiSigner signer = new LtiOauthSigner(); + + + @Test + public void verifierShouldAllowUrlParametersInUrlParam() throws Exception { + + String baseUrl = "http://example.com/test"; + String urlParams = "?bestLanguage=java"; + + String key = "key"; + String secret = "secret"; + + Collection> myEntries = Arrays.asList( + new AbstractMap.SimpleEntry("hm", "okasy"), + new AbstractMap.SimpleEntry("hm", "asdf"), + new AbstractMap.SimpleEntry("wat", "asdfasdff") + ); + + Collection signedParameters = signer.signParameters(myEntries, key, secret, baseUrl + urlParams, "GET"); + + + // including the parameters in the url should yield success + LtiVerificationResult successResult = verifier.verifyParameters(signedParameters, baseUrl + urlParams, "GET", secret); + assertTrue(successResult.getSuccess()); + + // omitting the parameters in the url should yield failure + LtiVerificationResult failResult = verifier.verifyParameters(signedParameters, baseUrl, "GET", secret); + assertFalse(failResult.getSuccess()); + } + + @Test + public void verifierShouldAllowUrlParametersInEntryMap() throws Exception { + + String baseUrl = "http://example.com/test"; + String urlParams = "?bestLanguage=java"; + + String key = "key"; + String secret = "secret"; + + List> myEntries = Arrays.asList( + new AbstractMap.SimpleEntry("hm", "okasy"), + new AbstractMap.SimpleEntry("hm", "asdf"), + new AbstractMap.SimpleEntry("wat", "asdfasdff") + ); + + Collection signedParameters = signer.signParameters(myEntries, key, secret, baseUrl + urlParams, "GET"); + + List signedParamsWithUrlParams = new ArrayList<>(signedParameters); + signedParamsWithUrlParams.add(new AbstractMap.SimpleEntry("bestLanguage", "java")); + // including the url parameters in the entries map should yield success + LtiVerificationResult successResult = verifier.verifyParameters(signedParamsWithUrlParams, baseUrl, "GET", secret); + assertTrue(successResult.getSuccess()); + + // omitting the url parameters in the entries map and omitting them in the url should yield failure + LtiVerificationResult failResult = verifier.verifyParameters(signedParameters, baseUrl, "GET", secret); + assertFalse(failResult.getSuccess()); + } +} diff --git a/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java b/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java index 614e0f0..c1c613a 100644 --- a/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java +++ b/src/test/java/org/imsglobal/lti/launch/LtiVerifierAndSignerTest.java @@ -58,22 +58,4 @@ public void verifierShouldVerifyCorrectlySignedLtiGetServiceRequests() throws Ex assertTrue(result.getSuccess()); } - @Test - public void verifierShouldDoStuff() throws Exception { - - String key = "key"; - String secret = "secret"; - - Collection> myEntries = Arrays.asList( - new AbstractMap.SimpleEntry("hm", "okasy"), - new AbstractMap.SimpleEntry("hm", "asdf"), - new AbstractMap.SimpleEntry("wat", "asdfasdff") - ); - - Collection signedEntries = signer.signParameters(myEntries, key, secret, "http://example.com/test", "GET"); - LtiVerificationResult result = verifier.verifyParameters(signedEntries, "http://example.com/test", "GET", secret); - - assertTrue(result.getSuccess()); - } - }