Skip to content

Commit

Permalink
Refactor update password api auth check and add unit test. (#12757)
Browse files Browse the repository at this point in the history
  • Loading branch information
@KomachiSion
KomachiSion authored Oct 17, 2024
1 parent a0fdc42 commit 2b178be
Show file tree
Hide file tree
Showing 9 changed files with 147 additions and 26 deletions.
7 changes: 6 additions & 1 deletion auth/src/main/java/com/alibaba/nacos/auth/AbstractProtocolAuthService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.alibaba.nacos.plugin.auth.spi.server.AuthPluginService;

import java.util.Optional;
import java.util.Properties;

/**
* Abstract protocol auth service.
Expand Down Expand Up @@ -84,7 +85,11 @@ public boolean validateAuthority(IdentityContext identityContext, Permission per
* @return resource
*/
protected Resource parseSpecifiedResource(Secured secured) {
return new Resource(null, null, secured.resource(), SignType.SPECIFIED, null);
Properties properties = new Properties();
for (String each : secured.tags()) {
properties.put(each, each);
}
return new Resource(null, null, secured.resource(), SignType.SPECIFIED, properties);
}

/**
Expand Down
28 changes: 27 additions & 1 deletion auth/src/test/java/com/alibaba/nacos/auth/GrpcProtocolAuthServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.alibaba.nacos.auth.annotation.Secured;
import com.alibaba.nacos.auth.config.AuthConfigs;
import com.alibaba.nacos.auth.mock.MockAuthPluginService;
import com.alibaba.nacos.auth.mock.MockResourceParser;
import com.alibaba.nacos.plugin.auth.api.IdentityContext;
import com.alibaba.nacos.plugin.auth.api.Permission;
import com.alibaba.nacos.plugin.auth.api.Resource;
Expand Down Expand Up @@ -85,7 +86,8 @@ void testParseResourceWithSpecifiedResource() throws NoSuchMethodException {
assertEquals(SignType.SPECIFIED, actual.getType());
assertNull(actual.getNamespaceId());
assertNull(actual.getGroup());
assertNull(actual.getProperties());
assertNotNull(actual.getProperties());
assertTrue(actual.getProperties().isEmpty());
}

@Test
Expand All @@ -96,6 +98,14 @@ void testParseResourceWithNonExistType() throws NoSuchMethodException {
assertEquals(Resource.EMPTY_RESOURCE, actual);
}

@Test
@Secured(signType = "non-exist", parser = MockResourceParser.class)
void testParseResourceWithNonExistTypeException() throws NoSuchMethodException {
Secured secured = getMethodSecure("testParseResourceWithNonExistTypeException");
Resource actual = protocolAuthService.parseResource(namingRequest, secured);
assertEquals(Resource.EMPTY_RESOURCE, actual);
}

@Test
@Secured()
void testParseResourceWithNamingType() throws NoSuchMethodException {
Expand Down Expand Up @@ -152,6 +162,22 @@ void testValidateAuthorityWithPlugin() throws AccessException {
new Permission(Resource.EMPTY_RESOURCE, "")));
}

@Test
@Secured(signType = SignType.CONFIG)
void testEnabledAuthWithPlugin() throws NoSuchMethodException {
Mockito.when(authConfigs.getNacosAuthSystemType()).thenReturn(MockAuthPluginService.TEST_PLUGIN);
Secured secured = getMethodSecure("testEnabledAuthWithPlugin");
assertTrue(protocolAuthService.enableAuth(secured));
}

@Test
@Secured(signType = SignType.CONFIG)
void testEnabledAuthWithoutPlugin() throws NoSuchMethodException {
Mockito.when(authConfigs.getNacosAuthSystemType()).thenReturn("non-exist-plugin");
Secured secured = getMethodSecure("testEnabledAuthWithoutPlugin");
assertFalse(protocolAuthService.enableAuth(secured));
}

private Secured getMethodSecure(String methodName) throws NoSuchMethodException {
Method method = GrpcProtocolAuthServiceTest.class.getDeclaredMethod(methodName);
return method.getAnnotation(Secured.class);
Expand Down
55 changes: 41 additions & 14 deletions auth/src/test/java/com/alibaba/nacos/auth/HttpProtocolAuthServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.alibaba.nacos.auth.annotation.Secured;
import com.alibaba.nacos.auth.config.AuthConfigs;
import com.alibaba.nacos.auth.mock.MockAuthPluginService;
import com.alibaba.nacos.auth.mock.MockResourceParser;
import com.alibaba.nacos.plugin.auth.api.IdentityContext;
import com.alibaba.nacos.plugin.auth.api.Permission;
import com.alibaba.nacos.plugin.auth.api.Resource;
Expand Down Expand Up @@ -56,12 +57,12 @@ class HttpProtocolAuthServiceTest {
@Mock
private HttpServletRequest request;

private HttpProtocolAuthService httpProtocolAuthService;
private HttpProtocolAuthService protocolAuthService;

@BeforeEach
void setUp() throws Exception {
httpProtocolAuthService = new HttpProtocolAuthService(authConfigs);
httpProtocolAuthService.initialize();
protocolAuthService = new HttpProtocolAuthService(authConfigs);
protocolAuthService.initialize();
Mockito.when(request.getParameter(eq(CommonParams.NAMESPACE_ID))).thenReturn("testNNs");
Mockito.when(request.getParameter(eq(CommonParams.GROUP_NAME))).thenReturn("testNG");
Mockito.when(request.getParameter(eq(CommonParams.SERVICE_NAME))).thenReturn("testS");
Expand All @@ -71,30 +72,40 @@ void setUp() throws Exception {
}

@Test
@Secured(resource = "testResource")
@Secured(resource = "testResource", tags = {"testTag"})
void testParseResourceWithSpecifiedResource() throws NoSuchMethodException {
Secured secured = getMethodSecure("testParseResourceWithSpecifiedResource");
Resource actual = httpProtocolAuthService.parseResource(request, secured);
Resource actual = protocolAuthService.parseResource(request, secured);
assertEquals("testResource", actual.getName());
assertEquals(SignType.SPECIFIED, actual.getType());
assertNull(actual.getNamespaceId());
assertNull(actual.getGroup());
assertNull(actual.getProperties());
assertNotNull(actual.getProperties());
assertEquals(1, actual.getProperties().size());
assertEquals("testTag", actual.getProperties().get("testTag"));
}

@Test
@Secured(signType = "non-exist")
void testParseResourceWithNonExistType() throws NoSuchMethodException {
Secured secured = getMethodSecure("testParseResourceWithNonExistType");
Resource actual = httpProtocolAuthService.parseResource(request, secured);
Resource actual = protocolAuthService.parseResource(request, secured);
assertEquals(Resource.EMPTY_RESOURCE, actual);
}

@Test
@Secured(signType = "non-exist", parser = MockResourceParser.class)
void testParseResourceWithNonExistTypeException() throws NoSuchMethodException {
Secured secured = getMethodSecure("testParseResourceWithNonExistTypeException");
Resource actual = protocolAuthService.parseResource(request, secured);
assertEquals(Resource.EMPTY_RESOURCE, actual);
}

@Test
@Secured()
void testParseResourceWithNamingType() throws NoSuchMethodException {
Secured secured = getMethodSecure("testParseResourceWithNamingType");
Resource actual = httpProtocolAuthService.parseResource(request, secured);
Resource actual = protocolAuthService.parseResource(request, secured);
assertEquals(SignType.NAMING, actual.getType());
assertEquals("testS", actual.getName());
assertEquals("testNNs", actual.getNamespaceId());
Expand All @@ -106,7 +117,7 @@ void testParseResourceWithNamingType() throws NoSuchMethodException {
@Secured(signType = SignType.CONFIG)
void testParseResourceWithConfigType() throws NoSuchMethodException {
Secured secured = getMethodSecure("testParseResourceWithConfigType");
Resource actual = httpProtocolAuthService.parseResource(request, secured);
Resource actual = protocolAuthService.parseResource(request, secured);
assertEquals(SignType.CONFIG, actual.getType());
assertEquals("testD", actual.getName());
assertEquals("testNNs", actual.getNamespaceId());
Expand All @@ -116,36 +127,52 @@ void testParseResourceWithConfigType() throws NoSuchMethodException {

@Test
void testParseIdentity() {
IdentityContext actual = httpProtocolAuthService.parseIdentity(request);
IdentityContext actual = protocolAuthService.parseIdentity(request);
assertNotNull(actual);
}

@Test
void testValidateIdentityWithoutPlugin() throws AccessException {
IdentityContext identityContext = new IdentityContext();
assertTrue(httpProtocolAuthService.validateIdentity(identityContext, Resource.EMPTY_RESOURCE));
assertTrue(protocolAuthService.validateIdentity(identityContext, Resource.EMPTY_RESOURCE));
}

@Test
void testValidateIdentityWithPlugin() throws AccessException {
Mockito.when(authConfigs.getNacosAuthSystemType()).thenReturn(MockAuthPluginService.TEST_PLUGIN);
IdentityContext identityContext = new IdentityContext();
assertFalse(httpProtocolAuthService.validateIdentity(identityContext, Resource.EMPTY_RESOURCE));
assertFalse(protocolAuthService.validateIdentity(identityContext, Resource.EMPTY_RESOURCE));
}

@Test
void testValidateAuthorityWithoutPlugin() throws AccessException {
assertTrue(httpProtocolAuthService.validateAuthority(new IdentityContext(),
assertTrue(protocolAuthService.validateAuthority(new IdentityContext(),
new Permission(Resource.EMPTY_RESOURCE, "")));
}

@Test
void testValidateAuthorityWithPlugin() throws AccessException {
Mockito.when(authConfigs.getNacosAuthSystemType()).thenReturn(MockAuthPluginService.TEST_PLUGIN);
assertFalse(httpProtocolAuthService.validateAuthority(new IdentityContext(),
assertFalse(protocolAuthService.validateAuthority(new IdentityContext(),
new Permission(Resource.EMPTY_RESOURCE, "")));
}

@Test
@Secured(signType = SignType.CONFIG)
void testEnabledAuthWithPlugin() throws NoSuchMethodException {
Mockito.when(authConfigs.getNacosAuthSystemType()).thenReturn(MockAuthPluginService.TEST_PLUGIN);
Secured secured = getMethodSecure("testEnabledAuthWithPlugin");
assertTrue(protocolAuthService.enableAuth(secured));
}

@Test
@Secured(signType = SignType.CONFIG)
void testEnabledAuthWithoutPlugin() throws NoSuchMethodException {
Mockito.when(authConfigs.getNacosAuthSystemType()).thenReturn("non-exist-plugin");
Secured secured = getMethodSecure("testEnabledAuthWithoutPlugin");
assertFalse(protocolAuthService.enableAuth(secured));
}

private Secured getMethodSecure(String methodName) throws NoSuchMethodException {
Method method = HttpProtocolAuthServiceTest.class.getDeclaredMethod(methodName);
return method.getAnnotation(Secured.class);
Expand Down
30 changes: 30 additions & 0 deletions auth/src/test/java/com/alibaba/nacos/auth/mock/MockResourceParser.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 1999-2023 Alibaba Group Holding Ltd.
*
* 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 com.alibaba.nacos.auth.mock;

import com.alibaba.nacos.api.exception.runtime.NacosRuntimeException;
import com.alibaba.nacos.auth.annotation.Secured;
import com.alibaba.nacos.auth.parser.ResourceParser;
import com.alibaba.nacos.plugin.auth.api.Resource;

public class MockResourceParser implements ResourceParser<Object> {

@Override
public Resource parse(Object request, Secured secured) {
throw new NacosRuntimeException(500);
}
}
18 changes: 18 additions & 0 deletions auth/src/test/java/com/alibaba/nacos/auth/parser/grpc/ConfigGrpcResourceParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.alibaba.nacos.api.common.Constants;
import com.alibaba.nacos.api.config.remote.request.ConfigBatchListenRequest;
import com.alibaba.nacos.api.config.remote.request.ConfigChangeNotifyRequest;
import com.alibaba.nacos.api.config.remote.request.ConfigPublishRequest;
import com.alibaba.nacos.api.remote.request.Request;
import com.alibaba.nacos.auth.annotation.Secured;
Expand Down Expand Up @@ -98,6 +99,23 @@ void testParseWithConfigBatchListenRequest() throws NoSuchMethodException {
assertEquals(StringUtils.EMPTY, actual.getGroup());
assertEquals(StringUtils.EMPTY, actual.getName());
assertEquals(Constants.Config.CONFIG_MODULE, actual.getType());
request.getConfigListenContexts().clear();
actual = resourceParser.parse(request, secured);
assertEquals(StringUtils.EMPTY, actual.getNamespaceId());
assertEquals(StringUtils.EMPTY, actual.getGroup());
assertEquals(StringUtils.EMPTY, actual.getName());
assertEquals(Constants.Config.CONFIG_MODULE, actual.getType());
}

@Test
@Secured(signType = Constants.Config.CONFIG_MODULE)
void testParseWithReflectionRequest() throws NoSuchMethodException {
Secured secured = getMethodSecure();
Request request = ConfigChangeNotifyRequest.build("rTestD", "rTestG", "rTestNs");
Resource actual = resourceParser.parse(request, secured);
assertEquals("rTestNs", actual.getNamespaceId());
assertEquals("rTestG", actual.getGroup());
assertEquals("rTestD", actual.getName());
}

private Request mockConfigRequest(String tenant, String group, String dataId) {
Expand Down
1 change: 0 additions & 1 deletion ...-auth-plugin/src/main/java/com/alibaba/nacos/plugin/auth/impl/NacosAuthPluginService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ private String resolveToken(IdentityContext identityContext) {
public Boolean validateAuthority(IdentityContext identityContext, Permission permission) throws AccessException {
NacosUser user = (NacosUser) identityContext.getParameter(AuthConstants.NACOS_USER_KEY);
authenticationManager.authorize(permission, user);

return true;
}

Expand Down
3 changes: 2 additions & 1 deletion ...th-plugin/src/main/java/com/alibaba/nacos/plugin/auth/impl/controller/UserController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ public Object deleteUser(@RequestParam String username) {
* @since 1.2.0
*/
@PutMapping
@Secured(resource = AuthConstants.UPDATE_PASSWORD_ENTRY_POINT, action = ActionTypes.WRITE)
@Secured(resource = AuthConstants.UPDATE_PASSWORD_ENTRY_POINT, action = ActionTypes.WRITE, tags = {
AuthConstants.UPDATE_PASSWORD_ENTRY_POINT})
public Object updateUser(@RequestParam String username, @RequestParam String newPassword,
HttpServletResponse response, HttpServletRequest request) throws IOException {
// admin or same user
Expand Down
12 changes: 10 additions & 2 deletions ...h-plugin/src/main/java/com/alibaba/nacos/plugin/auth/impl/roles/NacosRoleServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -120,8 +121,7 @@ private void reload() {
* @return true if granted, false otherwise
*/
public boolean hasPermission(NacosUser nacosUser, Permission permission) {
//update password
if (AuthConstants.UPDATE_PASSWORD_ENTRY_POINT.equals(permission.getResource().getName())) {
if (isUpdatePasswordPermission(permission)) {
return true;
}

Expand Down Expand Up @@ -161,6 +161,14 @@ public boolean hasPermission(NacosUser nacosUser, Permission permission) {
return false;
}

/**
* If API is update user password, don't do permission check, because there is permission check in API logic.
*/
private boolean isUpdatePasswordPermission(Permission permission) {
Properties properties = permission.getResource().getProperties();
return null != properties && properties.contains(AuthConstants.UPDATE_PASSWORD_ENTRY_POINT);
}

public List<RoleInfo> getRoles(String username) {
List<RoleInfo> roleInfoList = roleInfoMap.get(username);
if (!authConfigs.isCachingEnabled() || roleInfoList == null) {
Expand Down
19 changes: 13 additions & 6 deletions ...ugin/src/test/java/com/alibaba/nacos/plugin/auth/impl/roles/NacosRoleServiceImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.lang.reflect.Method;
import java.util.Collections;
import java.util.List;
import java.util.Properties;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
Expand Down Expand Up @@ -113,10 +114,14 @@ void hasPermission() {

Permission permission2 = new Permission();
permission2.setAction("rw");
Resource resource = new Resource("public", "group", AuthConstants.UPDATE_PASSWORD_ENTRY_POINT, "rw", null);
Resource resource = new Resource("public", "group", AuthConstants.UPDATE_PASSWORD_ENTRY_POINT, "rw",
new Properties());
permission2.setResource(resource);
boolean res2 = nacosRoleService.hasPermission(nacosUser, permission2);
assertTrue(res2);
assertFalse(res2);
resource.getProperties().put(AuthConstants.UPDATE_PASSWORD_ENTRY_POINT, AuthConstants.UPDATE_PASSWORD_ENTRY_POINT);
boolean res3 = nacosRoleService.hasPermission(nacosUser, permission2);
assertTrue(res3);
}

@Test
Expand All @@ -127,7 +132,8 @@ void getRoles() {

@Test
void getRolesFromDatabase() {
Page<RoleInfo> roleInfoPage = nacosRoleService.getRolesFromDatabase("nacos", "ROLE_ADMIN", 1, Integer.MAX_VALUE);
Page<RoleInfo> roleInfoPage = nacosRoleService.getRolesFromDatabase("nacos", "ROLE_ADMIN", 1,
Integer.MAX_VALUE);
assertEquals(0, roleInfoPage.getTotalCount());
}

Expand All @@ -141,8 +147,8 @@ void getPermissions() {

@Test
void getPermissionsByRoleFromDatabase() {
Page<PermissionInfo> permissionsByRoleFromDatabase = nacosRoleService.getPermissionsByRoleFromDatabase("role-admin", 1,
Integer.MAX_VALUE);
Page<PermissionInfo> permissionsByRoleFromDatabase = nacosRoleService.getPermissionsByRoleFromDatabase(
"role-admin", 1, Integer.MAX_VALUE);
assertNull(permissionsByRoleFromDatabase);
}

Expand All @@ -169,7 +175,8 @@ void deleteRole() {

@Test
void getPermissionsFromDatabase() {
Page<PermissionInfo> permissionsFromDatabase = nacosRoleService.getPermissionsFromDatabase("role-admin", 1, Integer.MAX_VALUE);
Page<PermissionInfo> permissionsFromDatabase = nacosRoleService.getPermissionsFromDatabase("role-admin", 1,
Integer.MAX_VALUE);
assertEquals(0, permissionsFromDatabase.getTotalCount());
}

Expand Down

0 comments on commit 2b178be

Please sign in to comment.