From a2f89f2bf9d64a82c9acb8875ddd6afeef2b151e Mon Sep 17 00:00:00 2001 From: olivier lamy Date: Thu, 20 Apr 2017 12:36:48 +1000 Subject: [PATCH 1/5] implementation for #1481 Add a new base class UserStore Signed-off-by: olivier lamy --- .../jetty/security/PropertyUserStore.java | 63 ++++----------- .../org/eclipse/jetty/security/UserStore.java | 76 +++++++++++++++++++ .../jetty/security/TestLoginService.java | 28 +++++-- pom.xml | 2 +- 4 files changed, 113 insertions(+), 56 deletions(-) create mode 100644 jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/PropertyUserStore.java b/jetty-security/src/main/java/org/eclipse/jetty/security/PropertyUserStore.java index 604dbacef4f..43b58603ea2 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/PropertyUserStore.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/PropertyUserStore.java @@ -18,12 +18,19 @@ package org.eclipse.jetty.security; +import org.eclipse.jetty.util.PathWatcher; +import org.eclipse.jetty.util.PathWatcher.PathWatchEvent; +import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; +import org.eclipse.jetty.util.resource.PathResource; +import org.eclipse.jetty.util.resource.Resource; +import org.eclipse.jetty.util.security.Credential; + import java.io.File; import java.io.IOException; import java.nio.file.Path; -import java.security.Principal; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -31,20 +38,6 @@ import java.util.Map; import java.util.Properties; import java.util.Set; -import javax.security.auth.Subject; - - -import org.eclipse.jetty.server.UserIdentity; -import org.eclipse.jetty.util.PathWatcher; -import org.eclipse.jetty.util.PathWatcher.PathWatchEvent; -import org.eclipse.jetty.util.StringUtil; -import org.eclipse.jetty.util.component.AbstractLifeCycle; -import org.eclipse.jetty.util.log.Log; -import org.eclipse.jetty.util.log.Logger; -import org.eclipse.jetty.util.resource.PathResource; -import org.eclipse.jetty.util.resource.Resource; -import org.eclipse.jetty.util.security.Credential; - /** * PropertyUserStore *

@@ -59,7 +52,7 @@ import org.eclipse.jetty.util.security.Credential; * * If DIGEST Authentication is used, the password must be in a recoverable format, either plain text or OBF:. */ -public class PropertyUserStore extends AbstractLifeCycle implements PathWatcher.Listener +public class PropertyUserStore extends UserStore implements PathWatcher.Listener { private static final Logger LOG = Log.getLogger(PropertyUserStore.class); @@ -69,10 +62,7 @@ public class PropertyUserStore extends AbstractLifeCycle implements PathWatcher. protected PathWatcher pathWatcher; protected boolean hotReload = false; // default is not to reload - protected IdentityService _identityService = new DefaultIdentityService(); protected boolean _firstLoad = true; // true if first load, false from that point on - protected final List _knownUsers = new ArrayList(); - protected final Map _knownUserIdentities = new HashMap(); protected List _listeners; /** @@ -145,12 +135,6 @@ public class PropertyUserStore extends AbstractLifeCycle implements PathWatcher. { _configPath = configPath; } - - /* ------------------------------------------------------------ */ - public UserIdentity getUserIdentity(String userName) - { - return _knownUserIdentities.get(userName); - } /* ------------------------------------------------------------ */ /** @@ -199,8 +183,8 @@ public class PropertyUserStore extends AbstractLifeCycle implements PathWatcher. StringBuilder s = new StringBuilder(); s.append(this.getClass().getName()); s.append("["); - s.append("users.count=").append(this._knownUsers.size()); - s.append("identityService=").append(this._identityService); + s.append("users.count=").append(this.getKnownUserIdentities().size()); + s.append("identityService=").append(this.getIdentityService()); s.append("]"); return s.toString(); } @@ -220,7 +204,7 @@ public class PropertyUserStore extends AbstractLifeCycle implements PathWatcher. if (getConfigResource().exists()) properties.load(getConfigResource().getInputStream()); - Set known = new HashSet(); + Set known = new HashSet<>(); for (Map.Entry entry : properties.entrySet()) { @@ -243,27 +227,12 @@ public class PropertyUserStore extends AbstractLifeCycle implements PathWatcher. } known.add(username); Credential credential = Credential.getCredential(credentials); - - Principal userPrincipal = new AbstractLoginService.UserPrincipal(username,credential); - Subject subject = new Subject(); - subject.getPrincipals().add(userPrincipal); - subject.getPrivateCredentials().add(credential); - - if (roles != null) - { - for (String role : roleArray) - { - subject.getPrincipals().add(new AbstractLoginService.RolePrincipal(role)); - } - } - - subject.setReadOnly(); - - _knownUserIdentities.put(username,_identityService.newUserIdentity(subject,userPrincipal,roleArray)); + addUser( username, credential, roleArray ); notifyUpdate(username,credential,roleArray); } } + final List _knownUsers = new ArrayList(); synchronized (_knownUsers) { /* @@ -277,7 +246,7 @@ public class PropertyUserStore extends AbstractLifeCycle implements PathWatcher. String user = users.next(); if (!known.contains(user)) { - _knownUserIdentities.remove(user); + removeUser( user ); notifyRemove(user); } } diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java b/jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java new file mode 100644 index 00000000000..d23a17e9692 --- /dev/null +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java @@ -0,0 +1,76 @@ +// +// ======================================================================== +// Copyright (c) 1995-2017 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.security; + +import org.eclipse.jetty.server.UserIdentity; +import org.eclipse.jetty.util.component.AbstractLifeCycle; +import org.eclipse.jetty.util.security.Credential; + +import javax.security.auth.Subject; +import java.security.Principal; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Base class to store User + */ +public class UserStore extends AbstractLifeCycle +{ + private IdentityService _identityService = new DefaultIdentityService(); + private final Map _knownUserIdentities = new ConcurrentHashMap<>(); + + protected void addUser( String username, Credential credential, String[] roles) + { + Principal userPrincipal = new AbstractLoginService.UserPrincipal( username, credential); + Subject subject = new Subject(); + subject.getPrincipals().add(userPrincipal); + subject.getPrivateCredentials().add(credential); + + if (roles != null) + { + for (String role : roles) + { + subject.getPrincipals().add(new AbstractLoginService.RolePrincipal(role)); + } + } + + subject.setReadOnly(); + _knownUserIdentities.put(username,_identityService.newUserIdentity(subject,userPrincipal,roles)); + } + + protected void removeUser(String username) + { + _knownUserIdentities.remove(username); + } + + public UserIdentity getUserIdentity(String userName) + { + return _knownUserIdentities.get(userName); + } + + public IdentityService getIdentityService() + { + return _identityService; + } + + protected Map getKnownUserIdentities() + { + return _knownUserIdentities; + } +} diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/TestLoginService.java b/jetty-security/src/test/java/org/eclipse/jetty/security/TestLoginService.java index e8adc097cf8..a977e9bdd31 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/TestLoginService.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/TestLoginService.java @@ -19,9 +19,14 @@ package org.eclipse.jetty.security; +import java.security.Principal; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Set; +import org.eclipse.jetty.server.UserIdentity; import org.eclipse.jetty.util.security.Credential; /** @@ -31,9 +36,8 @@ import org.eclipse.jetty.util.security.Credential; */ public class TestLoginService extends AbstractLoginService { - protected Map _users = new HashMap<>(); - protected Map _roles = new HashMap<>(); - + + UserStore userStore = new UserStore(); public TestLoginService(String name) @@ -43,9 +47,7 @@ public class TestLoginService extends AbstractLoginService public void putUser (String username, Credential credential, String[] roles) { - UserPrincipal userPrincipal = new UserPrincipal(username,credential); - _users.put(username, userPrincipal); - _roles.put(username, roles); + userStore.addUser( username, credential, roles ); } /** @@ -54,7 +56,16 @@ public class TestLoginService extends AbstractLoginService @Override protected String[] loadRoleInfo(UserPrincipal user) { - return _roles.get(user.getName()); + UserIdentity userIdentity = userStore.getUserIdentity( user.getName() ); + Set roles = userIdentity.getSubject().getPrincipals( RolePrincipal.class); + if (roles == null) + return null; + + List list = new ArrayList<>(); + for (RolePrincipal r:roles) + list.add(r.getName()); + + return list.toArray(new String[roles.size()]); } /** @@ -63,7 +74,8 @@ public class TestLoginService extends AbstractLoginService @Override protected UserPrincipal loadUserInfo(String username) { - return _users.get(username); + UserIdentity userIdentity = userStore.getUserIdentity( username ); + return userIdentity == null ? null : (UserPrincipal) userIdentity.getUserPrincipal(); } } diff --git a/pom.xml b/pom.xml index baf709ed44f..f593767451d 100644 --- a/pom.xml +++ b/pom.xml @@ -581,7 +581,7 @@ org.apache.maven.plugins maven-surefire-plugin - 2.19.1 + 2.20 @{argLine} -Dfile.encoding=UTF-8 -Duser.language=en -Duser.region=US -showversion -Xmx1g -Xms1g -XX:+PrintGCDetails false From 99a98127cd9806fdd765a1400dc3db9dac9616b5 Mon Sep 17 00:00:00 2001 From: olivier lamy Date: Thu, 20 Apr 2017 12:50:14 +1000 Subject: [PATCH 2/5] #1481 HashLogin has now a setter to configure the UserStore implementation to use Signed-off-by: olivier lamy --- .../jetty/security/HashLoginService.java | 43 +++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java b/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java index 04ecbcaf208..360a8af3e4a 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java @@ -49,11 +49,12 @@ public class HashLoginService extends AbstractLoginService { private static final Logger LOG = Log.getLogger(HashLoginService.class); - private PropertyUserStore _propertyUserStore; private File _configFile; private Resource _configResource; private boolean hotReload = false; // default is not to reload - + private UserStore _userStore; + + /* ------------------------------------------------------------ */ public HashLoginService() @@ -139,13 +140,21 @@ public class HashLoginService extends AbstractLoginService this.hotReload = enable; } - + /** + * Configure the {@link UserStore} implementation to use. + * If none, for backward compat if none the {@link PropertyUserStore} will be used + * @param userStore the {@link UserStore} implementation to use + */ + public void setUserStore(UserStore userStore) + { + this._userStore = userStore; + } /* ------------------------------------------------------------ */ @Override protected String[] loadRoleInfo(UserPrincipal user) { - UserIdentity id = _propertyUserStore.getUserIdentity(user.getName()); + UserIdentity id = _userStore.getUserIdentity(user.getName()); if (id == null) return null; @@ -162,13 +171,11 @@ public class HashLoginService extends AbstractLoginService } - - /* ------------------------------------------------------------ */ @Override protected UserPrincipal loadUserInfo(String userName) { - UserIdentity id = _propertyUserStore.getUserIdentity(userName); + UserIdentity id = _userStore.getUserIdentity(userName); if (id != null) { return (UserPrincipal)id.getUserPrincipal(); @@ -187,16 +194,18 @@ public class HashLoginService extends AbstractLoginService protected void doStart() throws Exception { super.doStart(); - - if (_propertyUserStore == null) + + // can be null so we switch to previous behaviour using PropertyUserStore + if (_userStore == null) { if(LOG.isDebugEnabled()) LOG.debug("doStart: Starting new PropertyUserStore. PropertiesFile: " + _configFile + " hotReload: " + hotReload); - - _propertyUserStore = new PropertyUserStore(); - _propertyUserStore.setHotReload(hotReload); - _propertyUserStore.setConfigPath(_configFile); - _propertyUserStore.start(); + + PropertyUserStore propertyUserStore = new PropertyUserStore(); + propertyUserStore.setHotReload(hotReload); + propertyUserStore.setConfigPath(_configFile); + propertyUserStore.start(); + this._userStore = propertyUserStore; } } @@ -208,8 +217,8 @@ public class HashLoginService extends AbstractLoginService protected void doStop() throws Exception { super.doStop(); - if (_propertyUserStore != null) - _propertyUserStore.stop(); - _propertyUserStore = null; + if (_userStore != null) + _userStore.stop(); + _userStore = null; } } From 6a88e13b3ab89e638841b5a4cb9c640a69f1a72a Mon Sep 17 00:00:00 2001 From: olivier lamy Date: Thu, 20 Apr 2017 14:24:15 +1000 Subject: [PATCH 3/5] make those methods public for external use Signed-off-by: olivier lamy --- .../src/main/java/org/eclipse/jetty/security/UserStore.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java b/jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java index d23a17e9692..faad5a96776 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/UserStore.java @@ -35,7 +35,7 @@ public class UserStore extends AbstractLifeCycle private IdentityService _identityService = new DefaultIdentityService(); private final Map _knownUserIdentities = new ConcurrentHashMap<>(); - protected void addUser( String username, Credential credential, String[] roles) + public void addUser( String username, Credential credential, String[] roles) { Principal userPrincipal = new AbstractLoginService.UserPrincipal( username, credential); Subject subject = new Subject(); @@ -54,7 +54,7 @@ public class UserStore extends AbstractLifeCycle _knownUserIdentities.put(username,_identityService.newUserIdentity(subject,userPrincipal,roles)); } - protected void removeUser(String username) + public void removeUser(String username) { _knownUserIdentities.remove(username); } @@ -69,7 +69,7 @@ public class UserStore extends AbstractLifeCycle return _identityService; } - protected Map getKnownUserIdentities() + public Map getKnownUserIdentities() { return _knownUserIdentities; } From 626327fa67dab948fc070e98978647cd84f61199 Mon Sep 17 00:00:00 2001 From: olivier lamy Date: Thu, 20 Apr 2017 15:01:41 +1000 Subject: [PATCH 4/5] add as simple unit test #1481 --- .../jetty/security/HashLoginService.java | 7 +- .../eclipse/jetty/security/UserStoreTest.java | 70 +++++++++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 jetty-security/src/test/java/org/eclipse/jetty/security/UserStoreTest.java diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java b/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java index 360a8af3e4a..87559a22938 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java @@ -22,6 +22,7 @@ import java.io.File; import java.util.ArrayList; import java.util.List; import java.util.Set; +import java.util.stream.Collectors; import org.eclipse.jetty.server.UserIdentity; import org.eclipse.jetty.util.log.Log; @@ -163,9 +164,9 @@ public class HashLoginService extends AbstractLoginService if (roles == null) return null; - List list = new ArrayList<>(); - for (RolePrincipal r:roles) - list.add(r.getName()); + List list = roles.stream() + .map( rolePrincipal -> rolePrincipal.getName() ) + .collect( Collectors.toList() ); return list.toArray(new String[roles.size()]); } diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/UserStoreTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/UserStoreTest.java new file mode 100644 index 00000000000..cd61c4c5cb2 --- /dev/null +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/UserStoreTest.java @@ -0,0 +1,70 @@ +// +// ======================================================================== +// Copyright (c) 1995-2017 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.security; + +import org.eclipse.jetty.server.UserIdentity; +import org.eclipse.jetty.util.security.Credential; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +public class UserStoreTest +{ + UserStore userStore; + + @Before + public void setup() { + userStore = new UserStore(); + } + + @Test + public void addUser() + { + this.userStore.addUser( "foo", Credential.getCredential( "beer" ), new String[]{"pub"} ); + Assert.assertEquals(1, this.userStore.getKnownUserIdentities().size()); + UserIdentity userIdentity = this.userStore.getUserIdentity( "foo" ); + Assert.assertNotNull( userIdentity ); + Assert.assertEquals( "foo", userIdentity.getUserPrincipal().getName() ); + Set + roles = userIdentity.getSubject().getPrincipals( AbstractLoginService.RolePrincipal.class); + List list = roles.stream() + .map( rolePrincipal -> rolePrincipal.getName() ) + .collect( Collectors.toList() ); + Assert.assertEquals(1, list.size()); + Assert.assertEquals( "pub", list.get( 0 ) ); + } + + @Test + public void removeUser() + { + this.userStore.addUser( "foo", Credential.getCredential( "beer" ), new String[]{"pub"} ); + Assert.assertEquals(1, this.userStore.getKnownUserIdentities().size()); + UserIdentity userIdentity = this.userStore.getUserIdentity( "foo" ); + Assert.assertNotNull( userIdentity ); + Assert.assertEquals( "foo", userIdentity.getUserPrincipal().getName() ); + userStore.removeUser( "foo" ); + userIdentity = this.userStore.getUserIdentity( "foo" ); + Assert.assertNull( userIdentity ); + } + +} From 9f3841d8e08dcb06f0a52346929617a30d0da0be Mon Sep 17 00:00:00 2001 From: olivier lamy Date: Thu, 20 Apr 2017 15:41:26 +1000 Subject: [PATCH 5/5] fix after review Signed-off-by: olivier lamy --- .../java/org/eclipse/jetty/security/HashLoginService.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java b/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java index 87559a22938..e1ee2cf4d09 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java @@ -54,7 +54,7 @@ public class HashLoginService extends AbstractLoginService private Resource _configResource; private boolean hotReload = false; // default is not to reload private UserStore _userStore; - + private boolean _userStoreAutoCreate = false; /* ------------------------------------------------------------ */ @@ -206,7 +206,8 @@ public class HashLoginService extends AbstractLoginService propertyUserStore.setHotReload(hotReload); propertyUserStore.setConfigPath(_configFile); propertyUserStore.start(); - this._userStore = propertyUserStore; + _userStore = propertyUserStore; + _userStoreAutoCreate = true; } } @@ -218,7 +219,7 @@ public class HashLoginService extends AbstractLoginService protected void doStop() throws Exception { super.doStop(); - if (_userStore != null) + if (_userStore != null && _userStoreAutoCreate) _userStore.stop(); _userStore = null; }