Skip to content

Commit a30ea9d

Browse files
stataru8stataru8
committed
KARAF-5014: consider first group role in users.properties and ignore empty roles
1 parent a8d9901 commit a30ea9d

File tree

5 files changed

+124
-36
lines changed

5 files changed

+124
-36
lines changed

‎jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,11 @@
2121
import java.lang.reflect.Field;
2222
import java.nio.charset.StandardCharsets;
2323
import java.security.MessageDigest;
24+
import java.security.Principal;
2425
import java.util.HashSet;
2526
import java.util.Map;
27+
import java.util.Set;
28+
2629
import javax.security.auth.Subject;
2730
import javax.security.auth.callback.Callback;
2831
import javax.security.auth.callback.CallbackHandler;
@@ -214,13 +217,13 @@ public boolean login() throws LoginException {
214217
String groupInfo = users.get(infos[i].trim());
215218
if (groupInfo != null) {
216219
String[] roles = groupInfo.split(",");
217-
for (int j = 1; j < roles.length; j++) {
218-
principals.add(new RolePrincipal(roles[j].trim()));
220+
for (int j = 0; j < roles.length; j++) {
221+
addRole(principals, roles[j].trim());
219222
}
220223
}
221224
} else {
222225
// it's an user reference
223-
principals.add(new RolePrincipal(infos[i].trim()));
226+
addRole(principals, infos[i].trim());
224227
}
225228
}
226229

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

239+
private void addRole(Set<Principal> principals, String trimmedRole) {
240+
if (!trimmedRole.isEmpty())
241+
principals.add(new RolePrincipal(trimmedRole));
242+
}
236243
}

‎jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java‎

Lines changed: 43 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,13 @@ public void addUser(String username, String password) {
5252
if (username.startsWith(GROUP_PREFIX))
5353
throw new IllegalArgumentException("Prefix not permitted: " + GROUP_PREFIX);
5454

55-
addUserInternal(username, password);
55+
addUserInternal(username, encryptionSupport.encrypt(password));
5656
}
5757

58-
private void addUserInternal(String username, String password) {
58+
private void addUserInternal(String username, String encPassword) {
5959
String[] infos = null;
6060
StringBuilder userInfoBuffer = new StringBuilder();
6161

62-
String encPassword = encryptionSupport.encrypt(password);
6362
String userInfos = users.get(username);
6463

6564
//If user already exists, update password
@@ -139,8 +138,11 @@ private List<RolePrincipal> listRoles(String name) {
139138
List<RolePrincipal> result = new ArrayList<>();
140139
String userInfo = users.get(name);
141140
String[] infos = userInfo.split(",");
142-
for (int i = 1; i < infos.length; i++) {
141+
for (int i = getFirstRoleIndex(name); i < infos.length; i++) {
143142
String roleName = infos[i];
143+
if(roleName.trim().isEmpty())
144+
continue;
145+
144146
if (roleName.startsWith(GROUP_PREFIX)) {
145147
for (RolePrincipal rp : listRoles(roleName)) {
146148
if (!result.contains(rp)) {
@@ -157,22 +159,38 @@ private List<RolePrincipal> listRoles(String name) {
157159
return result;
158160
}
159161

162+
private int getFirstRoleIndex(String name) {
163+
if (name.trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) {
164+
return 0;
165+
}
166+
return 1;
167+
}
168+
160169
@Override
161170
public void addRole(String username, String role) {
162171
String userInfos = users.get(username);
163172
if (userInfos != null) {
164-
for (RolePrincipal rp : listRoles(username)) {
165-
if (role.equals(rp.getName())) {
166-
return;
173+
174+
// for groups, empty info should be replaced with role
175+
// for users, empty info means empty password and role should be appended
176+
if(userInfos.trim().isEmpty()
177+
&& username.trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) {
178+
users.put(username, role);
179+
180+
} else {
181+
for (RolePrincipal rp : listRoles(username)) {
182+
if (role.equals(rp.getName())) {
183+
return;
184+
}
167185
}
168-
}
169-
for (GroupPrincipal gp : listGroups(username)) {
170-
if (role.equals(GROUP_PREFIX + gp.getName())) {
171-
return;
186+
for (GroupPrincipal gp : listGroups(username)) {
187+
if (role.equals(GROUP_PREFIX + gp.getName())) {
188+
return;
189+
}
172190
}
191+
String newUserInfos = userInfos + "," + role;
192+
users.put(username, newUserInfos);
173193
}
174-
String newUserInfos = userInfos + "," + role;
175-
users.put(username, newUserInfos);
176194
}
177195
try {
178196
users.save();
@@ -191,12 +209,17 @@ public void deleteRole(String username, String role) {
191209
//If user already exists, remove the role
192210
if (userInfos != null && userInfos.length() > 0) {
193211
infos = userInfos.split(",");
194-
String password = infos[0];
195-
userInfoBuffer.append(password);
196212

197-
for (int i = 1; i < infos.length; i++) {
213+
int firstRoleIndex = getFirstRoleIndex(username);
214+
if(firstRoleIndex == 1) {// index 0 is password
215+
String password = infos[0];
216+
userInfoBuffer.append(password);
217+
}
218+
for (int i = firstRoleIndex; i < infos.length; i++) {
198219
if (infos[i] != null && !infos[i].equals(role)) {
199-
userInfoBuffer.append(",");
220+
if(userInfoBuffer.length() > 0) {
221+
userInfoBuffer.append(",");
222+
}
200223
userInfoBuffer.append(infos[i]);
201224
}
202225
}
@@ -222,7 +245,7 @@ private List<GroupPrincipal> listGroups(String userName) {
222245
String userInfo = users.get(userName);
223246
if (userInfo != null) {
224247
String[] infos = userInfo.split(",");
225-
for (int i = 1; i < infos.length; i++) {
248+
for (int i = getFirstRoleIndex(userName); i < infos.length; i++) {
226249
String name = infos[i];
227250
if (name.startsWith(GROUP_PREFIX)) {
228251
result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length())));
@@ -236,7 +259,7 @@ private List<GroupPrincipal> listGroups(String userName) {
236259
public void addGroup(String username, String group) {
237260
String groupName = GROUP_PREFIX + group;
238261
if (users.get(groupName) == null) {
239-
addUserInternal(groupName, "group");
262+
addUserInternal(groupName, ""); // groups don't have password
240263
}
241264
addRole(username, groupName);
242265
}
@@ -282,7 +305,7 @@ public Map<GroupPrincipal, String> listGroups() {
282305
public void createGroup(String group) {
283306
String groupName = GROUP_PREFIX + group;
284307
if (users.get(groupName) == null) {
285-
addUserInternal(groupName, "group");
308+
addUserInternal(groupName, ""); // groups don't have password
286309
} else {
287310
throw new IllegalArgumentException("Group: " + group + " already exist");
288311
}

‎jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,11 @@
1818

1919
import java.io.File;
2020
import java.io.IOException;
21+
import java.security.Principal;
2122
import java.util.HashSet;
2223
import java.util.Map;
24+
import java.util.Set;
25+
2326
import javax.security.auth.Subject;
2427
import javax.security.auth.callback.Callback;
2528
import javax.security.auth.callback.CallbackHandler;
@@ -141,13 +144,13 @@ public boolean login() throws LoginException {
141144
String groupInfo = users.get(infos[i].trim());
142145
if (groupInfo != null) {
143146
String[] roles = groupInfo.split(",");
144-
for (int j = 1; j < roles.length; j++) {
145-
principals.add(new RolePrincipal(roles[j].trim()));
147+
for (int j = 0; j < roles.length; j++) {
148+
addRole(principals, roles[j].trim());
146149
}
147150
}
148151
} else {
149152
// it's an user reference
150-
principals.add(new RolePrincipal(infos[i].trim()));
153+
addRole(principals, infos[i].trim());
151154
}
152155
}
153156

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

166+
private void addRole(Set<Principal> principals, String trimmedRole) {
167+
if (!trimmedRole.isEmpty())
168+
principals.add(new RolePrincipal(trimmedRole));
169+
}
163170
}

‎jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java‎

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,16 @@
1818

1919
import static org.apache.karaf.jaas.modules.PrincipalHelper.names;
2020
import static org.hamcrest.Matchers.containsInAnyOrder;
21+
import static org.hamcrest.Matchers.contains;
22+
import static org.hamcrest.MatcherAssert.assertThat;
2123
import static org.junit.Assert.assertEquals;
2224
import static org.junit.Assert.assertNotNull;
25+
import static org.junit.Assert.assertTrue;
2326
import static org.junit.Assert.fail;
2427

2528
import java.io.File;
2629
import java.io.IOException;
30+
import java.util.Arrays;
2731
import java.util.List;
2832
import java.util.stream.Collectors;
2933

@@ -55,7 +59,7 @@ public void testUserRoles() throws IOException {
5559
engine.addRole("a", "role2");
5660

5761
UserPrincipal upa = getUser(engine, "a");
58-
Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2"));
62+
assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2"));
5963

6064
engine.addGroup("a", "g");
6165
engine.addGroupRole("g", "role2");
@@ -64,8 +68,8 @@ public void testUserRoles() throws IOException {
6468
engine.addGroup("b", "g2");
6569
engine.addGroupRole("g2", "role4");
6670

67-
Assert.assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b"));
68-
Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3"));
71+
assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b"));
72+
assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3"));
6973

7074
checkLoading();
7175

@@ -79,11 +83,11 @@ public void testUserRoles() throws IOException {
7983

8084
GroupPrincipal gp = engine.listGroups(upa).iterator().next();
8185
engine.deleteGroupRole("g", "role2");
82-
Assert.assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3"));
86+
assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3"));
8387

8488
// role2 should still be there as it was added to the user directly too
85-
Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3"));
86-
Assert.assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4"));
89+
assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3"));
90+
assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4"));
8791

8892
engine.deleteGroup("b", "g");
8993
engine.deleteGroup("b", "g2");
@@ -101,10 +105,10 @@ private void checkLoading() throws IOException {
101105
UserPrincipal upb_2 = getUser(engine, "b");
102106

103107
assertEquals(3, engine.listRoles(upa_2).size());
104-
Assert.assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3"));
108+
assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3"));
105109

106110
assertEquals(3, engine.listRoles(upb_2).size());
107-
Assert.assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4"));
111+
assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4"));
108112
}
109113

110114
private UserPrincipal getUser(PropertiesBackingEngine engine, String name) {
@@ -114,6 +118,50 @@ private UserPrincipal getUser(PropertiesBackingEngine engine, String name) {
114118
return matchingUsers.iterator().next();
115119
}
116120

121+
@Test
122+
public void testUserPassword() throws IOException {
123+
Properties p = new Properties(f);
124+
PropertiesBackingEngine engine = new PropertiesBackingEngine(p);
125+
126+
// update password when user has no roles
127+
engine.addUser("a", "pass1");
128+
engine.addUser("a", "pass2");
129+
assertThat(Arrays.asList(p.get("a").split(",")), contains("pass2"));
130+
UserPrincipal upa = getUser(engine, "a");
131+
assertTrue(engine.listRoles(upa).isEmpty());
132+
133+
// update empty password when user has no roles
134+
engine.addUser("b", "");
135+
engine.addUser("b", "pass3");
136+
assertThat(Arrays.asList(p.get("b").split(",")), contains("pass3"));
137+
UserPrincipal upb = getUser(engine, "b");
138+
assertTrue(engine.listRoles(upb).isEmpty());
139+
140+
// update password when user has roles
141+
engine.addUser("c", "pass4");
142+
engine.addRole("c", "role1");
143+
engine.addGroup("c", "g1");
144+
engine.addGroupRole("g1", "role2");
145+
engine.addUser("c", "pass5");
146+
assertThat(Arrays.asList(p.get("c").split(",")),
147+
contains("pass5", "role1", PropertiesBackingEngine.GROUP_PREFIX + "g1"));
148+
UserPrincipal upc = getUser(engine, "c");
149+
assertThat(names(engine.listRoles(upc)), containsInAnyOrder("role1", "role2"));
150+
assertThat(names(engine.listGroups(upc)), containsInAnyOrder("g1"));
151+
152+
// update empty password when user has roles
153+
engine.addUser("d", "");
154+
engine.addRole("d", "role3");
155+
engine.addGroup("d", "g2");
156+
engine.addGroupRole("g2", "role4");
157+
engine.addUser("d", "pass6");
158+
assertThat(Arrays.asList(p.get("d").split(",")),
159+
contains("pass6", "role3", PropertiesBackingEngine.GROUP_PREFIX + "g2"));
160+
UserPrincipal upd = getUser(engine, "d");
161+
assertThat(names(engine.listRoles(upd)), containsInAnyOrder("role3", "role4"));
162+
assertThat(names(engine.listGroups(upd)), containsInAnyOrder("g2"));
163+
}
164+
117165
@After
118166
public void cleanup() {
119167
if (!f.delete()) {

‎jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,11 @@ public void testLoginWithGroups() throws Exception {
110110
pbe.addUser("abc", "xyz");
111111
pbe.addRole("abc", "myrole");
112112
pbe.addUser("pqr", "abc");
113+
pbe.addRole("pqr", ""); // should be ignored
113114
pbe.addGroup("pqr", "group1");
114115
pbe.addGroupRole("group1", "r1");
116+
pbe.addGroupRole("group1", ""); // should be ignored
117+
pbe.addGroupRole("group1", "r2");
115118

116119
PropertiesLoginModule module = new PropertiesLoginModule();
117120
Map<String, String> options = new HashMap<>();
@@ -123,10 +126,10 @@ public void testLoginWithGroups() throws Exception {
123126
Assert.assertTrue(module.login());
124127
Assert.assertTrue(module.commit());
125128

126-
Assert.assertEquals(3, subject.getPrincipals().size());
129+
Assert.assertEquals(4, subject.getPrincipals().size());
127130
assertThat(names(subject.getPrincipals(UserPrincipal.class)), containsInAnyOrder("pqr"));
128131
assertThat(names(subject.getPrincipals(GroupPrincipal.class)), containsInAnyOrder("group1"));
129-
assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1"));
132+
assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1", "r2"));
130133
} finally {
131134
if (!f.delete()) {
132135
Assert.fail("Could not delete temporary file: " + f);

0 commit comments

Comments
 (0)