-
Notifications
You must be signed in to change notification settings - Fork 256
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
Address range support in ClientIpActivationStrategy #172
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
/* | ||
* The MIT License | ||
* | ||
* Copyright (c) 2013 Edin Dazdarevic (edin.dazdarevic@gmail.com) | ||
|
||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal | ||
* in the Software without restriction, including without limitation the rights | ||
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
* copies of the Software, and to permit persons to whom the Software is | ||
* furnished to do so, subject to the following conditions: | ||
|
||
* The above copyright notice and this permission notice shall be included in | ||
* all copies or substantial portions of the Software. | ||
|
||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
* THE SOFTWARE. | ||
* | ||
* */ | ||
|
||
package org.togglz.servlet.activation; | ||
|
||
import java.math.BigInteger; | ||
import java.net.InetAddress; | ||
import java.net.UnknownHostException; | ||
import java.nio.ByteBuffer; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
// From https://github.com/edazdarevic/CIDRUtils | ||
|
||
/** | ||
* A class that enables to get an IP range from CIDR specification. It supports | ||
* both IPv4 and IPv6. | ||
*/ | ||
public class CIDRUtils { | ||
private final String cidr; | ||
|
||
private InetAddress inetAddress; | ||
private InetAddress startAddress; | ||
private InetAddress endAddress; | ||
private final int prefixLength; | ||
|
||
|
||
public CIDRUtils(String cidr) throws UnknownHostException { | ||
|
||
this.cidr = cidr; | ||
|
||
/* split CIDR to address and prefix part */ | ||
if (this.cidr.contains("/")) { | ||
int index = this.cidr.indexOf("/"); | ||
String addressPart = this.cidr.substring(0, index); | ||
String networkPart = this.cidr.substring(index + 1); | ||
|
||
inetAddress = InetAddress.getByName(addressPart); | ||
prefixLength = Integer.parseInt(networkPart); | ||
|
||
calculate(); | ||
} else { | ||
throw new IllegalArgumentException("not an valid CIDR format!"); | ||
} | ||
} | ||
|
||
|
||
private void calculate() throws UnknownHostException { | ||
|
||
ByteBuffer maskBuffer; | ||
int targetSize; | ||
if (inetAddress.getAddress().length == 4) { | ||
maskBuffer = | ||
ByteBuffer | ||
.allocate(4) | ||
.putInt(-1); | ||
targetSize = 4; | ||
} else { | ||
maskBuffer = ByteBuffer.allocate(16) | ||
.putLong(-1L) | ||
.putLong(-1L); | ||
targetSize = 16; | ||
} | ||
|
||
BigInteger mask = (new BigInteger(1, maskBuffer.array())).not().shiftRight(prefixLength); | ||
|
||
ByteBuffer buffer = ByteBuffer.wrap(inetAddress.getAddress()); | ||
BigInteger ipVal = new BigInteger(1, buffer.array()); | ||
|
||
BigInteger startIp = ipVal.and(mask); | ||
BigInteger endIp = startIp.add(mask.not()); | ||
|
||
byte[] startIpArr = toBytes(startIp.toByteArray(), targetSize); | ||
byte[] endIpArr = toBytes(endIp.toByteArray(), targetSize); | ||
|
||
this.startAddress = InetAddress.getByAddress(startIpArr); | ||
this.endAddress = InetAddress.getByAddress(endIpArr); | ||
|
||
} | ||
|
||
private byte[] toBytes(byte[] array, int targetSize) { | ||
int counter = 0; | ||
List<Byte> newArr = new ArrayList<Byte>(); | ||
while (counter < targetSize && (array.length - 1 - counter >= 0)) { | ||
newArr.add(0, array[array.length - 1 - counter]); | ||
counter++; | ||
} | ||
|
||
int size = newArr.size(); | ||
for (int i = 0; i < (targetSize - size); i++) { | ||
|
||
newArr.add(0, (byte) 0); | ||
} | ||
|
||
byte[] ret = new byte[newArr.size()]; | ||
for (int i = 0; i < newArr.size(); i++) { | ||
ret[i] = newArr.get(i); | ||
} | ||
return ret; | ||
} | ||
|
||
public String getNetworkAddress() { | ||
|
||
return this.startAddress.getHostAddress(); | ||
} | ||
|
||
public String getBroadcastAddress() { | ||
return this.endAddress.getHostAddress(); | ||
} | ||
|
||
public boolean isInRange(String ipAddress) throws UnknownHostException { | ||
InetAddress address = InetAddress.getByName(ipAddress); | ||
BigInteger start = new BigInteger(1, this.startAddress.getAddress()); | ||
BigInteger end = new BigInteger(1, this.endAddress.getAddress()); | ||
BigInteger target = new BigInteger(1, address.getAddress()); | ||
|
||
int st = start.compareTo(target); | ||
int te = target.compareTo(end); | ||
|
||
return (st == -1 || st == 0) && (te == -1 || te == 0); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,30 @@ | ||
package org.togglz.servlet.activation; | ||
|
||
import java.util.List; | ||
|
||
import javax.servlet.http.HttpServletRequest; | ||
|
||
import org.togglz.core.activation.Parameter; | ||
import org.togglz.core.activation.ParameterBuilder; | ||
import org.togglz.core.logging.Log; | ||
import org.togglz.core.logging.LogFactory; | ||
import org.togglz.core.repository.FeatureState; | ||
import org.togglz.core.spi.ActivationStrategy; | ||
import org.togglz.core.user.FeatureUser; | ||
import org.togglz.core.util.Strings; | ||
import org.togglz.servlet.util.HttpServletRequestHolder; | ||
|
||
import java.net.InetAddress; | ||
import java.net.UnknownHostException; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
import javax.servlet.http.HttpServletRequest; | ||
|
||
/** | ||
* Activation strategy that will use the IP address of the client to decide if a feature is active or not. | ||
* | ||
* @author Christian Kaltepoth | ||
*/ | ||
public class ClientIpActivationStrategy implements ActivationStrategy | ||
{ | ||
private final Log log = LogFactory.getLog(ClientIpActivationStrategy.class); | ||
|
||
public static final String ID = "client-ip"; | ||
|
||
|
@@ -39,28 +45,49 @@ public String getName() | |
@Override | ||
public boolean isActive(FeatureState featureState, FeatureUser user) | ||
{ | ||
|
||
HttpServletRequest request = HttpServletRequestHolder.get(); | ||
if (request != null) { | ||
|
||
String allowedIpsParam = featureState.getParameter(PARAM_IPS); | ||
List<String> allowsIps = Strings.splitAndTrim(allowedIpsParam, "[\\s,]+"); | ||
|
||
// TODO: This should support a simple form of subnet matching | ||
return allowsIps.contains(request.getRemoteAddr()); | ||
|
||
List<String> parts = Strings.splitAndTrim(featureState.getParameter(PARAM_IPS), "[\\s,]+"); | ||
|
||
List<InetAddress> ips = new ArrayList<>(); | ||
List<CIDRUtils> cidrUtils = new ArrayList<>(); | ||
for (String part : parts) { | ||
try { | ||
if (part.contains("/")) { | ||
cidrUtils.add(new CIDRUtils(part)); | ||
} else { | ||
ips.add(InetAddress.getByName(part)); | ||
} | ||
} catch (Exception e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catching |
||
log.warn("Ignoring illegal IP address or CIDR range " + part); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this exception should be catched here. How would the togglz-console UI handle it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First you should catch And we should perhaps use the |
||
} | ||
} | ||
|
||
try { | ||
if (ips.contains(InetAddress.getByName(request.getRemoteAddr()))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just checked the implementation. If the given address is an IPv4 or IPv6 address, no DNS lookup is done. |
||
return true; | ||
} | ||
|
||
for (CIDRUtils cidrUtil : cidrUtils) { | ||
if (cidrUtil.isInRange(request.getRemoteAddr())) { | ||
return true; | ||
} | ||
} | ||
} catch (UnknownHostException e) { | ||
log.error("Illegal address " + request.getRemoteAddr()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this exception should be catched here. How would the togglz-console UI handle it? |
||
} | ||
} | ||
|
||
return false; | ||
|
||
} | ||
|
||
@Override | ||
public Parameter[] getParameters() | ||
{ | ||
return new Parameter[] { | ||
ParameterBuilder.create(PARAM_IPS).label("Client IPs") | ||
.description("A comma-separated list of client IPs for which the feature should be active.") | ||
.description("A comma-separated list of client IPs or address ranges in CIDR notation (e.g. 10.1.2.0/24) for which the feature should be active.") | ||
}; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
package org.togglz.servlet.activation; | ||
|
||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
import static org.togglz.servlet.activation.ClientIpActivationStrategyTest.MockRequest.requestFrom; | ||
import static org.togglz.servlet.activation.ClientIpActivationStrategyTest.MockRequestAssert.assertThat; | ||
|
||
import org.assertj.core.api.AbstractAssert; | ||
import org.assertj.core.api.Assertions; | ||
import org.junit.After; | ||
import org.junit.Test; | ||
import org.togglz.core.Feature; | ||
import org.togglz.core.repository.FeatureState; | ||
import org.togglz.servlet.util.HttpServletRequestHolder; | ||
|
||
import javax.servlet.http.HttpServletRequest; | ||
|
||
public class ClientIpActivationStrategyTest { | ||
|
||
protected static class MockRequest { | ||
private final HttpServletRequest request; | ||
|
||
public static MockRequest requestFrom(String remoteAddr) { | ||
return new MockRequest(remoteAddr); | ||
} | ||
|
||
private MockRequest(String remoteAddr) { | ||
request = mock(HttpServletRequest.class); | ||
when(request.getRemoteAddr()).thenReturn(remoteAddr); | ||
HttpServletRequestHolder.bind(request); | ||
} | ||
|
||
public HttpServletRequest getRequest() { | ||
return request; | ||
} | ||
} | ||
|
||
protected static class MockRequestAssert extends AbstractAssert<MockRequestAssert, MockRequest> { | ||
protected MockRequestAssert(MockRequest actual) { | ||
super(actual, MockRequestAssert.class); | ||
} | ||
|
||
public static MockRequestAssert assertThat(MockRequest actual) { | ||
return new MockRequestAssert(actual); | ||
} | ||
|
||
public MockRequestAssert isActiveWithParams(String params) { | ||
if (!strategy().isActive(featureState(params), null)) { | ||
Assertions.fail("Expected the strategy to turn the feature active with params " + params); | ||
} | ||
return this; | ||
} | ||
|
||
public MockRequestAssert isInactiveWithParams(String params) { | ||
if (strategy().isActive(featureState(params), null)) { | ||
Assertions.fail("Expected the strategy to turn the feature inactive with params " + params); | ||
} | ||
return this; | ||
} | ||
|
||
private static ClientIpActivationStrategy strategy() { | ||
return new ClientIpActivationStrategy(); | ||
} | ||
|
||
|
||
private static FeatureState featureState(String ips) { | ||
return new FeatureState(TestFeature.TEST_FEATURE) | ||
.enable() | ||
.setStrategyId(ClientIpActivationStrategy.ID) | ||
.setParameter(ClientIpActivationStrategy.PARAM_IPS, ips); | ||
} | ||
|
||
private enum TestFeature implements Feature { | ||
TEST_FEATURE | ||
} | ||
} | ||
|
||
@After | ||
public void cleanup() { | ||
HttpServletRequestHolder.release(); | ||
} | ||
|
||
@Test | ||
public void shouldBeInactiveForNullParams() throws Exception { | ||
assertThat(requestFrom("10.1.2.3")).isInactiveWithParams(null); | ||
} | ||
|
||
@Test | ||
public void shouldBeInactiveForEmptyParams() throws Exception { | ||
assertThat(requestFrom("10.1.2.3")).isInactiveWithParams(""); | ||
} | ||
|
||
@Test | ||
public void shouldBeInactiveForNonMatchingIp() throws Exception { | ||
assertThat(requestFrom("10.1.2.3")).isInactiveWithParams("10.1.2.4"); | ||
} | ||
|
||
@Test | ||
public void shouldBeActiveForFirstMatchingIp() throws Exception { | ||
assertThat(requestFrom("192.168.0.1")).isActiveWithParams("192.168.0.1,10.1.2.3"); | ||
} | ||
|
||
@Test | ||
public void shouldBeActiveForSecondMatchingIp() throws Exception { | ||
assertThat(requestFrom("10.1.2.3")).isActiveWithParams("192.168.0.1,10.1.2.3"); | ||
} | ||
|
||
@Test | ||
public void shouldBeInactiveForNonMatchingRange() throws Exception { | ||
assertThat(requestFrom("10.1.2.16")).isInactiveWithParams("192.168.0.1,10.1.2.0/28"); | ||
} | ||
|
||
@Test | ||
public void shouldBeActiveForFirstMatchingRange() throws Exception { | ||
assertThat(requestFrom("192.168.0.5")).isActiveWithParams("192.168.0.0/24,10.1.2.0/24"); | ||
} | ||
|
||
@Test | ||
public void shouldBeActiveForSecondMatchingRange() throws Exception { | ||
assertThat(requestFrom("10.1.2.3")).isActiveWithParams("192.168.0.0/24,10.1.2.0/24"); | ||
} | ||
|
||
@Test | ||
public void shouldBeInactiveForInvalidCidrNotation() throws Exception { | ||
assertThat(requestFrom("10.1.2.3")).isInactiveWithParams("192.168.0.0/24,abc/24"); | ||
} | ||
|
||
@Test | ||
public void shouldBeInactiveForNonMatchingIpv6() throws Exception { | ||
assertThat(requestFrom("2001:db8:0:0:0:0:0:1")).isInactiveWithParams("2001:db8:0:0:0:0:0:2"); | ||
} | ||
|
||
@Test | ||
public void shouldBeActiveForMatchingIpv6() throws Exception { | ||
assertThat(requestFrom("2001:db8:0:0:0:0:0:1")).isActiveWithParams("2001:db8:0:0:0:0:0:1"); | ||
} | ||
|
||
@Test | ||
public void shouldBeInactiveForNonMatchingIpv6ShortForm() throws Exception { | ||
assertThat(requestFrom("2001:db8:0:0:0:0:0:1")).isInactiveWithParams("2001:db8::2"); | ||
} | ||
|
||
@Test | ||
public void shouldBeActiveForMatchingIpv6ShortForm() throws Exception { | ||
assertThat(requestFrom("2001:db8:0:0:0:0:0:1")).isActiveWithParams("2001:db8::1"); | ||
} | ||
|
||
@Test | ||
public void shouldBeActiveForMatchingIpv6Range() throws Exception { | ||
assertThat(requestFrom("2001:db8:0:0:0:0:0:1")).isActiveWithParams("2001:db8::/24"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that you have to build these lists first and validate in a separate code block. Why not doing this in one step. This would even be more efficient, because you have a result as soon as the first entry matches and therefore don't have parse the following addresses.