Skip to content

Commit

Permalink
KARAF-5014: consider first group role in users.properties and ignore …
Browse files Browse the repository at this point in the history 
…empty roles
  • Loading branch information
@stataru8
stataru8 committed Sep 27, 2024
1 parent efdf64d commit dac3692
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 36 deletions.
13 changes: 10 additions & 3 deletions ...les/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}

Expand All @@ -233,4 +236,8 @@ public boolean login() throws LoginException {
return true;
}

private void addRole(Set<Principal> principals, String trimmedRole) {
if (!trimmedRole.isEmpty())
principals.add(new RolePrincipal(trimmedRole));
}
}
63 changes: 43 additions & 20 deletions ...dules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -139,8 +138,11 @@ private List<RolePrincipal> listRoles(String name) {
List<RolePrincipal> 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)) {
Expand All @@ -157,22 +159,38 @@ private List<RolePrincipal> 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 user, 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();
Expand All @@ -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]);
}
}
Expand All @@ -222,7 +245,7 @@ private List<GroupPrincipal> 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())));
Expand All @@ -236,7 +259,7 @@ private List<GroupPrincipal> 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);
}
Expand Down Expand Up @@ -282,7 +305,7 @@ public Map<GroupPrincipal, String> 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");
}
Expand Down
13 changes: 10 additions & 3 deletions ...modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}

Expand All @@ -160,4 +163,8 @@ public boolean login() throws LoginException {
return true;
}

private void addRole(Set<Principal> principals, String trimmedRole) {
if (!trimmedRole.isEmpty())
principals.add(new RolePrincipal(trimmedRole));
}
}
64 changes: 56 additions & 8 deletions ...s/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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");
Expand All @@ -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();

Expand All @@ -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");
Expand All @@ -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) {
Expand All @@ -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()) {
Expand Down
7 changes: 5 additions & 2 deletions ...les/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String> options = new HashMap<>();
Expand All @@ -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);
Expand Down

0 comments on commit dac3692

Please sign in to comment.