From ea5c3a1c009352dbef246921abbd36d41fe7acdf Mon Sep 17 00:00:00 2001 From: Andreas Svanberg Date: Wed, 16 Apr 2025 16:34:34 +0200 Subject: [PATCH 01/13] Support for configuring end user consent requirement for clients Developers can decide if consent is required and for everyone else it is *always* required. --- .../se/su/dsv/oauth2/AuthorizationServer.java | 18 +++++ .../su/dsv/oauth2/EmbeddedConfiguration.java | 2 +- .../su/dsv/oauth2/JDBCClientRepository.java | 11 +-- .../java/se/su/dsv/oauth2/admin/Client.java | 3 +- .../se/su/dsv/oauth2/admin/ClientData.java | 5 +- .../se/su/dsv/oauth2/admin/ClientManager.java | 12 ++-- .../oauth2/admin/repository/ClientRow.java | 3 +- .../web/client/ClientAdminController.java | 35 +++++++-- .../web/client/DeveloperAccessCheck.java | 9 +++ .../oauth2/web/client/NewClientRequest.java | 14 +++- .../migration/V5__resource_owner_consent.sql | 6 ++ .../resources/templates/admin/client/edit.jte | 11 +++ .../resources/templates/admin/client/new.jte | 11 +++ .../resources/templates/admin/client/show.jte | 2 + .../dsv/oauth2/PublicClientCodeFlowTest.java | 2 +- .../ResourceServerRegisteredClientTest.java | 2 +- .../web/client/ClientAdminControllerTest.java | 72 ++++++++++++++++++- 17 files changed, 193 insertions(+), 25 deletions(-) create mode 100644 src/main/java/se/su/dsv/oauth2/web/client/DeveloperAccessCheck.java create mode 100644 src/main/resources/db/migration/V5__resource_owner_consent.sql diff --git a/src/main/java/se/su/dsv/oauth2/AuthorizationServer.java b/src/main/java/se/su/dsv/oauth2/AuthorizationServer.java index c123b0f..f209c1a 100644 --- a/src/main/java/se/su/dsv/oauth2/AuthorizationServer.java +++ b/src/main/java/se/su/dsv/oauth2/AuthorizationServer.java @@ -12,6 +12,7 @@ import org.springframework.security.config.Customizer; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.http.SessionCreationPolicy; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.crypto.password.DelegatingPasswordEncoder; import org.springframework.security.crypto.password.NoOpPasswordEncoder; @@ -28,6 +29,7 @@ import se.su.dsv.oauth2.shibboleth.Entitlement; import se.su.dsv.oauth2.shibboleth.ShibbolethConfigurer; import se.su.dsv.oauth2.shibboleth.ShibbolethTokenPopulator; import se.su.dsv.oauth2.staging.StagingSecurityConfigurer; +import se.su.dsv.oauth2.web.client.DeveloperAccessCheck; import java.util.Map; import java.util.Optional; @@ -162,4 +164,20 @@ public class AuthorizationServer extends SpringBootServletInitializer { return delegatingPasswordEncoder; } + + @Bean + public DeveloperAccessCheck developerAccessCheck(Config config) { + final String developerAuthority = Entitlement.asAuthority(config.developerEntitlement()); + return authentication -> { + if (!authentication.isAuthenticated()) { + return false; + } + for (GrantedAuthority authority : authentication.getAuthorities()) { + if (developerAuthority.equals(authority.getAuthority())) { + return true; + } + } + return false; + }; + } } diff --git a/src/main/java/se/su/dsv/oauth2/EmbeddedConfiguration.java b/src/main/java/se/su/dsv/oauth2/EmbeddedConfiguration.java index 95b8dfe..0dd25c0 100644 --- a/src/main/java/se/su/dsv/oauth2/EmbeddedConfiguration.java +++ b/src/main/java/se/su/dsv/oauth2/EmbeddedConfiguration.java @@ -34,7 +34,7 @@ public class EmbeddedConfiguration { String scopeString = System.getenv("CLIENT_SCOPES"); return new ClientRow(clientId, clientId, clientId, "dev@localhost", - redirectUri, scopeString, clientSecret); + redirectUri, scopeString, clientSecret, false); } private static class InMemoryClientrepository implements ClientRepository { diff --git a/src/main/java/se/su/dsv/oauth2/JDBCClientRepository.java b/src/main/java/se/su/dsv/oauth2/JDBCClientRepository.java index 696ac37..73232bd 100644 --- a/src/main/java/se/su/dsv/oauth2/JDBCClientRepository.java +++ b/src/main/java/se/su/dsv/oauth2/JDBCClientRepository.java @@ -29,7 +29,7 @@ public class JDBCClientRepository implements ClientRepository { @Override public List getClients(final Principal owner) { - return getJdbc().sql("SELECT id, client_id, name, contact_email, redirect_uri, scopes, client_secret FROM v2_client WHERE id IN (SELECT client_id FROM v2_client_owner WHERE owner = :owner)") + return getJdbc().sql("SELECT id, client_id, name, contact_email, redirect_uri, scopes, client_secret, requires_consent FROM v2_client WHERE id IN (SELECT client_id FROM v2_client_owner WHERE owner = :owner)") .param("owner", owner.getName()) .query(ClientRow.class) .list(); @@ -45,7 +45,7 @@ public class JDBCClientRepository implements ClientRepository { @Override public Optional getClientRowById(final String id) { - return getJdbc().sql("SELECT id, client_id, name, contact_email, redirect_uri, scopes, client_secret FROM v2_client WHERE id = :id") + return getJdbc().sql("SELECT id, client_id, name, contact_email, redirect_uri, scopes, client_secret, requires_consent FROM v2_client WHERE id = :id") .param("id", id) .query(ClientRow.class) .optional(); @@ -53,7 +53,7 @@ public class JDBCClientRepository implements ClientRepository { @Override public Optional getClientRowByClientId(final String clientId) { - return getJdbc().sql("SELECT id, client_id, name, contact_email, redirect_uri, scopes, client_secret FROM v2_client WHERE client_id = :clientId") + return getJdbc().sql("SELECT id, client_id, name, contact_email, redirect_uri, scopes, client_secret, requires_consent FROM v2_client WHERE client_id = :clientId") .param("clientId", clientId) .query(ClientRow.class) .optional(); @@ -62,14 +62,15 @@ public class JDBCClientRepository implements ClientRepository { @Override public void addNewClient(final ClientRow clientRow) { getJdbc().sql(""" - INSERT INTO v2_client (id, client_id, client_secret, name, redirect_uri, contact_email, scopes) - VALUES (:id, :clientId, :clientSecret, :name, :redirectUri, :contactEmail, :scopes) + INSERT INTO v2_client (id, client_id, client_secret, name, redirect_uri, contact_email, scopes, requires_consent) + VALUES (:id, :clientId, :clientSecret, :name, :redirectUri, :contactEmail, :scopes, :requiresConsent) ON DUPLICATE KEY UPDATE client_id = VALUES(client_id), client_secret = VALUES(client_secret), name = VALUES(name), redirect_uri = VALUES(redirect_uri), contact_email = VALUES(contact_email), + requires_consent = VALUES(requires_consent), scopes = VALUES(scopes); """) .paramSource(clientRow) diff --git a/src/main/java/se/su/dsv/oauth2/admin/Client.java b/src/main/java/se/su/dsv/oauth2/admin/Client.java index 7368e02..1bc45ea 100644 --- a/src/main/java/se/su/dsv/oauth2/admin/Client.java +++ b/src/main/java/se/su/dsv/oauth2/admin/Client.java @@ -11,6 +11,7 @@ public record Client( String redirectUri, boolean isPublic, Set scopes, - List owners) + List owners, + boolean requiresConsent) { } diff --git a/src/main/java/se/su/dsv/oauth2/admin/ClientData.java b/src/main/java/se/su/dsv/oauth2/admin/ClientData.java index 6e6213d..7613335 100644 --- a/src/main/java/se/su/dsv/oauth2/admin/ClientData.java +++ b/src/main/java/se/su/dsv/oauth2/admin/ClientData.java @@ -8,10 +8,11 @@ public record ClientData( URI redirectURI, boolean isPublic, Set scopes, - String contactEmail) + String contactEmail, + boolean requiresConsent) { // When creating a resource server client public ClientData(String clientName, String contactEmail) { - this(clientName, null, false, Set.of(), contactEmail); + this(clientName, null, false, Set.of(), contactEmail, true); } } diff --git a/src/main/java/se/su/dsv/oauth2/admin/ClientManager.java b/src/main/java/se/su/dsv/oauth2/admin/ClientManager.java index afcae5f..484ce0a 100644 --- a/src/main/java/se/su/dsv/oauth2/admin/ClientManager.java +++ b/src/main/java/se/su/dsv/oauth2/admin/ClientManager.java @@ -44,7 +44,7 @@ public class ClientManager implements RegisteredClientRepository, ClientManageme String scopeString = toScopeString(clientData); ClientRow clientRow = new ClientRow(id, clientId, clientData.clientName(), clientData.contactEmail(), - redirectURI, scopeString, encodedClientSecret); + redirectURI, scopeString, encodedClientSecret, clientData.requiresConsent()); clientRepository.addNewClient(clientRow); @@ -66,7 +66,7 @@ public class ClientManager implements RegisteredClientRepository, ClientManageme ClientRow updated = new ClientRow(id, currentClient.clientId(), clientData.clientName(), clientData.contactEmail(), getRedirectUri(clientData), - toScopeString(clientData), currentClient.clientSecret()); + toScopeString(clientData), currentClient.clientSecret(), clientData.requiresConsent()); clientRepository.update(updated); return toClient(updated); } @@ -133,7 +133,8 @@ public class ClientManager implements RegisteredClientRepository, ClientManageme clientRow.redirectUri(), isPublic, scopes, - owners); + owners, + clientRow.requiresConsent()); } // Used by various components of the OAuth 2.0 infrastructure to upgrade @@ -193,7 +194,10 @@ public class ClientManager implements RegisteredClientRepository, ClientManageme redirectUris.add(clientRow.redirectUri()); } }) - .clientSettings(ClientSettings.builder().requireProofKey(clientRow.isPublic()).build()) + .clientSettings(ClientSettings.builder() + .requireAuthorizationConsent(clientRow.requiresConsent()) + .requireProofKey(clientRow.isPublic()) + .build()) .scopes(currentScopes -> currentScopes.addAll(clientRow.scopeSet())) .build(); } diff --git a/src/main/java/se/su/dsv/oauth2/admin/repository/ClientRow.java b/src/main/java/se/su/dsv/oauth2/admin/repository/ClientRow.java index 2764de8..69de20c 100644 --- a/src/main/java/se/su/dsv/oauth2/admin/repository/ClientRow.java +++ b/src/main/java/se/su/dsv/oauth2/admin/repository/ClientRow.java @@ -12,7 +12,8 @@ public record ClientRow( String contactEmail, String redirectUri, String scopes, - String clientSecret) + String clientSecret, + boolean requiresConsent) { public Set scopeSet() { if (scopes == null) { diff --git a/src/main/java/se/su/dsv/oauth2/web/client/ClientAdminController.java b/src/main/java/se/su/dsv/oauth2/web/client/ClientAdminController.java index c9b3848..71f1e5c 100644 --- a/src/main/java/se/su/dsv/oauth2/web/client/ClientAdminController.java +++ b/src/main/java/se/su/dsv/oauth2/web/client/ClientAdminController.java @@ -1,6 +1,7 @@ package se.su.dsv.oauth2.web.client; import org.springframework.http.HttpStatus; +import org.springframework.security.core.Authentication; import org.springframework.security.oauth2.server.authorization.settings.AuthorizationServerSettings; import org.springframework.security.web.csrf.CsrfToken; import org.springframework.stereotype.Controller; @@ -28,13 +29,16 @@ public class ClientAdminController { private final ClientManagementService clientManagementService; private final AuthorizationServerSettings authorizationServerSettings; + private final DeveloperAccessCheck developerAccessCheck; public ClientAdminController( final ClientManagementService clientManagementService, - AuthorizationServerSettings authorizationServerSettings) + AuthorizationServerSettings authorizationServerSettings, + DeveloperAccessCheck developerAccessCheck) { this.clientManagementService = clientManagementService; this.authorizationServerSettings = authorizationServerSettings; + this.developerAccessCheck = developerAccessCheck; } @GetMapping @@ -46,13 +50,13 @@ public class ClientAdminController { @GetMapping("/new") public String newClient(Model model) { - model.addAttribute("newClient", new NewClientRequest(null, null, null, null, null)); + model.addAttribute("newClient", NewClientRequest.empty()); return "admin/client/new"; } @PostMapping("/new") public String createClient( - Principal principal, + Authentication principal, Model model, RedirectAttributes redirectAttributes, @ModelAttribute("newClient") NewClientRequest newClientRequest, @@ -62,12 +66,14 @@ public class ClientAdminController { model.addAttribute("errors", bindingResult.getAllErrors()); return "admin/client/new"; } + boolean requiresConsent = determineConsentRequired(principal, newClientRequest); ClientData clientData = new ClientData( newClientRequest.name(), newClientRequest.redirectUri(), newClientRequest.isPublic(), newClientRequest.scopes(), - newClientRequest.contact()); + newClientRequest.contact(), + requiresConsent); NewClient newClient = clientManagementService.createClient(principal, clientData); redirectAttributes.addFlashAttribute("message", "New client created"); redirectAttributes.addFlashAttribute("clientId", newClient.clientId()); @@ -132,7 +138,7 @@ public class ClientAdminController { @PostMapping("/{id}/edit") public String editClient( @PathVariable("id") String id, - Principal principal, + Authentication principal, Model model, RedirectAttributes redirectAttributes, @ModelAttribute("newClient") NewClientRequest newClientRequest, @@ -142,17 +148,29 @@ public class ClientAdminController { model.addAttribute("errors", bindingResult.getAllErrors()); return "admin/client/edit"; } + boolean requiresConsent = determineConsentRequired(principal, newClientRequest); ClientData clientData = new ClientData( newClientRequest.name(), newClientRequest.redirectUri(), newClientRequest.isPublic(), newClientRequest.scopes(), - newClientRequest.contact()); + newClientRequest.contact(), + requiresConsent); final Client updatedClient = clientManagementService.updateClient(principal, id, clientData); redirectAttributes.addFlashAttribute("message", "Client updated"); return "redirect:/admin/client/" + updatedClient.id(); } + private boolean determineConsentRequired(final Authentication principal, final NewClientRequest newClientRequest) { + if (hasDeveloperAccess(principal)) { + // Allow developers to decide if consent is required + return newClientRequest.requiresConsentBoolean(); + } else { + // For everyone else it's always required + return true; + } + } + @ModelAttribute public CsrfToken csrfToken(CsrfToken token) { return token; @@ -162,4 +180,9 @@ public class ClientAdminController { public AuthorizationServerSettings authorizationServerSettings() { return authorizationServerSettings; } + + @ModelAttribute(name = "developerAccess") + public boolean hasDeveloperAccess(Authentication principal) { + return developerAccessCheck.test(principal); + } } diff --git a/src/main/java/se/su/dsv/oauth2/web/client/DeveloperAccessCheck.java b/src/main/java/se/su/dsv/oauth2/web/client/DeveloperAccessCheck.java new file mode 100644 index 0000000..6677a39 --- /dev/null +++ b/src/main/java/se/su/dsv/oauth2/web/client/DeveloperAccessCheck.java @@ -0,0 +1,9 @@ +package se.su.dsv.oauth2.web.client; + +import org.springframework.security.core.Authentication; + +import java.util.function.Predicate; + +@FunctionalInterface +public interface DeveloperAccessCheck extends Predicate { +} diff --git a/src/main/java/se/su/dsv/oauth2/web/client/NewClientRequest.java b/src/main/java/se/su/dsv/oauth2/web/client/NewClientRequest.java index 935dd31..058f1c5 100644 --- a/src/main/java/se/su/dsv/oauth2/web/client/NewClientRequest.java +++ b/src/main/java/se/su/dsv/oauth2/web/client/NewClientRequest.java @@ -14,7 +14,8 @@ public record NewClientRequest( String contact, URI redirectUri, String public_, - String scope) + String scope, + String requiresConsent) { public static NewClientRequest from(final Client client) { return new NewClientRequest( @@ -22,7 +23,12 @@ public record NewClientRequest( client.contact(), URI.create(client.redirectUri()), client.isPublic() ? "on" : null, - String.join("\r\n", client.scopes())); + String.join("\r\n", client.scopes()), + client.requiresConsent() ? "on" : null); + } + + public static NewClientRequest empty() { + return new NewClientRequest(null, null, null, null, null, null); } public boolean isPublic() { @@ -40,4 +46,8 @@ public record NewClientRequest( public String redirectUriString() { return redirectUri == null ? "" : redirectUri.toString(); } + + public boolean requiresConsentBoolean() { + return "on".equals(requiresConsent); + } } diff --git a/src/main/resources/db/migration/V5__resource_owner_consent.sql b/src/main/resources/db/migration/V5__resource_owner_consent.sql new file mode 100644 index 0000000..b0d2f67 --- /dev/null +++ b/src/main/resources/db/migration/V5__resource_owner_consent.sql @@ -0,0 +1,6 @@ +ALTER TABLE `v2_client` + # Defaulting user consent to true so that future clients will require user consent + ADD COLUMN `requires_consent` BOOLEAN NOT NULL DEFAULT TRUE; + +# Making sure current clients stay the same +UPDATE `v2_client` SET `requires_consent` = FALSE; diff --git a/src/main/resources/templates/admin/client/edit.jte b/src/main/resources/templates/admin/client/edit.jte index aa09249..2366410 100644 --- a/src/main/resources/templates/admin/client/edit.jte +++ b/src/main/resources/templates/admin/client/edit.jte @@ -7,6 +7,7 @@ @param Client client @param String feedback @param List errors +@param boolean developerAccess @template.base(title = "Edit client " + client.name(), content = @`