From f73724076ce240f2b30a7ecfdc82aeb6f0abf739 Mon Sep 17 00:00:00 2001 From: Jakub Sawicki Date: Tue, 12 Sep 2017 17:06:06 +0200 Subject: [PATCH 1/2] Base user authorities and roles based on CMDB The process of fetching owners of providers is placed in CmdbOwnerService. It fetches info about all the CMDB providers and caches these values in a manner, which makes querying it fast. The cache is invalidated every x seconds. The process of determining correct authorities is performed in PortalUserImpl every call to the getAuthorities method, which guarantees the application always gets correct authorities. --- .../ltos/repository/CmdbRepository.java | 19 +-- .../security/AuthenticationProviderDev.java | 61 +++++----- .../ltos/security/AuthenticationService.java | 82 +++++-------- .../ltos/security/CmdbOwnerService.java | 101 ++++++++++++++++ .../OpenIDConnectAuthenticationFilter.java | 2 +- .../pl/cyfronet/ltos/security/PortalUser.java | 78 +----------- .../ltos/security/PortalUserFactory.java | 18 +++ .../ltos/security/PortalUserImpl.java | 111 ++++++++++++++++++ .../security/RestAuthenticationFilter.java | 15 ++- .../ltos/repository/AuthenticationMocks.java | 31 +++-- .../ltos/security/CmdbOwnerServiceTest.java | 105 +++++++++++++++++ 11 files changed, 439 insertions(+), 184 deletions(-) create mode 100644 src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java create mode 100644 src/main/java/pl/cyfronet/ltos/security/PortalUserFactory.java create mode 100644 src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java create mode 100644 src/test/java/pl/cyfronet/ltos/security/CmdbOwnerServiceTest.java diff --git a/src/main/java/pl/cyfronet/ltos/repository/CmdbRepository.java b/src/main/java/pl/cyfronet/ltos/repository/CmdbRepository.java index 45cd2af..d633f15 100644 --- a/src/main/java/pl/cyfronet/ltos/repository/CmdbRepository.java +++ b/src/main/java/pl/cyfronet/ltos/repository/CmdbRepository.java @@ -1,32 +1,37 @@ package pl.cyfronet.ltos.repository; -import java.util.Map; - -import javax.annotation.PostConstruct; - import org.json.JSONObject; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.security.oauth2.client.OAuth2RestOperations; import org.springframework.stereotype.Component; - import pl.cyfronet.bazaar.engine.extension.metric.SiteSelectMetric; +import javax.annotation.PostConstruct; +import java.util.Map; + @Component public class CmdbRepository { @Value("${cmdb.url}") private String cmdbUrl; + @Value("${cmdb.url.prefix:/cmdb}") + private String prefix; + @Autowired private OAuth2RestOperations restTemplate; public JSONObject get(String type, String fieldName, String fieldValue) { return new JSONObject(restTemplate - .getForObject(cmdbUrl + "/cmdb/" + type + "/filters/" + fieldName + "/" + fieldValue, Map.class)); + .getForObject(cmdbUrl + prefix + type + "/filters/" + fieldName + "/" + fieldValue, Map.class)); } public JSONObject get(String type) { - return new JSONObject(restTemplate.getForObject(cmdbUrl + "/cmdb/" + type + "/list", Map.class)); + return new JSONObject(restTemplate.getForObject(cmdbUrl + prefix + type + "/list", Map.class)); + } + + public JSONObject getById(String type, String id) { + return new JSONObject(restTemplate.getForObject(cmdbUrl + prefix + type + "/id/" + id, Map.class)); } @PostConstruct diff --git a/src/main/java/pl/cyfronet/ltos/security/AuthenticationProviderDev.java b/src/main/java/pl/cyfronet/ltos/security/AuthenticationProviderDev.java index 52123bb..2870d67 100644 --- a/src/main/java/pl/cyfronet/ltos/security/AuthenticationProviderDev.java +++ b/src/main/java/pl/cyfronet/ltos/security/AuthenticationProviderDev.java @@ -4,10 +4,8 @@ import com.agreemount.bean.identity.TeamMember; import com.agreemount.bean.identity.provider.IdentityProvider; import com.google.common.base.Preconditions; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Value; import org.springframework.security.authentication.AuthenticationProvider; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; @@ -22,7 +20,6 @@ import pl.cyfronet.ltos.bean.Team; import pl.cyfronet.ltos.bean.User; import pl.cyfronet.ltos.repository.TeamRepository; -import pl.cyfronet.ltos.security.PortalUser.PortalUserBuilder; import javax.persistence.EntityManager; import javax.persistence.PersistenceContext; @@ -35,19 +32,20 @@ import java.util.List; import java.util.stream.Collectors; +@Slf4j public class AuthenticationProviderDev implements AuthenticationProvider { - static Logger log = LoggerFactory - .getLogger(AuthenticationProviderDev.class); + @Autowired + private UserOperations operations; @Autowired - UserOperations operations; + private IdentityProvider identityProvider; @Autowired - IdentityProvider identityProvider; + private TeamRepository teamRepository; @Autowired - TeamRepository teamRepository; + private PortalUserFactory portalUserFactory; @Override public Authentication authenticate(Authentication authentication) @@ -60,7 +58,7 @@ public Authentication authenticate(Authentication authentication) + authentication.getAuthorities().toString()); log.debug("LOGGING: details = " + operations); log.debug("LOGGING: creation = " + operations ); - PortalUserBuilder builder = PortalUser.builder(); + PortalUserImpl.Data.DataBuilder builder = PortalUserImpl.Data.builder(); builder.credentials(authentication.getCredentials()); return shouldAuthenticateAgainstThirdPartySystem(name, password, builder); } else if (authentication.getClass().isAssignableFrom(TestingAuthenticationToken.class)) { @@ -91,8 +89,10 @@ public Identity getIdentity(User user) { return identity; } - private PortalUser shouldAuthenticateAgainstThirdPartySystem(String name, - String password, PortalUserBuilder builder) { + private PortalUser shouldAuthenticateAgainstThirdPartySystem( + String name, + String password, + PortalUserImpl.Data.DataBuilder builder) { if (name.equals("admin") && password.equals("admin")) { User user = operations.loadUserByUnityIdentity("admin"); UserInfo info = UserInfo.fromUser(user); @@ -116,12 +116,10 @@ private PortalUser shouldAuthenticateAgainstThirdPartySystem(String name, } builder.user(user); builder.principal(info); - builder.authorities(Arrays.asList( - new SimpleGrantedAuthority("ROLE_ADMIN"), - new SimpleGrantedAuthority("ROLE_PROVIDER"), - new SimpleGrantedAuthority("ROLE_USER")) - ); - builder.isAuthenticated(true); + builder.authority(new SimpleGrantedAuthority("ROLE_ADMIN")); + builder.authority(new SimpleGrantedAuthority("ROLE_PROVIDER")); + builder.authority(new SimpleGrantedAuthority("ROLE_USER")); + builder.authenticated(true); Identity identity = getIdentity(user); Preconditions.checkNotNull(identity, "Identity [%s] was not found", name); @@ -129,7 +127,7 @@ private PortalUser shouldAuthenticateAgainstThirdPartySystem(String name, log.debug("LOGGING: identity for engine set = " + identity ); - return builder.build(); + return portalUserFactory.createPortalUser(builder.build()); } if (name.equals("user") && password.equals("user")) { User user = operations.loadUserByUnityIdentity("user"); @@ -152,7 +150,7 @@ private PortalUser shouldAuthenticateAgainstThirdPartySystem(String name, new SimpleGrantedAuthority("ROLE_USER"), new SimpleGrantedAuthority("ROLE_MANAGER") )); - builder.isAuthenticated(true); + builder.authenticated(true); Identity identity = getIdentity(user); Preconditions.checkNotNull(identity, "Identity [%s] was not found", name); @@ -160,19 +158,18 @@ private PortalUser shouldAuthenticateAgainstThirdPartySystem(String name, log.debug("LOGGING: identity for engine set = " + identity ); - return builder.build(); + return portalUserFactory.createPortalUser(builder.build()); } if (name.equals("noreg")) { UserInfo info = createNoregInfo(); builder.principal(info); - builder.isAuthenticated(true); - + builder.authenticated(true); - return builder.build(); + return portalUserFactory.createPortalUser(builder.build()); } return null; } - + private UserInfo createAdminInfo() { UserInfo info = UserInfo.builder() .unityPersistentIdentity("admin") @@ -182,7 +179,7 @@ private UserInfo createAdminInfo() { .email("adam@adminowski.pl").build(); return info; } - + private UserInfo createUserInfo() { UserInfo info = UserInfo.builder() .unityPersistentIdentity("user") @@ -192,7 +189,7 @@ private UserInfo createUserInfo() { .email("ukasz@userowski.pl").build(); return info; } - + private UserInfo createNoregInfo() { UserInfo info = UserInfo.builder() .unityPersistentIdentity("user") @@ -221,7 +218,7 @@ public User saveUser(User user) { em.flush(); return user; } - + @Transactional public Role saveRole(Role role) { em.persist(role); @@ -230,7 +227,7 @@ public Role saveRole(Role role) { } /* - * Consider new version of spring data rest - such + * Consider new version of spring data rest - such * searches are probably implemented by repositories */ @Transactional @@ -247,7 +244,7 @@ public User loadUserByUnityIdentity(String username) } return principal; } - + @Transactional public Role loadOrCreateRoleByName(String roleName) throws UsernameNotFoundException { @@ -267,7 +264,7 @@ public Role loadOrCreateRoleByName(String roleName) } return theRole; } - + } -} \ No newline at end of file +} diff --git a/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java b/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java index e49b086..1761893 100644 --- a/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java +++ b/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java @@ -1,31 +1,26 @@ package pl.cyfronet.ltos.security; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; - -import javax.net.ssl.HttpsURLConnection; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import com.agreemount.bean.identity.Identity; +import com.agreemount.bean.identity.provider.IdentityProvider; +import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.oauth2.client.OAuth2RestOperations; import org.springframework.stereotype.Service; - -import com.agreemount.bean.identity.Identity; -import com.agreemount.bean.identity.provider.IdentityProvider; - -import pl.cyfronet.ltos.bean.Role; import pl.cyfronet.ltos.bean.User; +import pl.cyfronet.ltos.repository.CmdbRepository; import pl.cyfronet.ltos.repository.UserRepository; import pl.cyfronet.ltos.security.AuthenticationProviderDev.UserOperations; +import javax.net.ssl.HttpsURLConnection; +import java.util.*; +import java.util.stream.Collectors; + @Service +@Slf4j public class AuthenticationService { - private static final Logger log = LoggerFactory.getLogger(AuthenticationService.class); + private static String AUTHORITY_ROLE_PREFIX = "ROLE_"; @Value("${unity.server.base}") private String authorizeUrl; @@ -48,15 +43,21 @@ public class AuthenticationService { @Autowired IdentityProvider identityProvider; - //development variable, users whose email matches this will have provider role assigned - @Value("${provider.email:null}") - private String providerEmail; + @Autowired + private CmdbRepository cmdbRepository; - public void engineLogin(User user) { - Identity identity = new Identity(); - identity.setLogin(user.getEmail()); - identity.setRoles(user.getRoles().stream().map(entry -> entry.getName()).collect(Collectors.toList())); + @Autowired + private PortalUserFactory portalUserFactory; + public void engineLogin(PortalUser user) { + Identity identity = new Identity(); + identity.setLogin(user.getUserBean().getEmail()); + identity.setRoles(new ArrayList<>(user.getUserBean().getRoles().stream() + .map(entry -> entry.getName()) + .collect(Collectors.toList()))); + if (user.getAuthorities().contains(new SimpleGrantedAuthority("ROLE_PROVIDER"))) { + identity.getRoles().add("provider"); + } identityProvider.setIdentity(identity); } @@ -69,13 +70,16 @@ private PortalUser getPortalUser(UserInfo userInfo) { User user = getUser(userInfo); userInfo.setId(user.getId()); - PortalUser.PortalUserBuilder builder = PortalUser.builder(); - builder.isAuthenticated(true); + PortalUserImpl.Data.DataBuilder builder = PortalUserImpl.Data.builder(); + builder.authenticated(true); builder.user(user); - builder.authorities(getRoles(user)); + builder.authorities(user.getRoles().stream() + .map(role -> AUTHORITY_ROLE_PREFIX + role.getName().toUpperCase(Locale.US)) + .map(name -> new SimpleGrantedAuthority(name)) + .collect(Collectors.toList())); builder.principal(userInfo); - return builder.build(); + return portalUserFactory.createPortalUser(builder.build()); } private UserInfo getUserInfo() { @@ -98,32 +102,6 @@ private User getUser(UserInfo userInfo) { userRepository.save(user); } - - Role providerRole = userOperations.loadOrCreateRoleByName("provider"); - if (user.getEmail().equals(providerEmail) && !user.hasRole("provider")) { - if (!user.getRoles().contains(providerRole)) { - user.getRoles().add(providerRole); - userRepository.save(user); - } - } else { // remove if not in settings - ArrayList elementsToRemove = new ArrayList<>(); - for (Role role : user.getRoles()) { - if (role.getName().equals(providerRole.getName())) { - elementsToRemove.add(role); - } - } - if (elementsToRemove.size() > 0) { - user.getRoles().removeAll(elementsToRemove); - userRepository.save(user); - } - } - return user; } - - private List getRoles(User user) { - return user.getRoles().stream() - .map(r -> new SimpleGrantedAuthority("ROLE_" + r.getName().toUpperCase())) - .collect(Collectors.toList()); - } } diff --git a/src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java b/src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java new file mode 100644 index 0000000..3e26a08 --- /dev/null +++ b/src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java @@ -0,0 +1,101 @@ +package pl.cyfronet.ltos.security; + +import lombok.extern.slf4j.Slf4j; +import org.json.JSONArray; +import org.json.JSONException; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.stereotype.Service; +import org.springframework.web.client.RestClientException; +import pl.cyfronet.ltos.repository.CmdbRepository; + +import java.util.*; + +@Service +@Slf4j +public class CmdbOwnerService { + + private long lastRefresh = 0L; + + private Map> ownerEmailToProviders = null; + + private final long refreshInterval; + + private final CmdbRepository cmdbRepository; + + @Autowired + public CmdbOwnerService( + CmdbRepository cmdbRepository, + @Value("${cmdb.owner.refreshInterval:5000}") long refreshInterval) { + this.cmdbRepository = cmdbRepository; + this.refreshInterval = refreshInterval; + } + + public Set getOwnedProviders(String email) { + if (isCacheInvalid()) { + synchronized (this) { + // If entering critical section after waiting on lock, the cache may already have + // been refreshed. So check again. + if (isCacheInvalid()) { + doRefresh(); + } + } + } + return ownerEmailToProviders.getOrDefault(email, Collections.EMPTY_SET); + } + + public void invalidateCache() { + lastRefresh = 0L; + } + + private boolean isCacheInvalid() { + return System.currentTimeMillis() - lastRefresh > refreshInterval; + } + + private void doRefresh() { + try { + try { + Map> map = new HashMap<>(); + + // expects a response in a shape of: { rows: [ { id: , ... }, ... ] } + JSONArray providersJSONArray = cmdbRepository.get("provider").getJSONArray("rows"); + List providerIds = new ArrayList<>(); + for (int i = 0; i < providersJSONArray.length(); i++) { + providerIds.add(providersJSONArray.getJSONObject(i).getString("id")); + } + + for (String providerId : providerIds) { + // expects { data: { owners: [ , , ... ], ... } } + // however, ignore entries with no owners field + JSONArray ownersJSONArray = cmdbRepository.getById("provider", providerId) + .getJSONObject("data") + .optJSONArray("owners"); + if (ownersJSONArray != null && ownersJSONArray.length() > 0) { + for (int i = 0; i < ownersJSONArray.length(); i++) { + String email = ownersJSONArray.getString(i); + Set providerIdsSet = map.getOrDefault(email, new HashSet<>()); + providerIdsSet.add(providerId); + map.put(email, providerIdsSet); + } + } + } + + ownerEmailToProviders = map; + } catch (Exception e) { + // ensure a clean map here to avoid it being in some dirty state + ownerEmailToProviders = Collections.EMPTY_MAP; + throw e; + } + // just log, handling the permissions in other way wouldn't make sense anyway + // maybe in the next refreshInterval millis everything will fix itself + } catch (RestClientException e) { + log.warn("Couldn't execute request to CMDB", e); + } catch (JSONException e) { + log.warn("Malformed response from CMDB", e); + } catch (Exception e) { + log.warn("Unexpected exception", e); + } finally { + lastRefresh = System.currentTimeMillis(); + } + } +} diff --git a/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java b/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java index d05be84..97d2fd3 100644 --- a/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java +++ b/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java @@ -51,7 +51,7 @@ public Authentication attemptAuthentication(HttpServletRequest request, HttpServ throws AuthenticationException, IOException, ServletException { try { PortalUser portalUser = authenticationService.getPortalUser(); - authenticationService.engineLogin(portalUser.getUserBean()); + authenticationService.engineLogin(portalUser); return portalUser; } catch (UserDeniedAuthorizationException | InvalidRequestException ex) { diff --git a/src/main/java/pl/cyfronet/ltos/security/PortalUser.java b/src/main/java/pl/cyfronet/ltos/security/PortalUser.java index 64347f0..c72135e 100644 --- a/src/main/java/pl/cyfronet/ltos/security/PortalUser.java +++ b/src/main/java/pl/cyfronet/ltos/security/PortalUser.java @@ -1,83 +1,11 @@ package pl.cyfronet.ltos.security; -import java.util.Collection; - -import lombok.Builder; - import org.springframework.security.core.Authentication; -import org.springframework.security.core.GrantedAuthority; - import pl.cyfronet.ltos.bean.User; import pl.cyfronet.ltos.security.policy.Identity; -/** - * @author bwilk - * - */ -@Builder -public class PortalUser implements Authentication, Identity { - - private static final long serialVersionUID = 1L; - - private boolean isAuthenticated; - - private String name; - private Object credentials; - - private UserInfo principal; - private User user; - private Collection authorities; - - @Override - public String getName() { - return name; +public interface PortalUser extends Authentication, Identity { + default User getUserBean() { + return (User) getDetails(); } - - @Override - public Collection getAuthorities() { - return authorities; - } - - @Override - public Object getCredentials() { - return credentials; - } - - public void setDetails(User details) { - if (principal != null && details.getId() != null) { - principal.setId(details.getId()); - } - this.user = details; - } - - @Override - public Object getDetails() { - return user; - } - - @Override - public Object getPrincipal() { - return principal; - } - - @Override - public boolean isAuthenticated() { - return isAuthenticated; - } - - @Override - public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException { - this.isAuthenticated = isAuthenticated; - - } - - public User getUserBean() { - return user; - } - - @Override - public Long getId() { - return user.getId(); - } - } diff --git a/src/main/java/pl/cyfronet/ltos/security/PortalUserFactory.java b/src/main/java/pl/cyfronet/ltos/security/PortalUserFactory.java new file mode 100644 index 0000000..13b2220 --- /dev/null +++ b/src/main/java/pl/cyfronet/ltos/security/PortalUserFactory.java @@ -0,0 +1,18 @@ +package pl.cyfronet.ltos.security; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.config.AutowireCapableBeanFactory; +import org.springframework.stereotype.Service; + +@Service +public class PortalUserFactory { + + @Autowired + private AutowireCapableBeanFactory beanFactory; + + public PortalUserImpl createPortalUser(PortalUserImpl.Data data) { + PortalUserImpl portalUser = new PortalUserImpl(data); + beanFactory.autowireBean(portalUser); + return portalUser; + } +} diff --git a/src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java b/src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java new file mode 100644 index 0000000..b090e86 --- /dev/null +++ b/src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java @@ -0,0 +1,111 @@ +package pl.cyfronet.ltos.security; + +import lombok.Builder; +import lombok.Getter; +import lombok.Singular; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import pl.cyfronet.ltos.bean.User; + +import java.util.HashSet; +import java.util.Set; +import java.util.stream.Collectors; + +import static java.util.Collections.unmodifiableSet; + +public class PortalUserImpl implements PortalUser { + + private static String AUTHORITY_ROLE_PREFIX = "ROLE_"; + private static String PROVIDER_AUTHORITY_ROLE_PREFIX = AUTHORITY_ROLE_PREFIX + "PROVIDER_"; + + @Builder + @Getter + public static class Data { + private boolean authenticated; + + private final String name; + private final Object credentials; + + private final UserInfo principal; + private final User user; + @Singular private final Set authorities; + } + + private final Data data; + + private Set authorities = null; + + @Autowired + private CmdbOwnerService cmdbOwnerService; + + //development variable, users whose email matches this will have provider role assigned + @Value("${provider.email:null}") + private Set devProviderEmails; + + public PortalUserImpl(Data data) { + this.data = data; + } + + @Override + public String getName() { + return data.name; + } + + @Override + public Set getAuthorities() { + Set authorities = new HashSet<>(data.authorities); + Set ownedProviders = cmdbOwnerService.getOwnedProviders(data.user.getEmail()); + if (ownedProviders != null && !ownedProviders.isEmpty()) { + authorities.add(new SimpleGrantedAuthority(AUTHORITY_ROLE_PREFIX + "PROVIDER")); + authorities.addAll(ownedProviders.stream() + .map(role -> PROVIDER_AUTHORITY_ROLE_PREFIX + role) + .map(authName -> new SimpleGrantedAuthority(authName)) + .collect(Collectors.toList())); + } else if (devProviderEmails.contains(data.user.getEmail())) { + authorities.add(new SimpleGrantedAuthority(AUTHORITY_ROLE_PREFIX + "PROVIDER")); + } + return unmodifiableSet(authorities); + } + + @Override + public Object getCredentials() { + return data.credentials; + } + + @Override + public Object getDetails() { + return data.user; + } + + @Override + public Object getPrincipal() { + return data.principal; + } + + @Override + public boolean isAuthenticated() { + return data.authenticated; + } + + @Override + public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException { + if (!data.authenticated && isAuthenticated) { + // following the guideline from the method's definition to only allow to invalidate Authentication + // allowing the reverse may pose a security problem + throw new IllegalArgumentException("You cannot set authenticated to true on an already invalidated Authentication instance. Create a new one instead."); + } + data.authenticated = isAuthenticated; + } + + public User getUserBean() { + return data.user; + } + + @Override + public Long getId() { + return data.user.getId(); + } + +} diff --git a/src/main/java/pl/cyfronet/ltos/security/RestAuthenticationFilter.java b/src/main/java/pl/cyfronet/ltos/security/RestAuthenticationFilter.java index 5551b8a..04ae61e 100644 --- a/src/main/java/pl/cyfronet/ltos/security/RestAuthenticationFilter.java +++ b/src/main/java/pl/cyfronet/ltos/security/RestAuthenticationFilter.java @@ -1,12 +1,5 @@ package pl.cyfronet.ltos.security; -import java.io.IOException; - -import javax.servlet.FilterChain; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; @@ -17,6 +10,12 @@ import org.springframework.security.oauth2.provider.authentication.BearerTokenExtractor; import org.springframework.web.filter.OncePerRequestFilter; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.io.IOException; + public class RestAuthenticationFilter extends OncePerRequestFilter { private static final Logger log = LoggerFactory.getLogger(RestAuthenticationFilter.class); @@ -46,7 +45,7 @@ private void tryToAuthorize(HttpServletRequest request) { PortalUser portalUser = authenticationService.getPortalUser(); - authenticationService.engineLogin(portalUser.getUserBean()); + authenticationService.engineLogin(portalUser); SecurityContextHolder.getContext().setAuthentication(portalUser); } } catch(Exception e) { diff --git a/src/test/java/pl/cyfronet/ltos/repository/AuthenticationMocks.java b/src/test/java/pl/cyfronet/ltos/repository/AuthenticationMocks.java index a971a15..2c40b58 100644 --- a/src/test/java/pl/cyfronet/ltos/repository/AuthenticationMocks.java +++ b/src/test/java/pl/cyfronet/ltos/repository/AuthenticationMocks.java @@ -1,19 +1,23 @@ package pl.cyfronet.ltos.repository; -import java.util.Arrays; -import java.util.List; - import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; - import pl.cyfronet.ltos.bean.Affiliation; import pl.cyfronet.ltos.bean.Role; import pl.cyfronet.ltos.bean.Team; import pl.cyfronet.ltos.bean.User; import pl.cyfronet.ltos.security.PortalUser; +import pl.cyfronet.ltos.security.PortalUserImpl.Data; import pl.cyfronet.ltos.security.UserInfo; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + public class AuthenticationMocks { private AuthenticationMocks() { @@ -30,16 +34,25 @@ public static Authentication userAuthentication(Long id) { team.setMembers(Arrays.asList(user)); affiliation.setOwner(user); role.setUsers(Arrays.asList(user)); - List auth = Arrays.asList(new SimpleGrantedAuthority("ROLE_USER")); - PortalUser pu = PortalUser.builder().principal(UserInfo.fromUser(user)).user(user).authorities(auth).isAuthenticated(true).build(); - return pu; + Collection auth = Arrays.asList(new SimpleGrantedAuthority("ROLE_USER")); + + return mockPortalUser(Data.builder().principal(UserInfo.fromUser(user)).user(user).authorities(auth).authenticated(true).build()); } public static Authentication adminAuthentication(Long id) { User user = User.builder().id(id).build(); List auth = Arrays.asList(new SimpleGrantedAuthority("ROLE_ADMIN"), new SimpleGrantedAuthority("ROLE_USER")); - PortalUser pu = PortalUser.builder().principal(UserInfo.fromUser(user)).user(user).authorities(auth).isAuthenticated(true).build(); - return pu; + return mockPortalUser(Data.builder().principal(UserInfo.fromUser(user)).user(user).authorities(auth).authenticated(true).build()); + } + + private static PortalUser mockPortalUser(Data data) { + PortalUser puMock = mock(PortalUser.class); + when(puMock.getPrincipal()).thenReturn(UserInfo.fromUser(data.getUser())); + when(puMock.getUserBean()).thenReturn(data.getUser()); + when(puMock.getDetails()).thenReturn(data.getUser()); + when(puMock.getAuthorities()).thenAnswer(invocation -> data.getAuthorities()); + when(puMock.isAuthenticated()).thenReturn(data.isAuthenticated()); + return puMock; } } diff --git a/src/test/java/pl/cyfronet/ltos/security/CmdbOwnerServiceTest.java b/src/test/java/pl/cyfronet/ltos/security/CmdbOwnerServiceTest.java new file mode 100644 index 0000000..4ff92be --- /dev/null +++ b/src/test/java/pl/cyfronet/ltos/security/CmdbOwnerServiceTest.java @@ -0,0 +1,105 @@ +package pl.cyfronet.ltos.security; + +import org.hamcrest.Matchers; +import org.json.JSONObject; +import org.junit.Test; +import pl.cyfronet.ltos.repository.CmdbRepository; + +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.mockito.internal.verification.VerificationModeFactory.times; + +public class CmdbOwnerServiceTest { + + @Test + public void shouldReturnProviderSetForEmail() { + CmdbRepository cmdbRepository = mockCmdbRepository(); + CmdbOwnerService service = new CmdbOwnerService( + cmdbRepository, + 1000); // something large, so that the cache won't expire + + Set providers1 = service.getOwnedProviders("1@owner"); + assertThat(providers1, Matchers.containsInAnyOrder("provider1", "provider2", "provider3")); + + Set providers2 = service.getOwnedProviders("2@owner"); + assertThat(providers2, Matchers.containsInAnyOrder("provider1", "provider3")); + + Set providers3 = service.getOwnedProviders("3@owner"); + assertThat(providers3, Matchers.containsInAnyOrder("provider2", "provider3")); + } + + @Test + public void shouldCacheResponses() { + CmdbRepository cmdbRepository = mockCmdbRepository(); + CmdbOwnerService service = new CmdbOwnerService( + cmdbRepository, + 1000); // something large, so that the cache won't expire + + service.getOwnedProviders("1@owner"); + service.getOwnedProviders("3@owner"); + service.getOwnedProviders("2@owner"); + service.getOwnedProviders("1@owner"); + + verify(cmdbRepository, times(1)).get("provider"); + verify(cmdbRepository, times(1)).getById("provider", "provider1"); + verify(cmdbRepository, times(1)).getById("provider", "provider2"); + verify(cmdbRepository, times(1)).getById("provider", "provider3"); + } + + @Test + public void shouldReloadWhenNeeded() { + CmdbRepository cmdbRepository = mockCmdbRepository(); + CmdbOwnerService service = new CmdbOwnerService( + cmdbRepository, + 1000); // something large, so that the cache won't expire + + service.getOwnedProviders("1@owner"); + service.invalidateCache(); + + Set providers1 = service.getOwnedProviders("1@owner"); + assertThat(providers1, Matchers.containsInAnyOrder("provider1", "provider3")); + + Set providers2 = service.getOwnedProviders("2@owner"); + assertThat(providers2, Matchers.containsInAnyOrder("provider1", "provider2", "provider3")); + + verify(cmdbRepository, times(2)).get("provider"); + verify(cmdbRepository, times(2)).getById("provider", "provider1"); + verify(cmdbRepository, times(2)).getById("provider", "provider2"); + verify(cmdbRepository, times(2)).getById("provider", "provider3"); + } + + private CmdbRepository mockCmdbRepository() { + CmdbRepository cmdbRepository = mock(CmdbRepository.class); + String repoProvidersReturn = "{\"rows\":[{\"id\":\"provider1\"},{\"id\":\"provider2\"},{\"id\":\"provider3\"}]}"; + Map providerByIdReturns = new HashMap<>(); + providerByIdReturns.put("provider1", new JSONObject[]{ + new JSONObject("{\"data\":{\"owners\":[\"1@owner\",\"2@owner\"]}}") + }); + providerByIdReturns.put("provider2", new JSONObject[]{ + new JSONObject("{\"data\":{\"owners\":[\"1@owner\",\"3@owner\"]}}"), + new JSONObject("{\"data\":{\"owners\":[\"2@owner\",\"3@owner\"]}}") + }); + providerByIdReturns.put("provider3", new JSONObject[]{ + new JSONObject("{\"data\":{\"owners\":[\"1@owner\",\"2@owner\",\"3@owner\"]}}") + }); + + when(cmdbRepository.get("provider")).thenReturn(new JSONObject(repoProvidersReturn)); + when(cmdbRepository.getById("provider", "provider1")).thenReturn( + new JSONObject("{\"data\":{\"owners\":[\"1@owner\",\"2@owner\"]}}") + ); + when(cmdbRepository.getById("provider", "provider2")).thenReturn( + new JSONObject("{\"data\":{\"owners\":[\"1@owner\",\"3@owner\"]}}"), + new JSONObject("{\"data\":{\"owners\":[\"2@owner\",\"3@owner\"]}}") + ); + when(cmdbRepository.getById("provider", "provider3")).thenReturn( + new JSONObject("{\"data\":{\"owners\":[\"1@owner\",\"2@owner\",\"3@owner\"]}}") + ); + return cmdbRepository; + } +} From 0c87ce603d7180117bfd80ec51eadf1166a4bcfd Mon Sep 17 00:00:00 2001 From: Jakub Sawicki Date: Fri, 15 Sep 2017 16:09:07 +0200 Subject: [PATCH 2/2] Correct thread-safe caching in CmdbOwnerService and several other fixes Removed IdentityProvider from places where it is unused. Corrected the way of handling provider.emails property. --- docker/indigo-slam.sh | 2 +- .../action/impl/HasAnyOfMetricsImpl.java | 7 +------ .../pl/cyfronet/ltos/controller/LegacyMethods.java | 9 --------- .../ltos/security/AuthenticationService.java | 10 ++++------ .../cyfronet/ltos/security/CmdbOwnerService.java | 12 +++--------- .../OpenIDConnectAuthenticationFilter.java | 14 ++++---------- .../pl/cyfronet/ltos/security/PortalUserImpl.java | 10 ++++------ 7 files changed, 17 insertions(+), 47 deletions(-) diff --git a/docker/indigo-slam.sh b/docker/indigo-slam.sh index 70de565..b20c3d8 100755 --- a/docker/indigo-slam.sh +++ b/docker/indigo-slam.sh @@ -16,7 +16,7 @@ java -Dserver.address=0.0.0.0 \ -Dunity.server.clientId=$IAM_CLIENT_ID \ -Dunity.server.clientSecret=$IAM_CLIENT_SECRET \ -Dcmdb.url=$CMDB_URL \ - -Dprovider.email=$PROVIDER_EMAIL \ + -Dprovider.emails=$PROVIDER_EMAIL \ -Djdbc.url=$JDBC_URL \ -Djdbc.username=$MYSQL_USER \ -Djdbc.password=$MYSQL_PASSWORD \ diff --git a/src/main/java/pl/cyfronet/bazaar/engine/extension/constraint/action/impl/HasAnyOfMetricsImpl.java b/src/main/java/pl/cyfronet/bazaar/engine/extension/constraint/action/impl/HasAnyOfMetricsImpl.java index 83f9391..9349f2b 100644 --- a/src/main/java/pl/cyfronet/bazaar/engine/extension/constraint/action/impl/HasAnyOfMetricsImpl.java +++ b/src/main/java/pl/cyfronet/bazaar/engine/extension/constraint/action/impl/HasAnyOfMetricsImpl.java @@ -1,11 +1,9 @@ package pl.cyfronet.bazaar.engine.extension.constraint.action.impl; import com.agreemount.bean.document.Document; -import com.agreemount.bean.identity.provider.IdentityProvider; import com.agreemount.slaneg.action.ActionContext; import com.agreemount.slaneg.constraint.action.impl.QualifierImpl; import com.google.common.base.Preconditions; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Component; import org.springframework.util.CollectionUtils; @@ -21,9 +19,6 @@ @Scope("prototype") public class HasAnyOfMetricsImpl extends QualifierImpl { - @Autowired - private IdentityProvider identityProvider; - @Override public boolean isAvailable() { String alias = getConstraintDefinition().getDocumentAlias(); @@ -46,4 +41,4 @@ public boolean isAvailable() { return result; } -} \ No newline at end of file +} diff --git a/src/main/java/pl/cyfronet/ltos/controller/LegacyMethods.java b/src/main/java/pl/cyfronet/ltos/controller/LegacyMethods.java index 49bd0e9..602bb88 100644 --- a/src/main/java/pl/cyfronet/ltos/controller/LegacyMethods.java +++ b/src/main/java/pl/cyfronet/ltos/controller/LegacyMethods.java @@ -1,8 +1,6 @@ package pl.cyfronet.ltos.controller; -import com.agreemount.bean.identity.provider.IdentityProvider; import lombok.extern.log4j.Log4j; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.security.core.context.SecurityContextHolder; @@ -12,7 +10,6 @@ import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.servlet.view.RedirectView; import pl.cyfronet.ltos.bean.User; -import pl.cyfronet.ltos.repository.UserRepository; import pl.cyfronet.ltos.security.UserInfo; import javax.servlet.http.HttpSession; @@ -22,12 +19,6 @@ @Log4j public class LegacyMethods { - @Autowired - private UserRepository userRepository; - - @Autowired - private IdentityProvider identityProvider; - @RequestMapping(value = "user/get", method = RequestMethod.GET) @Transactional public ResponseEntity getUser() throws IOException { diff --git a/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java b/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java index 1761893..530250b 100644 --- a/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java +++ b/src/main/java/pl/cyfronet/ltos/security/AuthenticationService.java @@ -9,12 +9,13 @@ import org.springframework.security.oauth2.client.OAuth2RestOperations; import org.springframework.stereotype.Service; import pl.cyfronet.ltos.bean.User; -import pl.cyfronet.ltos.repository.CmdbRepository; import pl.cyfronet.ltos.repository.UserRepository; import pl.cyfronet.ltos.security.AuthenticationProviderDev.UserOperations; import javax.net.ssl.HttpsURLConnection; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Locale; import java.util.stream.Collectors; @Service @@ -41,10 +42,7 @@ public class AuthenticationService { private UserOperations userOperations; @Autowired - IdentityProvider identityProvider; - - @Autowired - private CmdbRepository cmdbRepository; + private IdentityProvider identityProvider; @Autowired private PortalUserFactory portalUserFactory; diff --git a/src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java b/src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java index 3e26a08..fb6285a 100644 --- a/src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java +++ b/src/main/java/pl/cyfronet/ltos/security/CmdbOwnerService.java @@ -31,20 +31,14 @@ public CmdbOwnerService( this.refreshInterval = refreshInterval; } - public Set getOwnedProviders(String email) { + public synchronized Set getOwnedProviders(String email) { if (isCacheInvalid()) { - synchronized (this) { - // If entering critical section after waiting on lock, the cache may already have - // been refreshed. So check again. - if (isCacheInvalid()) { - doRefresh(); - } - } + doRefresh(); } return ownerEmailToProviders.getOrDefault(email, Collections.EMPTY_SET); } - public void invalidateCache() { + public synchronized void invalidateCache() { lastRefresh = 0L; } diff --git a/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java b/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java index 97d2fd3..17ad9e6 100644 --- a/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java +++ b/src/main/java/pl/cyfronet/ltos/security/OpenIDConnectAuthenticationFilter.java @@ -1,11 +1,5 @@ package pl.cyfronet.ltos.security; -import java.io.IOException; - -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.security.core.Authentication; @@ -17,7 +11,10 @@ import org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter; import org.springframework.security.web.authentication.SavedRequestAwareAuthenticationSuccessHandler; -import com.agreemount.bean.identity.provider.IdentityProvider; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.io.IOException; /** * Created by km on 04.08.16. @@ -27,9 +24,6 @@ public class OpenIDConnectAuthenticationFilter extends AbstractAuthenticationPro @Value("${unity.unauthorizedAction}") private String unauthorizedAction; - @Autowired - IdentityProvider identityProvider; - @Autowired private AuthenticationService authenticationService; diff --git a/src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java b/src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java index b090e86..4049e06 100644 --- a/src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java +++ b/src/main/java/pl/cyfronet/ltos/security/PortalUserImpl.java @@ -35,19 +35,17 @@ public static class Data { private final Data data; - private Set authorities = null; - @Autowired private CmdbOwnerService cmdbOwnerService; - //development variable, users whose email matches this will have provider role assigned - @Value("${provider.email:null}") - private Set devProviderEmails; - public PortalUserImpl(Data data) { this.data = data; } + /// development variable, users whose email matches this will have provider role assigned + @Value("${provider.emails:}") + private String devProviderEmails; + @Override public String getName() { return data.name;