Created OAuth2ExpressionParser that automatically wraps a Spring Expression

... with a method that provides meaningful errors.

This allows Spring Security OAuth to check for errors and throw Excepions when an error occurs
while ensuring that the exception is only thrown when necessary. For example, if an expression
has an or statment we do not want to throw the Exception if only part of the expression is false.

Fixes spring-atticgh-41, Fixes spring-atticgh-118

Author: rwinch

spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/expression/OAuth2ExpressionParser.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2002-2011 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.oauth2.provider.expression;
+
+import org.springframework.expression.Expression;
+import org.springframework.expression.ExpressionParser;
+import org.springframework.expression.ParseException;
+import org.springframework.expression.ParserContext;
+import org.springframework.util.Assert;
+
+/**
+ * <p>
+ * A custom {@link ExpressionParser} that automatically wraps SpEL expression with
+ * {@link OAuth2SecurityExpressionMethods#throwOnError(boolean)}. This makes it simple for users to specify an
+ * expression and then have it verified (providing errors) after the result of the expression is known.
+ * </p>
+ * <p>
+ * Note: The implication is that all expressions that are parsed must return a boolean result. This expectation is
+ * already true since Spring Security expects the result to be a boolean.
+ * </p>
+ * 
+ * @author Rob Winch
+ * 
+ */
+final class OAuth2ExpressionParser implements ExpressionParser {
+	private final ExpressionParser delegate;
+
+	public OAuth2ExpressionParser(ExpressionParser delegate) {
+		Assert.notNull(delegate, "delegate cannot be null");
+		this.delegate = delegate;
+	}
+
+	public Expression parseExpression(String expressionString) throws ParseException {
+		return delegate.parseExpression(wrapExpression(expressionString));
+	}
+
+	public Expression parseExpression(String expressionString, ParserContext context) throws ParseException {
+		return delegate.parseExpression(wrapExpression(expressionString), context);
+	}
+
+	private String wrapExpression(String expressionString) {
+		return "#oauth2.throwOnError(" + expressionString + ")";
+	}
+}

spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/expression/OAuth2MethodSecurityExpressionHandler.java

@@ -1,38 +1,36 @@
 package org.springframework.security.oauth2.provider.expression;
 
 import org.aopalliance.intercept.MethodInvocation;
+import org.springframework.expression.ExpressionParser;
 import org.springframework.expression.spel.support.StandardEvaluationContext;
 import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler;
 import org.springframework.security.core.Authentication;
 
 /**
+ * <p>
  * A security expression handler that can handle default method security expressions plus the set provided by
  * {@link OAuth2SecurityExpressionMethods} using the variable oauth2 to access the methods. For example, the expression
  * <code>#oauth2.clientHasRole('ROLE_ADMIN')</code> would invoke {@link OAuth2SecurityExpressionMethods#clientHasRole}
- *
+ * </p>
+ * <p>
+ * By default the {@link OAuth2ExpressionParser} is used. If this is undesirable one can inject their own
+ * {@link ExpressionParser} using {@link #setExpressionParser(ExpressionParser)}.
+ * </p>
+ * 
  * @author Dave Syer
  * @author Rob Winch
+ * @see OAuth2ExpressionParser
  */
 public class OAuth2MethodSecurityExpressionHandler extends DefaultMethodSecurityExpressionHandler {
-	
-	private boolean throwExceptionOnInvalidScope = true;
 
-	/**
-	 * Flag to determine the behaviour on access denied if the reason is . If set then we throw an
-	 * {@link InvalidScopeException} instead of returning true. This is unconventional for an access decision because it
-	 * vetos the other voters in the chain, but it enables us to pass a message to the caller with information about the
-	 * required scope.
-	 * 
-	 * @param throwException the flag to set (default true)
-	 */
-	public void setThrowExceptionOnInvalidScope(boolean throwException) {
-		this.throwExceptionOnInvalidScope = throwException;
+	public OAuth2MethodSecurityExpressionHandler() {
+		setExpressionParser(new OAuth2ExpressionParser(getExpressionParser()));
 	}
 
 	@Override
 	public StandardEvaluationContext createEvaluationContextInternal(Authentication authentication, MethodInvocation mi) {
 		StandardEvaluationContext ec = super.createEvaluationContextInternal(authentication, mi);
-		ec.setVariable("oauth2", new OAuth2SecurityExpressionMethods(authentication, throwExceptionOnInvalidScope));
+		ec.setVariable("oauth2", new OAuth2SecurityExpressionMethods(authentication));
 		return ec;
 	}
 }

spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/expression/OAuth2SecurityExpressionMethods.java

@@ -37,25 +37,33 @@ public class OAuth2SecurityExpressionMethods {
 
 	private Set<String> missingScopes = new LinkedHashSet<String>();
 
-	private boolean throwExceptionOnInvalidScope;
-
-	public OAuth2SecurityExpressionMethods(Authentication authentication, boolean throwExceptionOnInvalidScope) {
+	public OAuth2SecurityExpressionMethods(Authentication authentication) {
 		this.authentication = authentication;
-		this.throwExceptionOnInvalidScope = throwExceptionOnInvalidScope;
 	}
 
 	/**
-	 * Check if any scope decisions have been denied in the current context and throw an exception if so. Example usage:
+	 * Check if any scope decisions have been denied in the current context and throw an exception if so. This method
+	 * automatically wraps any expressions when using {@link OAuth2MethodSecurityExpressionHandler} or
+	 * {@link OAuth2WebSecurityExpressionHandler}.
+	 * 
+	 * OAuth2Example usage:
+	 * 
+	 * <pre>
+	 * access = &quot;#oauth2.hasScope('read') or (#oauth2.hasScope('other') and hasRole('ROLE_USER'))&quot;
+	 * </pre>
+	 * 
+	 * Will automatically be wrapped to ensure that explicit errors are propagated rather than a generic error when
+	 * returning false:
 	 * 
 	 * <pre>
-	 * access = &quot;#oauth2.sufficientScope(#oauth2.hasScope('read') or (#oauth2.hasScope('other') and hasRole('ROLE_USER')))&quot;
+	 * access = &quot;#oauth2.throwOnError(#oauth2.hasScope('read') or (#oauth2.hasScope('other') and hasRole('ROLE_USER'))&quot;
 	 * </pre>
 	 * 
 	 * @param decision the existing access decision
 	 * @return true if the OAuth2 token has one of these scopes
 	 * @throws InsufficientScopeException if the scope is invalid and we the flag is set to throw the exception
 	 */
-	public boolean sufficientScope(boolean decision) {
+	public boolean throwOnError(boolean decision) {
 		if (!decision && !missingScopes.isEmpty()) {
 			throw new InsufficientScopeException("Insufficient scope for this resource", missingScopes);
 		}
@@ -103,10 +111,8 @@ public boolean hasScope(String scope) {
 	 */
 	public boolean hasAnyScope(String... scopes) {
 		boolean result = OAuth2ExpressionUtils.hasAnyScope(authentication, scopes);
-		if (!result && throwExceptionOnInvalidScope) {
+		if (!result) {
 			missingScopes.addAll(Arrays.asList(scopes));
-			Throwable failure = new InsufficientScopeException("Insufficient scope for this resource", missingScopes);
-			throw new AccessDeniedException(failure.getMessage(), failure);
 		}
 		return result;
 	}
@@ -142,10 +148,8 @@ public boolean hasScopeMatching(String scopeRegex) {
 	public boolean hasAnyScopeMatching(String... scopesRegex) {
 
 		boolean result = OAuth2ExpressionUtils.hasAnyScopeMatching(authentication, scopesRegex);
-		if (!result && throwExceptionOnInvalidScope) {
+		if (!result) {
 			missingScopes.addAll(Arrays.asList(scopesRegex));
-			Throwable failure = new InsufficientScopeException("Insufficient scope for this resource", missingScopes);
-			throw new AccessDeniedException(failure.getMessage(), failure);
 		}
 		return result;
 	}
@@ -185,13 +189,4 @@ public boolean isUser() {
 	public boolean isClient() {
 		return OAuth2ExpressionUtils.isOAuthClientAuth(authentication);
 	}
-
-	/**
-	 * A flag to indicate that an exception should be thrown if a scope decision is negative.
-	 * 
-	 * @param throwExceptionOnInvalidScope flag value (default true)
-	 */
-	public void setThrowExceptionOnInvalidScope(boolean throwExceptionOnInvalidScope) {
-		this.throwExceptionOnInvalidScope = throwExceptionOnInvalidScope;
-	}
 }

spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/expression/OAuth2WebSecurityExpressionHandler.java

@@ -12,40 +12,38 @@
  */
 package org.springframework.security.oauth2.provider.expression;
 
+import org.springframework.expression.ExpressionParser;
 import org.springframework.expression.spel.support.StandardEvaluationContext;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.web.FilterInvocation;
 import org.springframework.security.web.access.expression.DefaultWebSecurityExpressionHandler;
 
 /**
+ * <p>
  * A security expression handler that can handle default web security expressions plus the set provided by
  * {@link OAuth2SecurityExpressionMethods} using the variable oauth2 to access the methods. For example, the expression
  * <code>#oauth2.clientHasRole('ROLE_ADMIN')</code> would invoke {@link OAuth2SecurityExpressionMethods#clientHasRole}.
+ * </p>
+ * <p>
+ * By default the {@link OAuth2ExpressionParser} is used. If this is undesirable one can inject their own
+ * {@link ExpressionParser} using {@link #setExpressionParser(ExpressionParser)}.
+ * </p>
  * 
  * @author Dave Syer
  * @author Rob Winch
  * 
+ * @see OAuth2ExpressionParser
  */
 public class OAuth2WebSecurityExpressionHandler extends DefaultWebSecurityExpressionHandler {
-	private boolean throwExceptionOnInvalidScope = true;
-
-	/**
-	 * Flag to determine the behaviour on access denied if the reason is . If set then we throw an
-	 * {@link InvalidScopeException} instead of returning true. This is unconventional for an access decision because it
-	 * vetos the other voters in the chain, but it enables us to pass a message to the caller with information about the
-	 * required scope.
-	 * 
-	 * @param throwException the flag to set (default true)
-	 */
-	public void setThrowExceptionOnInvalidScope(boolean throwException) {
-		this.throwExceptionOnInvalidScope = throwException;
+	public OAuth2WebSecurityExpressionHandler() {
+		setExpressionParser(new OAuth2ExpressionParser(getExpressionParser()));
 	}
-	
+
 	@Override
 	protected StandardEvaluationContext createEvaluationContextInternal(Authentication authentication,
 			FilterInvocation invocation) {
 		StandardEvaluationContext ec = super.createEvaluationContextInternal(authentication, invocation);
-		ec.setVariable("oauth2", new OAuth2SecurityExpressionMethods(authentication, throwExceptionOnInvalidScope));
+		ec.setVariable("oauth2", new OAuth2SecurityExpressionMethods(authentication));
 		return ec;
 	}
 }

spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/expression/OAuth2ExpressionParserTests.java

@@ -0,0 +1,52 @@
+package org.springframework.security.oauth2.provider.expression;
+
+import static org.mockito.Mockito.verify;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.expression.ExpressionParser;
+import org.springframework.expression.ParserContext;
+
+/**
+ * 
+ * @author Rob Winch
+ *
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class OAuth2ExpressionParserTests {
+	@Mock
+	private ExpressionParser delegate;
+	@Mock
+	private ParserContext parserContext;
+
+	private final String expressionString = "ORIGIONAL";
+
+	private final String wrappedExpression = "#oauth2.throwOnError(" + expressionString + ")";
+
+	private OAuth2ExpressionParser parser;
+	
+	@Before
+	public void setUp() {
+		parser = new OAuth2ExpressionParser(delegate);
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void constructorNull() {
+		new OAuth2ExpressionParser(null);
+	}
+	
+	@Test
+	public void parseExpression() {
+		parser.parseExpression(expressionString);
+		verify(delegate).parseExpression(wrappedExpression);
+	}
+	
+	@Test
+	public void parseExpressionWithContext() {
+		parser.parseExpression(expressionString, parserContext);
+		verify(delegate).parseExpression(wrappedExpression, parserContext);
+	}
+}