mirror of
https://github.com/arsvendg/Stirling-PDF.git
synced 2026-07-16 19:33:11 +02:00
feat(common,core,proprietary): remove unused injections, enhance type safety, and improve test mocks (#4213)
# Description of Changes This PR introduces several refactorings and minor enhancements across the `common`, `core`, and `proprietary` modules: - **Dependency Injection Cleanup** - Removed unused constructor-injected dependencies (e.g., `FileOrUploadService`, `ApplicationProperties`, redundant `@Autowired` annotations). - Simplified constructors to only require actively used dependencies. - **Model Enhancements** - Added `@NoArgsConstructor` to `FileInfo`, `PdfMetadata`, and `SignatureFile` to improve serialization/deserialization support. - **Service Improvements** - Improved `JobExecutorService` content type retrieval by assigning `MediaType` to a variable before conversion. - Enhanced `KeyPersistenceService` with type-safe `.filter(JwtVerificationKey.class::isInstance)`. - Annotated `decodePublicKey` in `KeyPersistenceService` with `@Override` for clarity. - **Controller & API Changes** - Updated `AdminSettingsController` to use `TypeReference<Map<String,Object>>` for safer conversion. - Improved long log and description strings with consistent formatting. - **Testing Updates** - Replaced `.lenient()` mock settings with `.defaultAnswer(RETURNS_DEFAULTS)` for `FileToPdf` static mocks. - Used `ArgumentMatchers.<TypeReference<List<BookmarkItem>>>any()` in `EditTableOfContentsControllerTest` for type safety. - Updated `UserServiceTest` default `AuthenticationType` from `SSO` to `OAUTH2`. - **Formatting** - Broke up long log/debug lines for better readability. - Removed redundant `@SuppressWarnings` where type safety was ensured. These changes aim to make the codebase leaner, more type-safe, and maintainable, while improving test reliability. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/DeveloperGuide.md#6-testing) for more details.
This commit is contained in:
Ludy
+22
-11
@@ -21,6 +21,7 @@ import org.springframework.web.bind.annotation.RequestMapping;
|
||||
import org.springframework.web.bind.annotation.RequestParam;
|
||||
import org.springframework.web.util.HtmlUtils;
|
||||
|
||||
import com.fasterxml.jackson.core.type.TypeReference;
|
||||
import com.fasterxml.jackson.databind.ObjectMapper;
|
||||
|
||||
import io.swagger.v3.oas.annotations.Operation;
|
||||
@@ -81,7 +82,8 @@ public class AdminSettingsController {
|
||||
@Operation(
|
||||
summary = "Get all application settings",
|
||||
description =
|
||||
"Retrieve all current application settings. Use includePending=true to include settings that will take effect after restart. Admin access required.")
|
||||
"Retrieve all current application settings. Use includePending=true to include"
|
||||
+ " settings that will take effect after restart. Admin access required.")
|
||||
@ApiResponses(
|
||||
value = {
|
||||
@ApiResponse(responseCode = "200", description = "Settings retrieved successfully"),
|
||||
@@ -95,7 +97,9 @@ public class AdminSettingsController {
|
||||
log.debug("Admin requested all application settings (includePending={})", includePending);
|
||||
|
||||
// Convert ApplicationProperties to Map
|
||||
Map<String, Object> settings = objectMapper.convertValue(applicationProperties, Map.class);
|
||||
Map<String, Object> settings =
|
||||
objectMapper.convertValue(
|
||||
applicationProperties, new TypeReference<Map<String, Object>>() {});
|
||||
|
||||
if (includePending && !pendingChanges.isEmpty()) {
|
||||
// Merge pending changes into the settings map
|
||||
@@ -112,7 +116,8 @@ public class AdminSettingsController {
|
||||
@Operation(
|
||||
summary = "Get pending settings changes",
|
||||
description =
|
||||
"Retrieve settings that have been modified but not yet applied (require restart). Admin access required.")
|
||||
"Retrieve settings that have been modified but not yet applied (require"
|
||||
+ " restart). Admin access required.")
|
||||
@ApiResponses(
|
||||
value = {
|
||||
@ApiResponse(
|
||||
@@ -137,7 +142,8 @@ public class AdminSettingsController {
|
||||
@Operation(
|
||||
summary = "Update application settings (delta updates)",
|
||||
description =
|
||||
"Update specific application settings using dot notation keys. Only sends changed values. Changes take effect on restart. Admin access required.")
|
||||
"Update specific application settings using dot notation keys. Only sends"
|
||||
+ " changed values. Changes take effect on restart. Admin access required.")
|
||||
@ApiResponses(
|
||||
value = {
|
||||
@ApiResponse(responseCode = "200", description = "Settings updated successfully"),
|
||||
@@ -178,7 +184,8 @@ public class AdminSettingsController {
|
||||
|
||||
return ResponseEntity.ok(
|
||||
String.format(
|
||||
"Successfully updated %d setting(s). Changes will take effect on application restart.",
|
||||
"Successfully updated %d setting(s). Changes will take effect on"
|
||||
+ " application restart.",
|
||||
updatedCount));
|
||||
|
||||
} catch (IOException e) {
|
||||
@@ -199,7 +206,8 @@ public class AdminSettingsController {
|
||||
@Operation(
|
||||
summary = "Get specific settings section",
|
||||
description =
|
||||
"Retrieve settings for a specific section (e.g., security, system, ui). Admin access required.")
|
||||
"Retrieve settings for a specific section (e.g., security, system, ui). Admin"
|
||||
+ " access required.")
|
||||
@ApiResponses(
|
||||
value = {
|
||||
@ApiResponse(
|
||||
@@ -288,7 +296,8 @@ public class AdminSettingsController {
|
||||
String escapedSectionName = HtmlUtils.htmlEscape(sectionName);
|
||||
return ResponseEntity.ok(
|
||||
String.format(
|
||||
"Successfully updated %d setting(s) in section '%s'. Changes will take effect on application restart.",
|
||||
"Successfully updated %d setting(s) in section '%s'. Changes will take"
|
||||
+ " effect on application restart.",
|
||||
updatedCount, escapedSectionName));
|
||||
|
||||
} catch (IOException e) {
|
||||
@@ -308,7 +317,8 @@ public class AdminSettingsController {
|
||||
@Operation(
|
||||
summary = "Get specific setting value",
|
||||
description =
|
||||
"Retrieve value for a specific setting key using dot notation. Admin access required.")
|
||||
"Retrieve value for a specific setting key using dot notation. Admin access"
|
||||
+ " required.")
|
||||
@ApiResponses(
|
||||
value = {
|
||||
@ApiResponse(
|
||||
@@ -348,7 +358,8 @@ public class AdminSettingsController {
|
||||
@Operation(
|
||||
summary = "Update specific setting value",
|
||||
description =
|
||||
"Update value for a specific setting key using dot notation. Admin access required.")
|
||||
"Update value for a specific setting key using dot notation. Admin access"
|
||||
+ " required.")
|
||||
@ApiResponses(
|
||||
value = {
|
||||
@ApiResponse(responseCode = "200", description = "Setting updated successfully"),
|
||||
@@ -376,7 +387,8 @@ public class AdminSettingsController {
|
||||
String escapedKey = HtmlUtils.htmlEscape(key);
|
||||
return ResponseEntity.ok(
|
||||
String.format(
|
||||
"Successfully updated setting '%s'. Changes will take effect on application restart.",
|
||||
"Successfully updated setting '%s'. Changes will take effect on"
|
||||
+ " application restart.",
|
||||
escapedKey));
|
||||
|
||||
} catch (IOException e) {
|
||||
@@ -532,7 +544,6 @@ public class AdminSettingsController {
|
||||
* Recursively mask sensitive fields in settings map. Sensitive fields are replaced with a
|
||||
* status indicator showing if they're configured.
|
||||
*/
|
||||
@SuppressWarnings("unchecked")
|
||||
private Map<String, Object> maskSensitiveFields(Map<String, Object> settings) {
|
||||
return maskSensitiveFieldsWithPath(settings, "");
|
||||
}
|
||||
|
||||
+9
-9
@@ -128,7 +128,7 @@ public class UserAuthenticationFilter extends OncePerRequestFilter {
|
||||
// Check if the authenticated user is disabled and invalidate their session if so
|
||||
if (authentication != null && authentication.isAuthenticated()) {
|
||||
|
||||
LoginMethod loginMethod = LoginMethod.UNKNOWN;
|
||||
UserLoginType loginMethod = UserLoginType.UNKNOWN;
|
||||
|
||||
boolean blockRegistration = false;
|
||||
|
||||
@@ -137,20 +137,20 @@ public class UserAuthenticationFilter extends OncePerRequestFilter {
|
||||
String username = null;
|
||||
if (principal instanceof UserDetails detailsUser) {
|
||||
username = detailsUser.getUsername();
|
||||
loginMethod = LoginMethod.USERDETAILS;
|
||||
loginMethod = UserLoginType.USERDETAILS;
|
||||
} else if (principal instanceof OAuth2User oAuth2User) {
|
||||
username = oAuth2User.getName();
|
||||
loginMethod = LoginMethod.OAUTH2USER;
|
||||
loginMethod = UserLoginType.OAUTH2USER;
|
||||
OAUTH2 oAuth = securityProp.getOauth2();
|
||||
blockRegistration = oAuth != null && oAuth.getBlockRegistration();
|
||||
} else if (principal instanceof CustomSaml2AuthenticatedPrincipal saml2User) {
|
||||
username = saml2User.name();
|
||||
loginMethod = LoginMethod.SAML2USER;
|
||||
loginMethod = UserLoginType.SAML2USER;
|
||||
SAML2 saml2 = securityProp.getSaml2();
|
||||
blockRegistration = saml2 != null && saml2.getBlockRegistration();
|
||||
} else if (principal instanceof String stringUser) {
|
||||
username = stringUser;
|
||||
loginMethod = LoginMethod.STRINGUSER;
|
||||
loginMethod = UserLoginType.STRINGUSER;
|
||||
}
|
||||
|
||||
// Retrieve all active sessions for the user
|
||||
@@ -164,8 +164,8 @@ public class UserAuthenticationFilter extends OncePerRequestFilter {
|
||||
boolean isUserDisabled = userService.isUserDisabled(username);
|
||||
|
||||
boolean notSsoLogin =
|
||||
!LoginMethod.OAUTH2USER.equals(loginMethod)
|
||||
&& !LoginMethod.SAML2USER.equals(loginMethod);
|
||||
!UserLoginType.OAUTH2USER.equals(loginMethod)
|
||||
&& !UserLoginType.SAML2USER.equals(loginMethod);
|
||||
|
||||
// Block user registration if not allowed by configuration
|
||||
if (blockRegistration && !isUserExists) {
|
||||
@@ -200,7 +200,7 @@ public class UserAuthenticationFilter extends OncePerRequestFilter {
|
||||
filterChain.doFilter(request, response);
|
||||
}
|
||||
|
||||
private enum LoginMethod {
|
||||
private enum UserLoginType {
|
||||
USERDETAILS("UserDetails"),
|
||||
OAUTH2USER("OAuth2User"),
|
||||
STRINGUSER("StringUser"),
|
||||
@@ -209,7 +209,7 @@ public class UserAuthenticationFilter extends OncePerRequestFilter {
|
||||
|
||||
private String method;
|
||||
|
||||
LoginMethod(String method) {
|
||||
UserLoginType(String method) {
|
||||
this.method = method;
|
||||
}
|
||||
|
||||
|
||||
+2
-3
@@ -12,7 +12,6 @@ import java.util.Map;
|
||||
import java.util.Optional;
|
||||
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;
|
||||
@@ -53,7 +52,6 @@ public class JwtService implements JwtServiceInterface {
|
||||
private final KeyPersistenceServiceInterface keyPersistenceService;
|
||||
private final boolean v2Enabled;
|
||||
|
||||
@Autowired
|
||||
public JwtService(
|
||||
@Qualifier("v2Enabled") boolean v2Enabled,
|
||||
KeyPersistenceServiceInterface keyPersistenceService) {
|
||||
@@ -155,7 +153,8 @@ public class JwtService implements JwtServiceInterface {
|
||||
keyPair = specificKeyPair.get();
|
||||
} else {
|
||||
log.warn(
|
||||
"Key ID {} not found in keystore, token may have been signed with an expired key",
|
||||
"Key ID {} not found in keystore, token may have been signed with an"
|
||||
+ " expired key",
|
||||
keyId);
|
||||
|
||||
if (keyId.equals(keyPersistenceService.getActiveKey().getKeyId())) {
|
||||
|
||||
-2
@@ -8,7 +8,6 @@ import java.time.LocalDateTime;
|
||||
import java.util.List;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.boot.autoconfigure.condition.ConditionalOnBooleanProperty;
|
||||
import org.springframework.scheduling.annotation.Scheduled;
|
||||
import org.springframework.stereotype.Service;
|
||||
@@ -30,7 +29,6 @@ public class KeyPairCleanupService {
|
||||
private final KeyPersistenceService keyPersistenceService;
|
||||
private final ApplicationProperties.Security.Jwt jwtProperties;
|
||||
|
||||
@Autowired
|
||||
public KeyPairCleanupService(
|
||||
KeyPersistenceService keyPersistenceService,
|
||||
ApplicationProperties applicationProperties) {
|
||||
|
||||
+2
-5
@@ -20,7 +20,6 @@ import java.util.List;
|
||||
import java.util.Optional;
|
||||
import java.util.stream.Collectors;
|
||||
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.cache.Cache;
|
||||
import org.springframework.cache.CacheManager;
|
||||
import org.springframework.cache.annotation.CacheEvict;
|
||||
@@ -43,16 +42,13 @@ public class KeyPersistenceService implements KeyPersistenceServiceInterface {
|
||||
public static final String KEY_SUFFIX = ".key";
|
||||
|
||||
private final ApplicationProperties.Security.Jwt jwtProperties;
|
||||
private final CacheManager cacheManager;
|
||||
private final Cache verifyingKeyCache;
|
||||
|
||||
private volatile JwtVerificationKey activeKey;
|
||||
|
||||
@Autowired
|
||||
public KeyPersistenceService(
|
||||
ApplicationProperties applicationProperties, CacheManager cacheManager) {
|
||||
this.jwtProperties = applicationProperties.getSecurity().getJwt();
|
||||
this.cacheManager = cacheManager;
|
||||
this.verifyingKeyCache = cacheManager.getCache("verifyingKeys");
|
||||
}
|
||||
|
||||
@@ -159,7 +155,7 @@ public class KeyPersistenceService implements KeyPersistenceServiceInterface {
|
||||
nativeCache.asMap().size());
|
||||
|
||||
return nativeCache.asMap().values().stream()
|
||||
.filter(value -> value instanceof JwtVerificationKey)
|
||||
.filter(JwtVerificationKey.class::isInstance)
|
||||
.map(value -> (JwtVerificationKey) value)
|
||||
.filter(
|
||||
key -> {
|
||||
@@ -233,6 +229,7 @@ public class KeyPersistenceService implements KeyPersistenceServiceInterface {
|
||||
return Base64.getEncoder().encodeToString(publicKey.getEncoded());
|
||||
}
|
||||
|
||||
@Override
|
||||
public PublicKey decodePublicKey(String encodedKey)
|
||||
throws NoSuchAlgorithmException, InvalidKeySpecException {
|
||||
byte[] keyBytes = Base64.getDecoder().decode(encodedKey);
|
||||
|
||||
+1
-1
@@ -189,7 +189,7 @@ class UserServiceTest {
|
||||
void testSaveUser_WithValidEmail_Success() throws Exception {
|
||||
// Given
|
||||
String emailUsername = "[email protected]";
|
||||
AuthenticationType authType = AuthenticationType.SSO;
|
||||
AuthenticationType authType = AuthenticationType.OAUTH2;
|
||||
|
||||
when(teamRepository.findByName("Default")).thenReturn(Optional.of(mockTeam));
|
||||
when(userRepository.save(any(User.class))).thenReturn(mockUser);
|
||||
|
||||
Reference in New Issue
Block a user