From 39bc00a3b08e3129e2244f123a466f4c9490ae36 Mon Sep 17 00:00:00 2001 From: Dominik Frantisek Bucik Date: Fri, 19 Nov 2021 16:14:21 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20=F0=9F=90=9B=20Fix=20ACR=20for=20implici?= =?UTF-8?q?t=20and=20authorization=5Fcode=20flows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING CHANGE: 🧨 Database needs to be updated: `ALTER TABLE saved_user_auth DROP source_class; ALTER TABLE saved_user_auth ADD COLUMN acr VARCHAR(1024);` --- .../db/hsql/hsql_database_tables.sql | 1 + .../db/mysql/mysql_database_tables.sql | 1 + .../db/psql/psql_database_tables.sql | 1 + .../oauth2/model/SavedUserAuthentication.java | 87 +++++++++++-------- .../ics/oauth2/token/DeviceTokenGranter.java | 2 + .../muni/ics/oauth2/web/DeviceEndpoint.java | 5 +- .../saml/PerunSamlAuthenticationProvider.java | 3 +- .../cz/muni/ics/oidc/saml/SamlPrincipal.java | 27 ++++++ .../oidc/server/PerunOIDCTokenService.java | 27 +++--- .../service/impl/DefaultOIDCTokenService.java | 1 - .../connect/token/ConnectTokenEnhancer.java | 1 - 11 files changed, 99 insertions(+), 57 deletions(-) create mode 100644 perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/SamlPrincipal.java diff --git a/perun-oidc-server-webapp/src/main/resources/db/hsql/hsql_database_tables.sql b/perun-oidc-server-webapp/src/main/resources/db/hsql/hsql_database_tables.sql index 6d360ca9f..e0327b4d0 100644 --- a/perun-oidc-server-webapp/src/main/resources/db/hsql/hsql_database_tables.sql +++ b/perun-oidc-server-webapp/src/main/resources/db/hsql/hsql_database_tables.sql @@ -86,6 +86,7 @@ CREATE TABLE IF NOT EXISTS authentication_holder_request_parameter ( CREATE TABLE IF NOT EXISTS saved_user_auth ( id BIGINT GENERATED BY DEFAULT AS IDENTITY(START WITH 1) PRIMARY KEY, + acr VARCHAR(1024), name VARCHAR(1024), authenticated BOOLEAN, source_class VARCHAR(2048) diff --git a/perun-oidc-server-webapp/src/main/resources/db/mysql/mysql_database_tables.sql b/perun-oidc-server-webapp/src/main/resources/db/mysql/mysql_database_tables.sql index d35d9c2f7..c52bd79c4 100644 --- a/perun-oidc-server-webapp/src/main/resources/db/mysql/mysql_database_tables.sql +++ b/perun-oidc-server-webapp/src/main/resources/db/mysql/mysql_database_tables.sql @@ -85,6 +85,7 @@ CREATE TABLE IF NOT EXISTS authentication_holder_request_parameter ( CREATE TABLE IF NOT EXISTS saved_user_auth ( id BIGINT AUTO_INCREMENT PRIMARY KEY, + acr VARCHAR(1024), name VARCHAR(1024), authenticated BOOLEAN, source_class VARCHAR(2048) diff --git a/perun-oidc-server-webapp/src/main/resources/db/psql/psql_database_tables.sql b/perun-oidc-server-webapp/src/main/resources/db/psql/psql_database_tables.sql index 846897818..d7e402d8b 100644 --- a/perun-oidc-server-webapp/src/main/resources/db/psql/psql_database_tables.sql +++ b/perun-oidc-server-webapp/src/main/resources/db/psql/psql_database_tables.sql @@ -86,6 +86,7 @@ CREATE TABLE IF NOT EXISTS authentication_holder_request_parameter ( CREATE TABLE IF NOT EXISTS saved_user_auth ( id BIGSERIAL PRIMARY KEY, + acr VARCHAR(1024), name VARCHAR(1024), authenticated BOOLEAN, source_class VARCHAR(2048) diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/model/SavedUserAuthentication.java b/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/model/SavedUserAuthentication.java index 88e43bde6..c0507e6db 100644 --- a/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/model/SavedUserAuthentication.java +++ b/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/model/SavedUserAuthentication.java @@ -17,8 +17,10 @@ package cz.muni.ics.oauth2.model; import cz.muni.ics.oauth2.model.convert.SimpleGrantedAuthorityStringConverter; +import cz.muni.ics.oidc.saml.SamlPrincipal; import java.util.Collection; import java.util.HashSet; +import java.util.stream.Collectors; import javax.persistence.Basic; import javax.persistence.CollectionTable; import javax.persistence.Column; @@ -32,8 +34,14 @@ import javax.persistence.Id; import javax.persistence.JoinColumn; import javax.persistence.Table; import javax.persistence.Transient; +import lombok.ToString; +import lombok.extern.slf4j.Slf4j; +import org.opensaml.saml2.core.AuthnContext; +import org.opensaml.saml2.core.AuthnContextClassRef; +import org.opensaml.saml2.core.AuthnStatement; import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.providers.ExpiringUsernameAuthenticationToken; /** * This class stands in for an original Authentication object. @@ -42,6 +50,8 @@ import org.springframework.security.core.GrantedAuthority; */ @Entity @Table(name="saved_user_auth") +@Slf4j +@ToString public class SavedUserAuthentication implements Authentication { private static final long serialVersionUID = -1804249963940323488L; @@ -50,18 +60,21 @@ public class SavedUserAuthentication implements Authentication { private String name; private Collection authorities; private boolean authenticated; - private String sourceClass; + private String acr; public SavedUserAuthentication(Authentication src) { setName(src.getName()); setAuthorities(new HashSet<>(src.getAuthorities())); setAuthenticated(src.isAuthenticated()); - - if (src instanceof SavedUserAuthentication) { - // if we're copying in a saved auth, carry over the original class name - setSourceClass(((SavedUserAuthentication) src).getSourceClass()); - } else { - setSourceClass(src.getClass().getName()); + if (src instanceof ExpiringUsernameAuthenticationToken) { + ExpiringUsernameAuthenticationToken token = (ExpiringUsernameAuthenticationToken) src; + this.acr = ((SamlPrincipal) token.getPrincipal()).getSamlCredential() + .getAuthenticationAssertion() + .getAuthnStatements().stream() + .map(AuthnStatement::getAuthnContext) + .map(AuthnContext::getAuthnContextClassRef) + .map(AuthnContextClassRef::getAuthnContextClassRef) + .collect(Collectors.joining()); } } @@ -85,6 +98,10 @@ public class SavedUserAuthentication implements Authentication { return name; } + public void setName(String name) { + this.name = name; + } + @Override @ElementCollection(fetch = FetchType.EAGER) @CollectionTable(name="saved_user_auth_authority", joinColumns=@JoinColumn(name="owner_id")) @@ -94,6 +111,32 @@ public class SavedUserAuthentication implements Authentication { return authorities; } + public void setAuthorities(Collection authorities) { + this.authorities = authorities; + } + + @Basic + @Column(name = "acr") + public String getAcr() { + return acr; + } + + public void setAcr(String acr) { + this.acr = acr; + } + + @Override + @Basic + @Column(name="authenticated") + public boolean isAuthenticated() { + return authenticated; + } + + @Override + public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException { + this.authenticated = isAuthenticated; + } + @Override @Transient public Object getCredentials() { @@ -112,34 +155,4 @@ public class SavedUserAuthentication implements Authentication { return getName(); } - @Override - @Basic - @Column(name="authenticated") - public boolean isAuthenticated() { - return authenticated; - } - - @Override - public void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException { - this.authenticated = isAuthenticated; - } - - @Basic - @Column(name="source_class") - public String getSourceClass() { - return sourceClass; - } - - public void setSourceClass(String sourceClass) { - this.sourceClass = sourceClass; - } - - public void setName(String name) { - this.name = name; - } - - public void setAuthorities(Collection authorities) { - this.authorities = authorities; - } - } diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/token/DeviceTokenGranter.java b/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/token/DeviceTokenGranter.java index 5399512c1..c2175afe7 100644 --- a/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/token/DeviceTokenGranter.java +++ b/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/token/DeviceTokenGranter.java @@ -22,6 +22,7 @@ import cz.muni.ics.oauth2.model.DeviceCode; import cz.muni.ics.oauth2.service.DeviceCodeService; import cz.muni.ics.oauth2.web.DeviceEndpoint; import java.util.Date; +import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.oauth2.common.exceptions.InvalidGrantException; import org.springframework.security.oauth2.provider.ClientDetails; @@ -42,6 +43,7 @@ import org.springframework.stereotype.Component; * */ @Component("deviceTokenGranter") +@Slf4j public class DeviceTokenGranter extends AbstractTokenGranter { public static final String GRANT_TYPE = "urn:ietf:params:oauth:grant-type:device_code"; diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/web/DeviceEndpoint.java b/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/web/DeviceEndpoint.java index 3416cb757..1c9e49c5f 100644 --- a/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/web/DeviceEndpoint.java +++ b/perun-oidc-server/src/main/java/cz/muni/ics/oauth2/web/DeviceEndpoint.java @@ -243,8 +243,9 @@ public class DeviceEndpoint { @PreAuthorize("hasRole('ROLE_USER')") @RequestMapping(value = "/" + USER_URL + "/approve", method = RequestMethod.POST) - public String approveDevice(@RequestParam("user_code") String userCode, @RequestParam(value = "user_oauth_approval") Boolean approve, ModelMap model, Authentication auth, HttpSession session) { - + public String approveDevice(@RequestParam("user_code") String userCode, + @RequestParam(value = "user_oauth_approval") Boolean approve, + ModelMap model, Authentication auth, HttpSession session) { AuthorizationRequest authorizationRequest = (AuthorizationRequest) session.getAttribute("authorizationRequest"); DeviceCode dc = (DeviceCode) session.getAttribute("deviceCode"); diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/PerunSamlAuthenticationProvider.java b/perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/PerunSamlAuthenticationProvider.java index edc366c91..0e828f847 100644 --- a/perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/PerunSamlAuthenticationProvider.java +++ b/perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/PerunSamlAuthenticationProvider.java @@ -30,8 +30,7 @@ public class PerunSamlAuthenticationProvider extends SAMLAuthenticationProvider @Override protected Object getPrincipal(SAMLCredential credential, Object userDetail) { PerunUser user = (PerunUser) userDetail; - return new User(String.valueOf(user.getId()), credential.getRemoteEntityID(), - getEntitlements(credential, userDetail)); + return new SamlPrincipal(user.getId(), credential, getEntitlements(credential, userDetail)); } @Override diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/SamlPrincipal.java b/perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/SamlPrincipal.java new file mode 100644 index 000000000..e9406624f --- /dev/null +++ b/perun-oidc-server/src/main/java/cz/muni/ics/oidc/saml/SamlPrincipal.java @@ -0,0 +1,27 @@ +package cz.muni.ics.oidc.saml; + +import java.util.Collection; +import lombok.Getter; +import lombok.Setter; +import lombok.ToString; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.userdetails.User; +import org.springframework.security.saml.SAMLCredential; + +@Getter +@Setter +@ToString +public class SamlPrincipal extends User { + + private Long perunUserId; + private SAMLCredential samlCredential; + + public SamlPrincipal(Long perunUserId, + SAMLCredential samlCredential, + Collection authorities) { + super(String.valueOf(perunUserId), "[PROTECTED]", authorities); + this.perunUserId = perunUserId; + this.samlCredential = samlCredential; + } + +} diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/oidc/server/PerunOIDCTokenService.java b/perun-oidc-server/src/main/java/cz/muni/ics/oidc/server/PerunOIDCTokenService.java index eebd20784..7f148306d 100644 --- a/perun-oidc-server/src/main/java/cz/muni/ics/oidc/server/PerunOIDCTokenService.java +++ b/perun-oidc-server/src/main/java/cz/muni/ics/oidc/server/PerunOIDCTokenService.java @@ -18,9 +18,6 @@ import lombok.extern.slf4j.Slf4j; import net.minidev.json.JSONArray; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.oauth2.provider.OAuth2Request; -import org.springframework.web.context.request.RequestAttributes; -import org.springframework.web.context.request.RequestContextHolder; -import org.springframework.web.context.request.ServletRequestAttributes; /** * Modifies ID Token. @@ -49,8 +46,11 @@ public class PerunOIDCTokenService extends DefaultOIDCTokenService { } @Override - protected void addCustomIdTokenClaims(JWTClaimsSet.Builder idClaims, ClientDetailsEntity client, OAuth2Request request, - String sub, OAuth2AccessTokenEntity accessToken) + protected void addCustomIdTokenClaims(JWTClaimsSet.Builder idClaims, + ClientDetailsEntity client, + OAuth2Request request, + String sub, + OAuth2AccessTokenEntity accessToken) { log.debug("modifying ID token"); String userId = accessToken.getAuthenticationHolder().getAuthentication().getName(); @@ -73,18 +73,17 @@ public class PerunOIDCTokenService extends DefaultOIDCTokenService { } } - String acr = getAuthnContextClass(); - if (acr != null) { - log.debug("adding to ID token claim acr with value {}", acr); - idClaims.claim("acr", acr); + if (accessToken.getAuthenticationHolder() != null + && accessToken.getAuthenticationHolder().getUserAuth() != null) + { + String acr = accessToken.getAuthenticationHolder().getUserAuth().getAcr(); + if (acr != null) { + log.debug("adding to ID token claim acr with value {}", acr); + idClaims.claim("acr", acr); + } } } - private String getAuthnContextClass() { - ServletRequestAttributes attr = (ServletRequestAttributes) RequestContextHolder.currentRequestAttributes(); - return (String) attr.getAttribute(SESSION_PARAM_ACR, RequestAttributes.SCOPE_SESSION); - } - /** * Converts claim values from com.google.gson.JsonElement to net.minidev.json.JSONObject or primitive value * diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/service/impl/DefaultOIDCTokenService.java b/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/service/impl/DefaultOIDCTokenService.java index eca32e18f..5b9c0df12 100644 --- a/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/service/impl/DefaultOIDCTokenService.java +++ b/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/service/impl/DefaultOIDCTokenService.java @@ -66,7 +66,6 @@ import org.springframework.stereotype.Service; * @author Amanda Anganes * */ -@Service @Slf4j public class DefaultOIDCTokenService implements OIDCTokenService { diff --git a/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/token/ConnectTokenEnhancer.java b/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/token/ConnectTokenEnhancer.java index 530d98d69..eb26c7ef9 100644 --- a/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/token/ConnectTokenEnhancer.java +++ b/perun-oidc-server/src/main/java/cz/muni/ics/openid/connect/token/ConnectTokenEnhancer.java @@ -44,7 +44,6 @@ import org.springframework.security.oauth2.provider.OAuth2Request; import org.springframework.security.oauth2.provider.token.TokenEnhancer; import org.springframework.stereotype.Service; -@Service @Slf4j public class ConnectTokenEnhancer implements TokenEnhancer {