From c01d0bc4fe1a52859453b8fe69c2c306690769a3 Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Fri, 27 Sep 2024 18:07:03 +0200 Subject: [PATCH] KARAF-5014: consider first group role in users.properties and ignore empty roles --- .../properties/DigestPasswordLoginModule.java | 13 +++- .../properties/PropertiesBackingEngine.java | 63 ++++++++++++------ .../properties/PropertiesLoginModule.java | 13 +++- .../PropertiesBackingEngineTest.java | 64 ++++++++++++++++--- .../properties/PropertiesLoginModuleTest.java | 7 +- 5 files changed, 124 insertions(+), 36 deletions(-) diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java index e4c33475aa1..35816954951 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java @@ -21,8 +21,11 @@ import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; +import java.security.Principal; import java.util.HashSet; import java.util.Map; +import java.util.Set; + import javax.security.auth.Subject; import javax.security.auth.callback.Callback; import javax.security.auth.callback.CallbackHandler; @@ -214,13 +217,13 @@ public boolean login() throws LoginException { String groupInfo = users.get(infos[i].trim()); if (groupInfo != null) { String[] roles = groupInfo.split(","); - for (int j = 1; j < roles.length; j++) { - principals.add(new RolePrincipal(roles[j].trim())); + for (int j = 0; j < roles.length; j++) { + addRole(principals, roles[j].trim()); } } } else { // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); + addRole(principals, infos[i].trim()); } } @@ -233,4 +236,8 @@ public boolean login() throws LoginException { return true; } + private void addRole(Set principals, String trimmedRole) { + if (!trimmedRole.isEmpty()) + principals.add(new RolePrincipal(trimmedRole)); + } } diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java index 9329a0a8bc6..e8199c0430f 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java @@ -52,14 +52,13 @@ public void addUser(String username, String password) { if (username.startsWith(GROUP_PREFIX)) throw new IllegalArgumentException("Prefix not permitted: " + GROUP_PREFIX); - addUserInternal(username, password); + addUserInternal(username, encryptionSupport.encrypt(password)); } - private void addUserInternal(String username, String password) { + private void addUserInternal(String username, String encPassword) { String[] infos = null; StringBuilder userInfoBuffer = new StringBuilder(); - String encPassword = encryptionSupport.encrypt(password); String userInfos = users.get(username); //If user already exists, update password @@ -139,8 +138,11 @@ private List listRoles(String name) { List result = new ArrayList<>(); String userInfo = users.get(name); String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { + for (int i = getFirstRoleIndex(name); i < infos.length; i++) { String roleName = infos[i]; + if(roleName.trim().isEmpty()) + continue; + if (roleName.startsWith(GROUP_PREFIX)) { for (RolePrincipal rp : listRoles(roleName)) { if (!result.contains(rp)) { @@ -157,22 +159,38 @@ private List listRoles(String name) { return result; } + private int getFirstRoleIndex(String name) { + if (name.trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) { + return 0; + } + return 1; + } + @Override public void addRole(String username, String role) { String userInfos = users.get(username); if (userInfos != null) { - for (RolePrincipal rp : listRoles(username)) { - if (role.equals(rp.getName())) { - return; + + // for groups, empty info should be replaced with role + // for users, empty info means empty password and role should be appended + if(userInfos.trim().isEmpty() + && username.trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) { + users.put(username, role); + + } else { + for (RolePrincipal rp : listRoles(username)) { + if (role.equals(rp.getName())) { + return; + } } - } - for (GroupPrincipal gp : listGroups(username)) { - if (role.equals(GROUP_PREFIX + gp.getName())) { - return; + for (GroupPrincipal gp : listGroups(username)) { + if (role.equals(GROUP_PREFIX + gp.getName())) { + return; + } } + String newUserInfos = userInfos + "," + role; + users.put(username, newUserInfos); } - String newUserInfos = userInfos + "," + role; - users.put(username, newUserInfos); } try { users.save(); @@ -191,12 +209,17 @@ public void deleteRole(String username, String role) { //If user already exists, remove the role if (userInfos != null && userInfos.length() > 0) { infos = userInfos.split(","); - String password = infos[0]; - userInfoBuffer.append(password); - for (int i = 1; i < infos.length; i++) { + int firstRoleIndex = getFirstRoleIndex(username); + if(firstRoleIndex == 1) {// index 0 is password + String password = infos[0]; + userInfoBuffer.append(password); + } + for (int i = firstRoleIndex; i < infos.length; i++) { if (infos[i] != null && !infos[i].equals(role)) { - userInfoBuffer.append(","); + if(userInfoBuffer.length() > 0) { + userInfoBuffer.append(","); + } userInfoBuffer.append(infos[i]); } } @@ -222,7 +245,7 @@ private List listGroups(String userName) { String userInfo = users.get(userName); if (userInfo != null) { String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { + for (int i = getFirstRoleIndex(userName); i < infos.length; i++) { String name = infos[i]; if (name.startsWith(GROUP_PREFIX)) { result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length()))); @@ -236,7 +259,7 @@ private List listGroups(String userName) { public void addGroup(String username, String group) { String groupName = GROUP_PREFIX + group; if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); + addUserInternal(groupName, ""); // groups don't have password } addRole(username, groupName); } @@ -282,7 +305,7 @@ public Map listGroups() { public void createGroup(String group) { String groupName = GROUP_PREFIX + group; if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); + addUserInternal(groupName, ""); // groups don't have password } else { throw new IllegalArgumentException("Group: " + group + " already exist"); } diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java index 556c0fa1d40..dc946d44002 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java @@ -18,8 +18,11 @@ import java.io.File; import java.io.IOException; +import java.security.Principal; import java.util.HashSet; import java.util.Map; +import java.util.Set; + import javax.security.auth.Subject; import javax.security.auth.callback.Callback; import javax.security.auth.callback.CallbackHandler; @@ -141,13 +144,13 @@ public boolean login() throws LoginException { String groupInfo = users.get(infos[i].trim()); if (groupInfo != null) { String[] roles = groupInfo.split(","); - for (int j = 1; j < roles.length; j++) { - principals.add(new RolePrincipal(roles[j].trim())); + for (int j = 0; j < roles.length; j++) { + addRole(principals, roles[j].trim()); } } } else { // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); + addRole(principals, infos[i].trim()); } } @@ -160,4 +163,8 @@ public boolean login() throws LoginException { return true; } + private void addRole(Set principals, String trimmedRole) { + if (!trimmedRole.isEmpty()) + principals.add(new RolePrincipal(trimmedRole)); + } } diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java index 1cb6a28fa8d..1fe4b2d5318 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java @@ -18,12 +18,16 @@ import static org.apache.karaf.jaas.modules.PrincipalHelper.names; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; +import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; @@ -55,7 +59,7 @@ public void testUserRoles() throws IOException { engine.addRole("a", "role2"); UserPrincipal upa = getUser(engine, "a"); - Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); engine.addGroup("a", "g"); engine.addGroupRole("g", "role2"); @@ -64,8 +68,8 @@ public void testUserRoles() throws IOException { engine.addGroup("b", "g2"); engine.addGroupRole("g2", "role4"); - Assert.assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b")); - Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); + assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b")); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); checkLoading(); @@ -79,11 +83,11 @@ public void testUserRoles() throws IOException { GroupPrincipal gp = engine.listGroups(upa).iterator().next(); engine.deleteGroupRole("g", "role2"); - Assert.assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3")); + assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3")); // role2 should still be there as it was added to the user directly too - Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); - Assert.assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4")); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); + assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4")); engine.deleteGroup("b", "g"); engine.deleteGroup("b", "g2"); @@ -101,10 +105,10 @@ private void checkLoading() throws IOException { UserPrincipal upb_2 = getUser(engine, "b"); assertEquals(3, engine.listRoles(upa_2).size()); - Assert.assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3")); + assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3")); assertEquals(3, engine.listRoles(upb_2).size()); - Assert.assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4")); + assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4")); } private UserPrincipal getUser(PropertiesBackingEngine engine, String name) { @@ -114,6 +118,50 @@ private UserPrincipal getUser(PropertiesBackingEngine engine, String name) { return matchingUsers.iterator().next(); } + @Test + public void testUserPassword() throws IOException { + Properties p = new Properties(f); + PropertiesBackingEngine engine = new PropertiesBackingEngine(p); + + // update password when user has no roles + engine.addUser("a", "pass1"); + engine.addUser("a", "pass2"); + assertThat(Arrays.asList(p.get("a").split(",")), contains("pass2")); + UserPrincipal upa = getUser(engine, "a"); + assertTrue(engine.listRoles(upa).isEmpty()); + + // update empty password when user has no roles + engine.addUser("b", ""); + engine.addUser("b", "pass3"); + assertThat(Arrays.asList(p.get("b").split(",")), contains("pass3")); + UserPrincipal upb = getUser(engine, "b"); + assertTrue(engine.listRoles(upb).isEmpty()); + + // update password when user has roles + engine.addUser("c", "pass4"); + engine.addRole("c", "role1"); + engine.addGroup("c", "g1"); + engine.addGroupRole("g1", "role2"); + engine.addUser("c", "pass5"); + assertThat(Arrays.asList(p.get("c").split(",")), + contains("pass5", "role1", PropertiesBackingEngine.GROUP_PREFIX + "g1")); + UserPrincipal upc = getUser(engine, "c"); + assertThat(names(engine.listRoles(upc)), containsInAnyOrder("role1", "role2")); + assertThat(names(engine.listGroups(upc)), containsInAnyOrder("g1")); + + // update empty password when user has roles + engine.addUser("d", ""); + engine.addRole("d", "role3"); + engine.addGroup("d", "g2"); + engine.addGroupRole("g2", "role4"); + engine.addUser("d", "pass6"); + assertThat(Arrays.asList(p.get("d").split(",")), + contains("pass6", "role3", PropertiesBackingEngine.GROUP_PREFIX + "g2")); + UserPrincipal upd = getUser(engine, "d"); + assertThat(names(engine.listRoles(upd)), containsInAnyOrder("role3", "role4")); + assertThat(names(engine.listGroups(upd)), containsInAnyOrder("g2")); + } + @After public void cleanup() { if (!f.delete()) { diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java index 9d43fbaa008..b02dce858d9 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java @@ -110,8 +110,11 @@ public void testLoginWithGroups() throws Exception { pbe.addUser("abc", "xyz"); pbe.addRole("abc", "myrole"); pbe.addUser("pqr", "abc"); + pbe.addRole("pqr", ""); // should be ignored pbe.addGroup("pqr", "group1"); pbe.addGroupRole("group1", "r1"); + pbe.addGroupRole("group1", ""); // should be ignored + pbe.addGroupRole("group1", "r2"); PropertiesLoginModule module = new PropertiesLoginModule(); Map options = new HashMap<>(); @@ -123,10 +126,10 @@ public void testLoginWithGroups() throws Exception { Assert.assertTrue(module.login()); Assert.assertTrue(module.commit()); - Assert.assertEquals(3, subject.getPrincipals().size()); + Assert.assertEquals(4, subject.getPrincipals().size()); assertThat(names(subject.getPrincipals(UserPrincipal.class)), containsInAnyOrder("pqr")); assertThat(names(subject.getPrincipals(GroupPrincipal.class)), containsInAnyOrder("group1")); - assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1")); + assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1", "r2")); } finally { if (!f.delete()) { Assert.fail("Could not delete temporary file: " + f);