From 29d6ca4f35ff3e669e7476df7948e992776e35a6 Mon Sep 17 00:00:00 2001 From: Dario Ghunney Ware Date: Wed, 30 Jul 2025 12:45:01 +0100 Subject: [PATCH] Fixing logout --- .github/workflows/build.yml | 4 +- .../configuration/InstallationPathConfig.java | 2 +- .../common/model/ApplicationProperties.java | 4 +- .../src/main/resources/settings.yml.template | 29 ++++++------ .../main/resources/static/js/fetch-utils.js | 12 +++-- .../src/main/resources/static/js/jwt-init.js | 5 ++- .../security/InitialSecuritySetup.java | 1 - .../security/config/AccountWebController.java | 7 ++- .../configuration/SecurityConfiguration.java | 2 +- .../filter/JwtAuthenticationFilter.java | 8 +--- .../filter/UserAuthenticationFilter.java | 9 +--- .../security/model/AuthenticationType.java | 3 +- ...stomSaml2AuthenticationSuccessHandler.java | 4 +- .../security/service/JwtService.java | 28 ++++++------ ...ervice.java => KeyPairCleanupService.java} | 15 ++++--- ...storeService.java => KeystoreService.java} | 10 ++--- ...ace.java => KeystoreServiceInterface.java} | 2 +- .../security/service/UserService.java | 14 ------ .../security/service/JwtServiceTest.java | 45 ++++++++++++------- ...st.java => KeyPairCleanupServiceTest.java} | 14 +++--- ...java => KeystoreServiceInterfaceTest.java} | 22 ++++----- exampleYmlFiles/test_cicd.yml | 1 + 22 files changed, 122 insertions(+), 119 deletions(-) rename app/proprietary/src/main/java/stirling/software/proprietary/security/service/{JwtKeyCleanupService.java => KeyPairCleanupService.java} (91%) rename app/proprietary/src/main/java/stirling/software/proprietary/security/service/{JwtKeystoreService.java => KeystoreService.java} (96%) rename app/proprietary/src/main/java/stirling/software/proprietary/security/service/{JwtKeystoreServiceInterface.java => KeystoreServiceInterface.java} (86%) rename app/proprietary/src/test/java/stirling/software/proprietary/security/service/{JwtKeyCleanupServiceTest.java => KeyPairCleanupServiceTest.java} (97%) rename app/proprietary/src/test/java/stirling/software/proprietary/security/service/{JwtKeystoreServiceInterfaceTest.java => KeystoreServiceInterfaceTest.java} (90%) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index d87e478d3..c229ee40e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -147,7 +147,9 @@ jobs: - name: Generate OpenAPI documentation run: ./gradlew :stirling-pdf:generateOpenApiDocs - + env: + DISABLE_ADDITIONAL_FEATURES: true + - name: Upload OpenAPI Documentation uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: diff --git a/app/common/src/main/java/stirling/software/common/configuration/InstallationPathConfig.java b/app/common/src/main/java/stirling/software/common/configuration/InstallationPathConfig.java index a31387c56..95de76dbd 100644 --- a/app/common/src/main/java/stirling/software/common/configuration/InstallationPathConfig.java +++ b/app/common/src/main/java/stirling/software/common/configuration/InstallationPathConfig.java @@ -46,7 +46,7 @@ public class InstallationPathConfig { STATIC_PATH = CUSTOM_FILES_PATH + "static" + File.separator; TEMPLATES_PATH = CUSTOM_FILES_PATH + "templates" + File.separator; SIGNATURES_PATH = CUSTOM_FILES_PATH + "signatures" + File.separator; - PRIVATE_KEY_PATH = CUSTOM_FILES_PATH + "keys" + File.separator; + PRIVATE_KEY_PATH = CONFIG_PATH + "keys" + File.separator; } private static String initializeBasePath() { diff --git a/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java b/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java index 3b3e31bd6..3680184f3 100644 --- a/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java +++ b/app/common/src/main/java/stirling/software/common/model/ApplicationProperties.java @@ -304,8 +304,8 @@ public class ApplicationProperties { private boolean enableKeystore = true; private boolean enableKeyRotation = false; private boolean enableKeyCleanup = true; - private int keyRetentionDays; - private int cleanupBatchSize; + private int keyRetentionDays = 7; + private int cleanupBatchSize = 100; } } diff --git a/app/core/src/main/resources/settings.yml.template b/app/core/src/main/resources/settings.yml.template index c58135b4f..1eb9efdd2 100644 --- a/app/core/src/main/resources/settings.yml.template +++ b/app/core/src/main/resources/settings.yml.template @@ -31,7 +31,7 @@ security: google: clientId: '' # client ID for Google OAuth2 clientSecret: '' # client secret for Google OAuth2 - scopes: https://www.googleapis.com/auth/userinfo.email, https://www.googleapis.com/auth/userinfo.profile # scopes for Google OAuth2 + scopes: email, profile # scopes for Google OAuth2 useAsUsername: email # field to use as the username for Google OAuth2. Available options are: [email | name | given_name | family_name] github: clientId: '' # client ID for GitHub OAuth2 @@ -51,20 +51,21 @@ security: provider: '' # The name of your Provider autoCreateUser: true # set to 'true' to allow auto-creation of non-existing users blockRegistration: false # set to 'true' to deny login with SSO without prior registration by an admin - registrationId: stirlingpdf-dario-saml # The name of your Service Provider (SP) app name. Should match the name in the path for your SSO & SLO URLs - idpMetadataUri: https://authentik.dev.stirlingpdf.com/api/v3/providers/saml/5/metadata/ # The uri for your Provider's metadata - idpSingleLoginUrl: https://authentik.dev.stirlingpdf.com/application/saml/stirlingpdf-dario-saml/sso/binding/post/ # The URL for initiating SSO. Provided by your Provider - idpSingleLogoutUrl: https://authentik.dev.stirlingpdf.com/application/saml/stirlingpdf-dario-saml/slo/binding/post/ # The URL for initiating SLO. Provided by your Provider - idpIssuer: authentik # The ID of your Provider - idpCert: classpath:authentik-Self-signed_Certificate_certificate.pem # The certificate your Provider will use to authenticate your app's SAML authentication requests. Provided by your Provider - privateKey: classpath:private_key.key # Your private key. Generated from your keypair - spCert: classpath:certificate.crt # Your signing certificate. Generated from your keypair + registrationId: stirling # The name of your Service Provider (SP) app name. Should match the name in the path for your SSO & SLO URLs + idpMetadataUri: https://dev-XXXXXXXX.okta.com/app/externalKey/sso/saml/metadata # The uri for your Provider's metadata + idpSingleLoginUrl: https://dev-XXXXXXXX.okta.com/app/dev-XXXXXXXX_stirlingpdf_1/externalKey/sso/saml # The URL for initiating SSO. Provided by your Provider + idpSingleLogoutUrl: https://dev-XXXXXXXX.okta.com/app/dev-XXXXXXXX_stirlingpdf_1/externalKey/slo/saml # The URL for initiating SLO. Provided by your Provider + idpIssuer: '' # The ID of your Provider + idpCert: classpath:okta.cert # The certificate your Provider will use to authenticate your app's SAML authentication requests. Provided by your Provider + privateKey: classpath:saml-private-key.key # Your private key. Generated from your keypair + spCert: classpath:saml-public-cert.crt # Your signing certificate. Generated from your keypair jwt: - enableKeyStore: true # Set to 'true' to enable JWT key store - enableKeyRotation: true # Set to 'true' to enable JWT key rotation - enableKeyCleanup: true # Set to 'true' to enable JWT key cleanup - keyRetentionDays: 7 # Number of days to retain old keys - cleanupBatchSize: 100 # Number of keys to clean up in each batch + persistence: true # Set to 'true' to enable JWT key store + enableKeyRotation: true # Set to 'true' to enable key pair rotation + enableKeyCleanup: true # Set to 'true' to enable key pair cleanup + keyRetentionDays: 7 # Number of days to retain old keys. The default is 7 days. + cleanupBatchSize: 100 # Number of keys to clean up in each batch. The default is 100. + secureCookie: false # Set to 'true' to use secure cookies for JWTs premium: key: 00000000-0000-0000-0000-000000000000 diff --git a/app/core/src/main/resources/static/js/fetch-utils.js b/app/core/src/main/resources/static/js/fetch-utils.js index 5946dc100..73c26ffb3 100644 --- a/app/core/src/main/resources/static/js/fetch-utils.js +++ b/app/core/src/main/resources/static/js/fetch-utils.js @@ -62,11 +62,15 @@ window.JWTManager = { fetch('/logout', { method: 'POST', credentials: 'include' - }).then(() => { - window.location.href = '/login'; + }).then(response => { + if (response.redirected) { + window.location.href = response.url; + } else { + window.location.href = '/login?logout=true'; + } }).catch(() => { - // Even if logout fails, redirect to login - window.location.href = '/login'; + // If logout fails, let server handle it + window.location.href = '/logout'; }); } }; diff --git a/app/core/src/main/resources/static/js/jwt-init.js b/app/core/src/main/resources/static/js/jwt-init.js index 4a8218c47..a2733bf35 100644 --- a/app/core/src/main/resources/static/js/jwt-init.js +++ b/app/core/src/main/resources/static/js/jwt-init.js @@ -47,11 +47,14 @@ console.log('User is not authenticated or token expired'); // Only redirect to login if we're not already on login/register pages const currentPath = window.location.pathname; + const currentSearch = window.location.search; + // Don't redirect if we're on logout page or already being logged out if (!currentPath.includes('/login') && !currentPath.includes('/register') && !currentPath.includes('/oauth') && !currentPath.includes('/saml') && - !currentPath.includes('/error')) { + !currentPath.includes('/error') && + !currentSearch.includes('logout=true')) { // Redirect to login after a short delay to allow other scripts to load setTimeout(() => { window.location.href = '/login'; diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/InitialSecuritySetup.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/InitialSecuritySetup.java index 4b09fe0e9..e145e2754 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/InitialSecuritySetup.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/InitialSecuritySetup.java @@ -43,7 +43,6 @@ public class InitialSecuritySetup { } } - userService.migrateOauth2ToSSO(); assignUsersToDefaultTeamIfMissing(); initializeInternalApiUser(); } catch (IllegalArgumentException | SQLException | UnsupportedProviderException e) { diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java index 830f8f195..46d0e7d3d 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/config/AccountWebController.java @@ -77,8 +77,11 @@ public class AccountWebController { @GetMapping("/login") public String login(HttpServletRequest request, Model model, Authentication authentication) { - // If the user is already authenticated, redirect them to the home page. - if (authentication != null && authentication.isAuthenticated()) { + // If the user is already authenticated and it's not a logout scenario, redirect them to the + // home page. + if (authentication != null + && authentication.isAuthenticated() + && request.getParameter("logout") == null) { return "redirect:/"; } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/configuration/SecurityConfiguration.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/configuration/SecurityConfiguration.java index ed309afda..7f2afc735 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/configuration/SecurityConfiguration.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/configuration/SecurityConfiguration.java @@ -186,7 +186,7 @@ public class SecurityConfiguration { // Configure session management based on JWT setting http.sessionManagement( sessionManagement -> { - if (v2Enabled && !securityProperties.isSaml2Active()) { + if (v2Enabled) { sessionManagement.sessionCreationPolicy( SessionCreationPolicy.STATELESS); } else { diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java index 4a0115c1a..68bf44b3e 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java @@ -69,10 +69,11 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter { } String jwtToken = jwtService.extractToken(request); - + // todo: X-API-KEY if (jwtToken == null) { // If they are unauthenticated and navigating to '/', redirect to '/login' instead of // sending a 401 + // todo: any unauthenticated requests should redirect to login if ("/".equals(request.getRequestURI()) && "GET".equalsIgnoreCase(request.getMethod())) { response.sendRedirect("/login"); @@ -130,11 +131,6 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter { authToken.setDetails(new WebAuthenticationDetailsSource().buildDetails(request)); SecurityContextHolder.getContext().setAuthentication(authToken); - - log.info( - "JWT authentication successful for user: {} - Authentication set in SecurityContext", - username); - } else { throw new UsernameNotFoundException("User not found: " + username); } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java index f6074ceff..f51a9d543 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java @@ -65,13 +65,6 @@ public class UserAuthenticationFilter extends OncePerRequestFilter { String requestURI = request.getRequestURI(); Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); - log.info( - "UserAuthenticationFilter - Authentication from SecurityContext: {}", - authentication != null - ? authentication.getClass().getSimpleName() - + " for " - + authentication.getName() - : "null"); // Check for session expiration (unsure if needed) // if (authentication != null && authentication.isAuthenticated()) { @@ -117,7 +110,7 @@ public class UserAuthenticationFilter extends OncePerRequestFilter { String method = request.getMethod(); String contextPath = request.getContextPath(); - if ("GET".equalsIgnoreCase(method) && !(contextPath + "/login").equals(requestURI)) { + if ("GET".equalsIgnoreCase(method) && !requestURI.startsWith(contextPath + "/login")) { response.sendRedirect(contextPath + "/login"); // redirect to the login page } else { response.setStatus(HttpStatus.UNAUTHORIZED.value()); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/model/AuthenticationType.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/model/AuthenticationType.java index cf9f15e35..c92c1655e 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/model/AuthenticationType.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/model/AuthenticationType.java @@ -2,7 +2,8 @@ package stirling.software.proprietary.security.model; public enum AuthenticationType { WEB, - @Deprecated(since = "1.0.2") SSO, + @Deprecated(since = "1.0.2") + SSO, OAUTH2, SAML2 } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/saml2/CustomSaml2AuthenticationSuccessHandler.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/saml2/CustomSaml2AuthenticationSuccessHandler.java index 57d667aa1..3255cbc15 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/saml2/CustomSaml2AuthenticationSuccessHandler.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/saml2/CustomSaml2AuthenticationSuccessHandler.java @@ -121,7 +121,7 @@ public class CustomSaml2AuthenticationSuccessHandler username, saml2Properties.getAutoCreateUser(), SAML2); log.debug("Successfully processed authentication for user: {}", username); - generateJWT(response, authentication); + generateJwt(response, authentication); response.sendRedirect(contextPath + "/"); } catch (IllegalArgumentException | SQLException | UnsupportedProviderException e) { log.debug( @@ -136,7 +136,7 @@ public class CustomSaml2AuthenticationSuccessHandler } } - private void generateJWT(HttpServletResponse response, Authentication authentication) { + private void generateJwt(HttpServletResponse response, Authentication authentication) { if (jwtService.isJwtEnabled()) { String jwt = jwtService.generateToken( diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtService.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtService.java index a8c7db2f5..32701aa59 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtService.java @@ -10,6 +10,7 @@ import java.util.function.Function; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.beans.factory.annotation.Value; import org.springframework.http.ResponseCookie; import org.springframework.security.core.Authentication; import org.springframework.security.core.userdetails.UserDetails; @@ -41,15 +42,17 @@ public class JwtService implements JwtServiceInterface { private static final String AUTHORIZATION_HEADER = "Authorization"; private static final String BEARER_PREFIX = "Bearer "; private static final String ISSUER = "Stirling PDF"; - private static final long EXPIRATION = 300000; // 5 minutes in milliseconds + private static final long EXPIRATION = 3600000; - private final JwtKeystoreServiceInterface keystoreService; + @Value("${stirling.security.jwt.secureCookie:true}") + private boolean secureCookie; + + private final KeystoreServiceInterface keystoreService; private final boolean v2Enabled; @Autowired public JwtService( - @Qualifier("v2Enabled") boolean v2Enabled, - JwtKeystoreServiceInterface keystoreService) { + @Qualifier("v2Enabled") boolean v2Enabled, KeystoreServiceInterface keystoreService) { this.v2Enabled = v2Enabled; this.keystoreService = keystoreService; } @@ -127,13 +130,13 @@ public class JwtService implements JwtServiceInterface { private Claims extractAllClaims(String token) { try { - // Extract key ID from token header if present String keyId = extractKeyId(token); KeyPair keyPair; if (keyId != null) { log.debug("Looking up key pair for key ID: {}", keyId); - Optional specificKeyPair = keystoreService.getKeyPairByKeyId(keyId); + Optional specificKeyPair = + keystoreService.getKeyPairByKeyId(keyId); // todo: move to in-memory cache if (specificKeyPair.isPresent()) { keyPair = specificKeyPair.get(); @@ -179,13 +182,8 @@ public class JwtService implements JwtServiceInterface { @Override public String extractToken(HttpServletRequest request) { - String authHeader = request.getHeader(AUTHORIZATION_HEADER); - - if (authHeader != null && authHeader.startsWith(BEARER_PREFIX)) { - return authHeader.substring(BEARER_PREFIX.length()); - } - Cookie[] cookies = request.getCookies(); + if (cookies != null) { for (Cookie cookie : cookies) { if (JWT_COOKIE_NAME.equals(cookie.getName())) { @@ -204,8 +202,8 @@ public class JwtService implements JwtServiceInterface { ResponseCookie cookie = ResponseCookie.from(JWT_COOKIE_NAME, Newlines.stripAll(token)) .httpOnly(true) - .secure(true) - .sameSite("None") + .secure(secureCookie) + .sameSite("Strict") .maxAge(EXPIRATION / 1000) .path("/") .build(); @@ -220,7 +218,7 @@ public class JwtService implements JwtServiceInterface { ResponseCookie cookie = ResponseCookie.from(JWT_COOKIE_NAME, "") .httpOnly(true) - .secure(true) + .secure(secureCookie) .sameSite("None") .maxAge(0) .path("/") diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeyCleanupService.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeyPairCleanupService.java similarity index 91% rename from app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeyCleanupService.java rename to app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeyPairCleanupService.java index 6f5a43b8c..7727dc1bc 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeyCleanupService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeyPairCleanupService.java @@ -10,12 +10,15 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBooleanProperty; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.scheduling.annotation.Scheduled; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; +import jakarta.annotation.PostConstruct; + import lombok.extern.slf4j.Slf4j; import stirling.software.common.configuration.InstallationPathConfig; @@ -25,16 +28,17 @@ import stirling.software.proprietary.security.model.JwtSigningKey; @Slf4j @Service -public class JwtKeyCleanupService { +@ConditionalOnBooleanProperty("v2") +public class KeyPairCleanupService { private final JwtSigningKeyRepository signingKeyRepository; - private final JwtKeystoreService keystoreService; + private final KeystoreService keystoreService; private final ApplicationProperties.Security.Jwt jwtProperties; @Autowired - public JwtKeyCleanupService( + public KeyPairCleanupService( JwtSigningKeyRepository signingKeyRepository, - JwtKeystoreService keystoreService, + KeystoreService keystoreService, ApplicationProperties applicationProperties) { this.signingKeyRepository = signingKeyRepository; this.keystoreService = keystoreService; @@ -42,6 +46,7 @@ public class JwtKeyCleanupService { } @Transactional + @PostConstruct @Scheduled(fixedDelay = 24, timeUnit = TimeUnit.HOURS) public void cleanup() { if (!jwtProperties.isEnableKeyCleanup() || !keystoreService.isKeystoreEnabled()) { @@ -111,7 +116,7 @@ public class JwtKeyCleanupService { } Path privateKeyDirectory = Paths.get(InstallationPathConfig.getPrivateKeyPath()); - Path keyFile = privateKeyDirectory.resolve(keyId + JwtKeystoreService.KEY_SUFFIX); + Path keyFile = privateKeyDirectory.resolve(keyId + KeystoreService.KEY_SUFFIX); if (Files.exists(keyFile)) { Files.delete(keyFile); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeystoreService.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeystoreService.java similarity index 96% rename from app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeystoreService.java rename to app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeystoreService.java index 64c1900e6..3cb73aadf 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeystoreService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeystoreService.java @@ -33,7 +33,7 @@ import stirling.software.proprietary.security.model.JwtSigningKey; @Slf4j @Service -public class JwtKeystoreService implements JwtKeystoreServiceInterface { +public class KeystoreService implements KeystoreServiceInterface { public static final String KEY_SUFFIX = ".key"; private final JwtSigningKeyRepository signingKeyRepository; @@ -43,7 +43,7 @@ public class JwtKeystoreService implements JwtKeystoreServiceInterface { private volatile String currentKeyId; @Autowired - public JwtKeystoreService( + public KeystoreService( JwtSigningKeyRepository signingKeyRepository, ApplicationProperties applicationProperties) { this.signingKeyRepository = signingKeyRepository; @@ -53,7 +53,6 @@ public class JwtKeystoreService implements JwtKeystoreServiceInterface { @PostConstruct public void initializeKeystore() { if (!isKeystoreEnabled()) { - log.info("Keystore is disabled, using in-memory key generation"); return; } @@ -61,7 +60,7 @@ public class JwtKeystoreService implements JwtKeystoreServiceInterface { ensurePrivateKeyDirectoryExists(); loadOrGenerateKeypair(); } catch (Exception e) { - log.error("Failed to initialize keystore, falling back to in-memory generation", e); + log.error("Failed to initialize keystore, using in-memory generation", e); } } @@ -153,7 +152,7 @@ public class JwtKeystoreService implements JwtKeystoreServiceInterface { } private KeyPair generateRSAKeypair() { - KeyPairGenerator keyPairGenerator = null; + KeyPairGenerator keyPairGenerator; try { keyPairGenerator = KeyPairGenerator.getInstance("RSA"); @@ -213,6 +212,7 @@ public class JwtKeystoreService implements JwtKeystoreServiceInterface { byte[] keyBytes = Base64.getDecoder().decode(encodedKey); PKCS8EncodedKeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); + return keyFactory.generatePrivate(keySpec); } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeystoreServiceInterface.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeystoreServiceInterface.java similarity index 86% rename from app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeystoreServiceInterface.java rename to app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeystoreServiceInterface.java index 4cf9d8f55..dc0564980 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/JwtKeystoreServiceInterface.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/KeystoreServiceInterface.java @@ -3,7 +3,7 @@ package stirling.software.proprietary.security.service; import java.security.KeyPair; import java.util.Optional; -public interface JwtKeystoreServiceInterface { +public interface KeystoreServiceInterface { KeyPair getActiveKeyPair(); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java index 982f551ca..6f213b25e 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java @@ -1,8 +1,5 @@ package stirling.software.proprietary.security.service; -import static stirling.software.proprietary.security.model.AuthenticationType.OAUTH2; -import static stirling.software.proprietary.security.model.AuthenticationType.SSO; - import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; @@ -63,17 +60,6 @@ public class UserService implements UserServiceInterface { private final ApplicationProperties.Security.OAUTH2 oAuth2; - @Transactional - public void migrateOauth2ToSSO() { - userRepository - .findByAuthenticationTypeIgnoreCase(OAUTH2.toString()) - .forEach( - user -> { - user.setAuthenticationType(SSO); - userRepository.save(user); - }); - } - // Handle OAUTH2 login and user auto creation. public void processSSOPostLogin( String username, boolean autoCreateUser, AuthenticationType type) diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtServiceTest.java index 288dd2adb..4f8726335 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtServiceTest.java @@ -11,6 +11,8 @@ import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.security.core.Authentication; @@ -55,7 +57,7 @@ class JwtServiceTest { private HttpServletResponse response; @Mock - private JwtKeystoreServiceInterface keystoreService; + private KeystoreServiceInterface keystoreService; private JwtService jwtService; private KeyPair testKeyPair; @@ -201,19 +203,10 @@ class JwtServiceTest { assertThrows(AuthenticationFailureException.class, () -> jwtService.extractClaims("invalid-token")); } - @Test - void testExtractTokenWithAuthorizationHeader() { - String token = "test-token"; - when(request.getHeader("Authorization")).thenReturn("Bearer " + token); - - assertEquals(token, jwtService.extractToken(request)); - } - @Test void testExtractTokenWithCookie() { String token = "test-token"; Cookie[] cookies = { new Cookie("stirling_jwt", token) }; - when(request.getHeader("Authorization")).thenReturn(null); when(request.getCookies()).thenReturn(cookies); assertEquals(token, jwtService.extractToken(request)); @@ -221,7 +214,6 @@ class JwtServiceTest { @Test void testExtractTokenWithNoCookies() { - when(request.getHeader("Authorization")).thenReturn(null); when(request.getCookies()).thenReturn(null); assertNull(jwtService.extractToken(request)); @@ -230,7 +222,6 @@ class JwtServiceTest { @Test void testExtractTokenWithWrongCookie() { Cookie[] cookies = {new Cookie("OTHER_COOKIE", "value")}; - when(request.getHeader("Authorization")).thenReturn(null); when(request.getCookies()).thenReturn(cookies); assertNull(jwtService.extractToken(request)); @@ -238,22 +229,30 @@ class JwtServiceTest { @Test void testExtractTokenWithInvalidAuthorizationHeader() { - when(request.getHeader("Authorization")).thenReturn("Basic token"); when(request.getCookies()).thenReturn(null); assertNull(jwtService.extractToken(request)); } - @Test - void testAddToken() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void testAddToken(boolean secureCookie) throws Exception { String token = "test-token"; - jwtService.addToken(response, token); + // Create new JwtService instance with the secureCookie parameter + JwtService testJwtService = createJwtServiceWithSecureCookie(secureCookie); + + testJwtService.addToken(response, token); verify(response).setHeader("Authorization", "Bearer " + token); verify(response).addHeader(eq("Set-Cookie"), contains("stirling_jwt=" + token)); verify(response).addHeader(eq("Set-Cookie"), contains("HttpOnly")); - verify(response).addHeader(eq("Set-Cookie"), contains("Secure")); + + if (secureCookie) { + verify(response).addHeader(eq("Set-Cookie"), contains("Secure")); + } else { + verify(response, org.mockito.Mockito.never()).addHeader(eq("Set-Cookie"), contains("Secure")); + } } @Test @@ -327,4 +326,16 @@ class JwtServiceTest { // Verify fallback to active keypair was used (called multiple times during token operations) verify(keystoreService, atLeast(1)).getActiveKeyPair(); } + + private JwtService createJwtServiceWithSecureCookie(boolean secureCookie) throws Exception { + // Use reflection to create JwtService with custom secureCookie value + JwtService testService = new JwtService(true, keystoreService); + + // Set the secureCookie field using reflection + java.lang.reflect.Field secureCookieField = JwtService.class.getDeclaredField("secureCookie"); + secureCookieField.setAccessible(true); + secureCookieField.set(testService, secureCookie); + + return testService; + } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtKeyCleanupServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/KeyPairCleanupServiceTest.java similarity index 97% rename from app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtKeyCleanupServiceTest.java rename to app/proprietary/src/test/java/stirling/software/proprietary/security/service/KeyPairCleanupServiceTest.java index bdfc25947..ae07362c8 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtKeyCleanupServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/KeyPairCleanupServiceTest.java @@ -27,13 +27,13 @@ import stirling.software.proprietary.security.database.repository.JwtSigningKeyR import stirling.software.proprietary.security.model.JwtSigningKey; @ExtendWith(MockitoExtension.class) -class JwtKeyCleanupServiceTest { +class KeyPairCleanupServiceTest { @Mock private JwtSigningKeyRepository signingKeyRepository; @Mock - private JwtKeystoreService keystoreService; + private KeystoreService keystoreService; @Mock private ApplicationProperties applicationProperties; @@ -47,7 +47,7 @@ class JwtKeyCleanupServiceTest { @TempDir private Path tempDir; - private JwtKeyCleanupService cleanupService; + private KeyPairCleanupService cleanupService; @BeforeEach void setUp() { @@ -59,7 +59,7 @@ class JwtKeyCleanupServiceTest { lenient().when(jwtConfig.getCleanupBatchSize()).thenReturn(100); lenient().when(keystoreService.isKeystoreEnabled()).thenReturn(true); - cleanupService = new JwtKeyCleanupService(signingKeyRepository, keystoreService, applicationProperties); + cleanupService = new KeyPairCleanupService(signingKeyRepository, keystoreService, applicationProperties); } @@ -101,7 +101,7 @@ class JwtKeyCleanupServiceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - + createTestKeyFile("key-1"); createTestKeyFile("key-2"); @@ -134,7 +134,7 @@ class JwtKeyCleanupServiceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - + createTestKeyFile("key-1"); createTestKeyFile("key-2"); createTestKeyFile("key-3"); @@ -161,7 +161,7 @@ class JwtKeyCleanupServiceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - + createTestKeyFile("key-1"); when(signingKeyRepository.countKeysEligibleForCleanup(any(LocalDateTime.class))).thenReturn(2L); diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtKeystoreServiceInterfaceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/KeystoreServiceInterfaceTest.java similarity index 90% rename from app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtKeystoreServiceInterfaceTest.java rename to app/proprietary/src/test/java/stirling/software/proprietary/security/service/KeystoreServiceInterfaceTest.java index 8979bcbf6..2380eb00b 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/JwtKeystoreServiceInterfaceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/KeystoreServiceInterfaceTest.java @@ -32,7 +32,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) -class JwtKeystoreServiceInterfaceTest { +class KeystoreServiceInterfaceTest { @Mock private JwtSigningKeyRepository repository; @@ -49,7 +49,7 @@ class JwtKeystoreServiceInterfaceTest { @TempDir Path tempDir; - private JwtKeystoreService keystoreService; + private KeystoreService keystoreService; private KeyPair testKeyPair; @BeforeEach @@ -70,7 +70,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); assertEquals(keystoreEnabled, keystoreService.isKeystoreEnabled()); } @@ -82,7 +82,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); KeyPair result = keystoreService.getActiveKeyPair(); @@ -98,7 +98,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); keystoreService.initializeKeystore(); KeyPair result = keystoreService.getActiveKeyPair(); @@ -122,7 +122,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); keystoreService.initializeKeystore(); KeyPair result = keystoreService.getActiveKeyPair(); @@ -146,7 +146,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); Optional result = keystoreService.getKeyPairByKeyId(keyId); @@ -163,7 +163,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); Optional result = keystoreService.getKeyPairByKeyId(keyId); @@ -177,7 +177,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); Optional result = keystoreService.getKeyPairByKeyId("any-key"); @@ -191,7 +191,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); keystoreService.initializeKeystore(); assertTrue(Files.exists(tempDir)); @@ -209,7 +209,7 @@ class JwtKeystoreServiceInterfaceTest { try (MockedStatic mockedStatic = mockStatic(InstallationPathConfig.class)) { mockedStatic.when(InstallationPathConfig::getPrivateKeyPath).thenReturn(tempDir.toString()); - keystoreService = new JwtKeystoreService(repository, applicationProperties); + keystoreService = new KeystoreService(repository, applicationProperties); keystoreService.initializeKeystore(); KeyPair result = keystoreService.getActiveKeyPair(); diff --git a/exampleYmlFiles/test_cicd.yml b/exampleYmlFiles/test_cicd.yml index 31e24da48..086f862d5 100644 --- a/exampleYmlFiles/test_cicd.yml +++ b/exampleYmlFiles/test_cicd.yml @@ -20,6 +20,7 @@ services: environment: DISABLE_ADDITIONAL_FEATURES: "false" SECURITY_ENABLELOGIN: "true" + V2: "false" PUID: 1002 PGID: 1002 UMASK: "022"