Skip to content

Commit

Permalink
Review updates - change SecurityServiceWrapper to RedisSecurityService
Browse files Browse the repository at this point in the history
  • Loading branch information
jdeppe-pivotal committed Oct 18, 2021
1 parent a693d26 commit 36fac97
Show file tree
Hide file tree
Showing 7 changed files with 106 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import org.apache.geode.redis.internal.pubsub.PubSubImpl;
import org.apache.geode.redis.internal.pubsub.Subscriptions;
import org.apache.geode.redis.internal.services.LockingStripedCoordinator;
import org.apache.geode.redis.internal.services.SecurityServiceWrapper;
import org.apache.geode.redis.internal.services.RedisSecurityService;
import org.apache.geode.redis.internal.services.StripedCoordinator;
import org.apache.geode.redis.internal.statistics.GeodeRedisStats;
import org.apache.geode.redis.internal.statistics.RedisStats;
Expand Down Expand Up @@ -84,7 +84,7 @@ public GeodeRedisServer(String bindAddress, int port, InternalCache cache) {
passiveExpirationManager = new PassiveExpirationManager(regionProvider);

DistributedMember member = cache.getDistributedSystem().getDistributedMember();
SecurityServiceWrapper securityService = new SecurityServiceWrapper(cache.getSecurityService());
RedisSecurityService securityService = new RedisSecurityService(cache.getSecurityService());

nettyRedisServer = new NettyRedisServer(() -> cache.getInternalDistributedSystem().getConfig(),
regionProvider, pubSub,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import io.netty.handler.codec.ByteToMessageDecoder;

import org.apache.geode.redis.internal.RedisException;
import org.apache.geode.redis.internal.services.SecurityServiceWrapper;
import org.apache.geode.redis.internal.services.RedisSecurityService;
import org.apache.geode.redis.internal.statistics.RedisStats;

/**
Expand Down Expand Up @@ -63,10 +63,10 @@ public class ByteToCommandDecoder extends ByteToMessageDecoder {
Integer.getInteger(UNAUTHENTICATED_MAX_BULK_STRING_LENGTH_PARAM, 16384);

private final RedisStats redisStats;
private final SecurityServiceWrapper securityService;
private final RedisSecurityService securityService;
private final ChannelId channelId;

public ByteToCommandDecoder(RedisStats redisStats, SecurityServiceWrapper securityService,
public ByteToCommandDecoder(RedisStats redisStats, RedisSecurityService securityService,
ChannelId channelId) {
this.redisStats = redisStats;
this.securityService = securityService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import java.io.IOException;
import java.math.BigInteger;
import java.util.Properties;
import java.util.List;
import java.util.Properties;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -56,7 +56,7 @@
import org.apache.geode.redis.internal.executor.UnknownExecutor;
import org.apache.geode.redis.internal.parameters.RedisParametersMismatchException;
import org.apache.geode.redis.internal.pubsub.PubSub;
import org.apache.geode.redis.internal.services.SecurityServiceWrapper;
import org.apache.geode.redis.internal.services.RedisSecurityService;
import org.apache.geode.redis.internal.statistics.RedisStats;
import org.apache.geode.security.NotAuthorizedException;
import org.apache.geode.security.ResourcePermission;
Expand Down Expand Up @@ -86,7 +86,7 @@ public class ExecutionHandlerContext extends ChannelInboundHandlerAdapter {
private final Runnable shutdownInvoker;
private final RedisStats redisStats;
private final DistributedMember member;
private final SecurityServiceWrapper securityService;
private final RedisSecurityService securityService;
private BigInteger scanCursor;
private BigInteger sscanCursor;
private final AtomicBoolean channelInactive = new AtomicBoolean();
Expand Down Expand Up @@ -119,7 +119,7 @@ public ExecutionHandlerContext(Channel channel,
String username,
int serverPort,
DistributedMember member,
SecurityServiceWrapper securityService) {
RedisSecurityService securityService) {
this.regionProvider = regionProvider;
this.pubsub = pubsub;
this.allowUnsupportedSupplier = allowUnsupportedSupplier;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
import org.apache.geode.management.ManagementException;
import org.apache.geode.redis.internal.RegionProvider;
import org.apache.geode.redis.internal.pubsub.PubSub;
import org.apache.geode.redis.internal.services.SecurityServiceWrapper;
import org.apache.geode.redis.internal.services.RedisSecurityService;
import org.apache.geode.redis.internal.statistics.RedisStats;

public class NettyRedisServer {
Expand All @@ -87,12 +87,12 @@ public class NettyRedisServer {
private final Channel serverChannel;
private final int serverPort;
private final DistributedMember member;
private final SecurityServiceWrapper securityService;
private final RedisSecurityService securityService;

public NettyRedisServer(Supplier<DistributionConfig> configSupplier,
RegionProvider regionProvider, PubSub pubsub, Supplier<Boolean> allowUnsupportedSupplier,
Runnable shutdownInvoker, int port, String requestedAddress, RedisStats redisStats,
DistributedMember member, SecurityServiceWrapper securityService) {
DistributedMember member, RedisSecurityService securityService) {
this.configSupplier = configSupplier;
this.regionProvider = regionProvider;
this.pubsub = pubsub;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
package org.apache.geode.redis.internal.netty;

/**
* Exception thrown by CommandParser (non-existent class) when a command has illegal syntax
* Exception thrown by {@link ByteToCommandDecoder} when a command has illegal syntax
*/
public class RedisCommandParserException extends Exception {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@
* login (authenticate), logout and authorize calls. It exists as a central point to track whether
* a given channel (connection) has previously been authenticated.
*/
public class SecurityServiceWrapper {
public class RedisSecurityService {

private final Map<String, Subject> subjects = new ConcurrentHashMap<>();

private final SecurityService securityService;

public SecurityServiceWrapper(SecurityService securityService) {
public RedisSecurityService(SecurityService securityService) {
this.securityService = securityService;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more contributor license
* agreements. See the NOTICE file distributed with this work for additional information regarding
* copyright ownership. The ASF licenses this file to You 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.apache.geode.redis.internal.services;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.util.Properties;

import io.netty.channel.ChannelId;
import org.apache.shiro.subject.Subject;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;

import org.apache.geode.internal.security.SecurityService;
import org.apache.geode.security.ResourcePermission;

public class RedisSecurityServiceTest {

private RedisSecurityService redisSecurityService;
private SecurityService geodeSecurityService;

@Before
public void setup() {
geodeSecurityService = mock(SecurityService.class);
redisSecurityService = new RedisSecurityService(geodeSecurityService);
}

@Test
public void serviceDelegatesToLogin_andCanAuthenticateCorrectly() {
ChannelId channelId = mock(ChannelId.class);
when(channelId.asShortText()).thenReturn("channelId");
Properties properties = mock(Properties.class);
Subject subject = mock(Subject.class);
when(geodeSecurityService.login(any())).thenReturn(subject);

assertThat(redisSecurityService.login(channelId, properties)).isEqualTo(subject);
assertThat(redisSecurityService.isAuthenticated(channelId)).isTrue();
}

@Test
public void serviceDelegatesToLogout() {
ChannelId channelId = mock(ChannelId.class);
when(channelId.asShortText()).thenReturn("channelId");
Properties properties = mock(Properties.class);
Subject subject = mock(Subject.class);
when(geodeSecurityService.login(any())).thenReturn(subject);

assertThat(redisSecurityService.login(channelId, properties)).isEqualTo(subject);

redisSecurityService.logout(channelId);

assertThat(redisSecurityService.isAuthenticated(channelId)).isFalse();
}

@Test
public void serviceDelegatesToAuthorize() {
ChannelId channelId = mock(ChannelId.class);
when(channelId.asShortText()).thenReturn("channelId");
Properties properties = mock(Properties.class);
Subject subject = mock(Subject.class);
when(geodeSecurityService.login(any())).thenReturn(subject);

assertThat(redisSecurityService.login(channelId, properties)).isEqualTo(subject);

redisSecurityService.authorize(mock(ResourcePermission.class), subject);

ArgumentCaptor<Subject> argumentCaptor = ArgumentCaptor.forClass(Subject.class);
verify(geodeSecurityService)
.authorize(any(ResourcePermission.class), argumentCaptor.capture());
assertThat(argumentCaptor.getValue()).isEqualTo(subject);
}

}

0 comments on commit 36fac97

Please sign in to comment.