[NAE-2241] Anonymous access refactor#408
Open
renczesstefan wants to merge 16 commits intorelease/7.0.0-rev10from
Open
[NAE-2241] Anonymous access refactor#408renczesstefan wants to merge 16 commits intorelease/7.0.0-rev10from
renczesstefan wants to merge 16 commits intorelease/7.0.0-rev10from
Conversation
- Introduced `anonymousAuthenticationKey` in `SecurityConfigurationProperties` for better anonymous user identification. - Updated `PublicTaskController` to use `ActorTransformer.toLoggedUser` for cleaner authorization checks. - Enhanced `LoggedUserConfiguration` to initialize the default user factory with `ActorTransformer.setUserFactory`. - Refactored `TaskAuthorizationService` to replace manual user transformation with `ActorTransformer.toUser` for consistency. - Overhauled `PrivateKeyReader` to use `Resource` and Apache Commons IO for improved file handling. - Added `isAnonymous` method to `AbstractActor` for accurate anonymous user detection. - Updated `UserServiceImpl` to handle anonymous users correctly using `ActorTransformer`. These changes ensure better modularity, maintainability, and extend the functionality of authorization and user handling mechanisms.
Introduced `NetgrifHttpRequestTransformFilter` to wrap HTTP requests, enabling custom request handling. Further, added `NetgrifOncePerRequestFilter` as a base proxy filter and improved user authentication with `PublicAuthenticationFilter`. Updated various services to enable anonymous user support and public API access.
Introduced a new RealmFilter to extract the realm information from HTTP headers or request bodies, fallback to a default realm if available, and attach it to the request. Updated related utility functions and PublicAuthenticationFilter to use the extracted realm for better request handling.
Removed the @requiredargsconstructor annotation from RealmFilter and implemented an explicit constructor for dependency injection. Updated NaeSecurityConfiguration to autowire RealmFilter and included it in the security filter chain preceding NetgrifHttpRequestTransformFilter.
Removed the @requiredargsconstructor annotation from RealmFilter and implemented an explicit constructor for dependency injection. Updated NaeSecurityConfiguration to autowire RealmFilter and included it in the security filter chain preceding NetgrifHttpRequestTransformFilter.
…rity context and not auth parameter - modified controller method properties to remove unnecessary auth param - instead of auth param, the security context will be used, because of AnonymousAuthentication token
# Conflicts: # application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.java
Replaced `LoggedUser` with `AbstractUser` across services for enhanced flexibility and alignment. Introduced `@RequiredArgsConstructor` annotation to eliminate redundant constructors and streamline dependency injection. Various private methods were reorganized for clarity and reusability, ensuring consistent coding patterns.
Removed deprecated authentication parameters from controllers, leveraging `ActorTransformer` for handling logged user context. Improved `AnonymousUser` handling by integrating `AnonymousUserRefService` to resolve missing users. Simplified method signatures and enhanced code maintainability.
Replaced specific authority checks with a broader `hasAnyAuthority` method, simplifying permission logic and allowing more flexible role-based access control. Added public endpoints to several APIs to extend functionality for anonymous or unauthorized users where appropriate. Removed unused imports for better code clarity and organization.
Extended `TaskController` to include a new "/public/case" endpoint path for retrieving tasks by cases. Refactored task search logic to leverage new `TaskSearchRequest` and `TaskSearchCaseRequest` builders, improving flexibility and maintainability. Adjusted `AbstractTaskController` to support the updated search functionality with enhanced parameters.
The AnonymousUserRefServiceImpl class was decoupled from the @service annotation and is now registered as a bean in AuthBeansConfiguration. This change also ensures the bean is only created if a UserFactory bean is missing, improving configurability and flexibility.
- Enhanced `AuthorizationService`: - Corrected the `hasAnyAuthority` method to use `equals` instead of `contains`. - Updated pre-authorization checks in the `TaskController` to include the `ANONYMOUS` role. - Improved Anonymous User Support: - Modified `AnonymousUser` and `AnonymousUserRef` to streamline initialization and compatibility with `Serializable`. - Added a new factory method in `DefaultUserFactory` to create `AnonymousUser` instances based on `AnonymousUserRef`. - Revised `ActorTransformer` Factories: - Injected `LoggedUserFactory` and `UserFactory` into `LoggedUserConfiguration` for better dependency management. - Updated `ActorTransformer` to support `LoggedUser` with improved user conversion logic. - Miscellaneous: - Renamed constants and clarified field documentation for better readability and accuracy.
renczesstefan
added
bugfix
WalkthroughThe PR restructures authentication and authorization handling by replacing direct Spring Security Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NetgrifHttpRequestTransformFilter
participant RealmFilter
participant PublicAuthenticationFilter
participant SecurityContext
participant RealmService
participant AnonymousUserRefService
participant AuthenticationProvider
Client->>NetgrifHttpRequestTransformFilter: HTTP Request
activate NetgrifHttpRequestTransformFilter
NetgrifHttpRequestTransformFilter->>NetgrifHttpRequestTransformFilter: Wrap in NetgrifHttpServletRequest
NetgrifHttpRequestTransformFilter->>RealmFilter: pass transformed request
deactivate NetgrifHttpRequestTransformFilter
activate RealmFilter
RealmFilter->>RealmFilter: Extract Realm from X-Realm-ID header
alt Realm not in header
RealmFilter->>RealmFilter: Check request body for realmId/realName
end
alt Still no Realm
RealmFilter->>RealmService: Get default Realm
RealmService-->>RealmFilter: Default Realm or null
end
RealmFilter->>NetgrifHttpRequestTransformFilter: Add Realm to additionalParams
RealmFilter->>PublicAuthenticationFilter: pass request with Realm
deactivate RealmFilter
activate PublicAuthenticationFilter
alt User already authenticated
PublicAuthenticationFilter->>SecurityContext: Continue (user exists)
else User not authenticated
PublicAuthenticationFilter->>RealmFilter: Get Realm from request
alt Realm marked for public access
PublicAuthenticationFilter->>AnonymousUserRefService: Get anonymous user for realm
AnonymousUserRefService-->>PublicAuthenticationFilter: AnonymousUserRef
alt AnonymousUserRef found
PublicAuthenticationFilter->>PublicAuthenticationFilter: Create AnonymousUser + LoggedUserImpl
PublicAuthenticationFilter->>AuthenticationProvider: Create AnonymousAuthenticationToken
AuthenticationProvider-->>PublicAuthenticationFilter: Token
PublicAuthenticationFilter->>SecurityContext: Set authentication
end
end
end
PublicAuthenticationFilter-->>Client: Continue to application
deactivate PublicAuthenticationFilter
Estimated code review effort🎯 5 (Critical) | ⏱️ ~95 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Removed 'ANONYMOUS' authority from task retrieval endpoint to tighten access control. Added `@NotNull` and `@Size` annotations to enforce input validation for improved reliability.
Deleted the `PublicPetriNetController`, `PublicTaskController`, `PublicUserController`, and `PublicWorkflowController` classes to streamline the codebase and eliminate unused public endpoints. Also updated test cases to reflect these changes by removing dependencies and ensuring existing functionality remains intact.
coderabbitai
bot
added
breaking change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
application-engine/src/main/java/com/netgrif/application/engine/configuration/security/jwt/JwtService.java (1)
39-64:⚠️ Potential issue | 🟠 MajorInject Spring's managed
ObjectMapperinstead of creating a new instance.JwtService currently creates its own ObjectMapper in the
@PostConstructmethod, bypassing Spring's auto-configured bean. This prevents the JWT serialization from using any custom Jackson configuration (such as theauthConfigModuledefined inJacksonConfigurationor other Spring-registered modules). Compare withRealmServiceImpl, which correctly injects the managed mapper via@Autowired.Make
objectMappera constructor parameter (following the existing@RequiredArgsConstructorpattern), removeconfigureObjectMapper(), and let Spring provide the pre-configured instance.🔧 Suggested refactor
- private ObjectMapper objectMapper; + private final ObjectMapper objectMapper; `@PostConstruct` private void resolveSecret() { - configureObjectMapper(); try { PrivateKeyReader reader = new PrivateKeyReader(properties.getAlgorithm()); secret = reader.get(properties.getPrivateKey()).getEncoded(); - private void configureObjectMapper() { - objectMapper = new ObjectMapper(); - objectMapper.findAndRegisterModules(); - }Also applies to: 115-118
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
348-357:⚠️ Potential issue | 🟠 MajorQuestionable fallback logic:
findByIdreturns anonymous user when requested ID not found.When a user with the specified
idis not found, the method falls back to returning anAnonymousUserbased on the realm. This changes the semantic contract offindById:
- Callers expect to find the user with the given ID or receive
null- Now they may receive an anonymous user with a different ID than requested
This could cause subtle bugs where code expects a specific user but silently receives an anonymous user.
Consider whether this fallback belongs here or if it should be handled at a higher level where the intent to fall back to anonymous is explicit:
🔍 Suggested approach
`@Override` public AbstractUser findById(String id, String realmId) { log.debug("Finding user by ID [{}]", id); String collectionName = collectionNameProvider.getCollectionNameForRealm(realmId); Optional<User> userOpt = userRepository.findById(new ObjectId(id), mongoTemplate, collectionName); - if (userOpt.isPresent()) { - return userOpt.get(); - } - Optional<AnonymousUserRef> anonymousUserRefOptional = anonymousUserRefService.getRef(realmId); - return anonymousUserRefOptional.map(anonymousUserRef -> new AnonymousUser(anonymousUserRef, authorityService.getOrCreate(Authority.anonymous))).orElse(null); + return userOpt.orElse(null); } + +// Add a separate method if anonymous fallback is needed: +public AbstractUser findByIdOrAnonymous(String id, String realmId) { ... }nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/AnonymousUserRef.java (1)
72-82:⚠️ Potential issue | 🟡 MinorDuplicate Javadoc blocks.
There are duplicate Javadoc comment blocks for the
processRolesfield (lines 72-81 and the implicit preceding block). Similarly, duplicate Javadocs appear forgroupIds(lines 84-89),sessionTimeout(lines 92-99), andgroups(lines 102-112). Please remove the redundant comments.application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java (1)
136-144: 🧹 Nitpick | 🔵 TrivialInconsistent authentication resolution pattern.
The
searchmethod at line 142 still uses direct cast(LoggedUser) auth.getPrincipal(), whilegetLoggedUserandpreferencesnow useresolveAuthenticationToken. Similarly,assignRolesToUser(line 220) andsavePreferences(line 319) use direct casts.Consider applying the new helper consistently across all endpoints, or document why certain endpoints don't need it.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowAuthorizationService.java (1)
3-4: 🧹 Nitpick | 🔵 TrivialRemove unused import.
The
LoggedUserimport on line 4 is no longer used in this interface after the parameter type changes toAbstractUser.🧹 Proposed cleanup
import com.netgrif.application.engine.objects.auth.domain.AbstractUser; -import com.netgrif.application.engine.objects.auth.domain.LoggedUser; import com.netgrif.application.engine.objects.petrinet.domain.PetriNet;application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/LocalisedTaskResource.java (1)
9-9: 🧹 Nitpick | 🔵 TrivialRemove unused import.
The
Authenticationimport is no longer used after the link-building updates removed theAuthenticationparameter from method references.🧹 Proposed cleanup
import org.springframework.hateoas.Link; import org.springframework.hateoas.server.mvc.WebMvcLinkBuilder; -import org.springframework.security.core.Authentication; import java.io.FileNotFoundException;application-engine/src/main/java/com/netgrif/application/engine/configuration/NaeSecurityConfiguration.java (1)
30-30: 🧹 Nitpick | 🔵 TrivialUnused import:
org.springframework.stereotype.Controller.This import doesn't appear to be used in the class.
🧹 Remove unused import
-import org.springframework.stereotype.Controller;application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskAuthorizationService.java (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove the unused
LoggedUserimport.All method signatures in this interface use
AbstractUserexclusively. TheLoggedUserimport on line 4 is not referenced anywhere and should be removed.
🤖 Fix all issues with AI agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/service/AuthorizationService.java`:
- Around line 25-31: In AuthorizationService.hasAnyAuthority, convert the
incoming varargs (rename parameter from authority to authorities) into a
Set<String> once, then check membership against the logged user's authorities
using a contains lookup to avoid the nested Arrays.stream per-item scan;
specifically, build the Set from the authorities parameter and replace the
Arrays.stream(...).anyMatch(...) logic with a single pass that tests
loggedUser.getAuthoritySet() entries' getAuthority() against the Set (use
getLoggedUserFromContext() as currently used or restore impersonation call if
needed).
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java`:
- Around line 342-351: The method resolveAuthenticationToken currently falls
back to SecurityContextHolder; remove that redundant fallback and simplify it to
just return the principal from the provided Authentication parameter (cast to
LoggedUser) or null if the injected auth is null. Update
resolveAuthenticationToken(Authentication auth) to only check auth and return
(LoggedUser) auth.getPrincipal() when non-null; remove any references to
SecurityContextHolder and its getContext() usage. This keeps callers like
getLoggedUser and preferences relying on the injected Authentication and removes
unnecessary defensive logic.
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/SecurityConfigurationProperties.java`:
- Around line 422-425: Add a Javadoc comment for the PublicProperties.enabled
field: describe that it toggles whether the public/anonymous access is enabled
for the application, state the default value (true), and briefly note the effect
(e.g., when false public endpoints/authentication for anonymous users are
disabled). Place the Javadoc immediately above the private boolean enabled =
true; declaration inside the PublicProperties class.
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/security/jwt/JwtService.java`:
- Around line 78-85: The JWT parsing code in JwtService uses
userMap.get("stringId") which will be null for older tokens; update the
LoggedUser population (in the block using LinkedHashMap<String,Object> userMap
and LoggedUser/LoggedUserImpl) to fall back to userMap.get("id") when "stringId"
is missing (convert to String if necessary) before calling user.setId(...), so
old tokens still populate user.id; keep the rest of fields the same.
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/security/jwt/PrivateKeyReader.java`:
- Around line 22-24: The get method in PrivateKeyReader currently opens a
FileInputStream from resource.getFile() and doesn't close it; change it to call
resource.getInputStream() and read the bytes inside a try-with-resources block
so the InputStream is always closed; update the logic in PrivateKeyReader.get
(and any local variables like fileInputStream/keyBytes) to use the InputStream
from resource.getInputStream() and IOUtils.toByteArray(inputStream) inside the
try-with-resources, preserving the same exceptions (IOException,
NoSuchAlgorithmException, InvalidKeySpecException).
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/security/PublicAuthenticationFilter.java`:
- Around line 90-94: The AnonymousAuthenticationToken in
PublicAuthenticationFilter uses a hardcoded key "engine"; replace this with a
single source of truth by introducing either a static constant (e.g.
ANONYMOUS_AUTH_KEY) or a configurable property injected into
PublicAuthenticationFilter and use that value when constructing
AnonymousAuthenticationToken (reference AnonymousAuthenticationToken and
PublicAuthenticationFilter) so the key can be changed centrally and is not
hardcoded.
- Around line 98-100: Authentication failures in PublicAuthenticationFilter are
being logged at debug level; change the catch block that logs "Anonymous public
auth failed for realm {}: {}" (currently using log.debug in
PublicAuthenticationFilter) to use log.warn (or log.error if preferred) so
failures are visible in production, keeping the same message parameters and
passing the Exception object to preserve stacktrace.
- Around line 85-88: The code in PublicAuthenticationFilter unsafely casts the
result of ActorTransformer.toLoggedUser(anonymousUser) to LoggedUserImpl which
can throw ClassCastException; change to store the result in the LoggedUser
interface (e.g., LoggedUser userDetails =
ActorTransformer.toLoggedUser(anonymousUser)) and call setSessionTimeout via the
LoggedUser type if it exists, or add a runtime type check (instanceof) before
casting to LoggedUserImpl and handle the alternative case (log/wrap/create
fallback) if it is not an instance; consider adding setSessionTimeout to the
LoggedUser interface (or a helper setter) so PublicAuthenticationFilter no
longer relies on LoggedUserImpl.
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/security/RealmFilter.java`:
- Around line 71-76: The realmFilterFilterRegistrationBean `@Bean` is declared
inside a `@Component` (RealmFilter) which prevents CGLIB proxying and can cause
unexpected behavior; move the bean method out of the RealmFilter class into a
dedicated `@Configuration` class (or change the containing type to `@Configuration`)
and define the FilterRegistrationBean<RealmFilter> there (keep the method name
realmFilterFilterRegistrationBean and the FilterRegistrationBean
creation/registration logic intact) so Spring treats it as a full
`@Configuration-managed` bean definition.
- Line 32: The constant REALM_NAME_BODY in RealmFilter is likely misspelled
("realName"); update its value to "realmName" so it matches naming conventions
and aligns with REALM_ID_HEADER usage; locate REALM_NAME_BODY in class
RealmFilter and change the string literal accordingly and run tests to ensure
request parsing still works.
- Around line 115-117: The helper isJsonNodeValueEmpty has inverted semantics
(it returns true when a JsonNode has a non-blank value); rename it to reflect
actual behavior (e.g., hasNonEmptyJsonNodeValue or
isJsonNodeValuePresentAndNotBlank) and update all internal calls in RealmFilter
to use the new name (replace occurrences of isJsonNodeValueEmpty(...) in the
conditional checks around the existing usage sites and ensure negation is
removed/adjusted as appropriate); keep the implementation the same, only change
the method name and its call sites so semantics match the identifier.
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java`:
- Around line 669-672: getProcessRolesCriteria fails when user.getProcessRoles()
is empty because Criteria.orOperator() cannot accept an empty array; guard it by
checking user.getProcessRoles().isEmpty() and in that case return a Criteria
that matches nothing (e.g. Criteria.where("_id").exists(false)); otherwise build
the existing stream mapping to
Criteria.where("permissions."+role.getStringId()).exists(true) and pass to new
Criteria().orOperator(...). Ensure you update the method getProcessRolesCriteria
and reference user.getProcessRoles() and the permission key construction
("permissions."+role.getStringId()) when making the change.
In
`@application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java`:
- Around line 162-166: The current ids.forEach(id -> id = decodeUrl(id)) in
getTransitionReferences does not mutate the list so encoded IDs are still
passed; replace that with a proper decoding step (e.g., use ids.replaceAll(id ->
decodeUrl(id)) or create a new List via
ids.stream().map(this::decodeUrl).collect(...)) and then pass the decoded list
into service.getTransitionReferences; update the code around
getTransitionReferences, the ids variable, and the decodeUrl usage accordingly.
In
`@application-engine/src/main/java/com/netgrif/application/engine/utils/HttpReqRespUtils.java`:
- Around line 57-67: The three extractor methods (extractRealmFromRequest,
extractBodyFromRequest, extractAuthReqTokenFromRequest) should guard against a
null request and avoid unchecked casts: first check that request != null,
retrieve Object param = request.getAdditionalParameter(...), then use instanceof
(pattern matching) to safely cast to Realm/JsonNode/NetgrifAuthenticationToken
and return the typed value or null when absent/incorrect type; this prevents
ClassCastException and NPEs while keeping the existing method signatures.
- Line 12: The HttpReqRespUtils class is a pure utility class and should not be
instantiable; add a private no-arg constructor (e.g., private HttpReqRespUtils()
{ throw new AssertionError("No instances."); }) to the HttpReqRespUtils class to
prevent construction and make intent explicit, keeping all existing static
methods unchanged.
In
`@application-engine/src/main/java/com/netgrif/application/engine/utils/HttpRequestParamConstants.java`:
- Around line 3-7: HttpRequestParamConstants is currently instantiable; make it
a proper constants utility by declaring the class final and adding a private
no-args constructor to prevent instantiation and subclassing (keep the existing
public static final String constants REQUEST_BODY, REALM, AUTH_REQ_TOKEN
unchanged); update the class declaration to final and add a private constructor
to enforce the utility pattern.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskAuthorizationService.java`:
- Around line 81-87: The current return in TaskAuthorizationService (inside
isAssigned check) may NPE when unboxing user.getAttributeValue("anonymous");
change the anonymous check to a null-safe Boolean comparison (e.g. use
Boolean.TRUE.equals(user.getAttributeValue("anonymous"))) and keep the existing
task ownership check task.getUserId().equals(user.getStringId()); ensure this
replacement is applied where the code currently returns
task.getUserId().equals(user.getStringId()) || (Boolean)
user.getAttributeValue("anonymous") so it safely handles nulls.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowAuthorizationService.java`:
- Around line 28-31: The code contains multiple TODOs and commented-out
impersonation-related code in WorkflowAuthorizationService (commented blocks
referencing userHasAtLeastOneRolePermission and userHasUserListPermission and
the return using getSelfOrImpersonated()); create a formal tracking issue in
your issue tracker for "implement impersonation support for authorization" and
replace each TODO/commented block with a single-line TODO that references that
issue ID (e.g., TODO: track impersonation work: ISSUE-1234) so the work is
discoverable; update all occurrences mentioned (the blocks around the current
commented code plus the other spots at 40-41, 62-63, 68-69) to reference the
same issue ID. Ensure the TODO text identifies the affected methods/classes
(WorkflowAuthorizationService and the
userHasAtLeastOneRolePermission/userHasUserListPermission usage) so reviewers
can find and complete the task later.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/web/AbstractTaskController.java`:
- Around line 182-189: Remove the dead, commented-out public search block in
AbstractTaskController: delete the entire commented method that defines
searchPublic (the multi-line commented code starting with “public
PagedModel<LocalisedTaskResource> searchPublic(...)” and ending with its closing
brace). This cleans up dead code and noise; ensure no other references to this
stub remain and run a compile to confirm nothing depended on the commented
method.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/web/requestbodies/TaskSearchRequest.java`:
- Around line 15-18: The `@Builder` on TaskSearchRequest currently uses
builderMethodName = "from", causing inconsistency with TaskSearchCaseRequest;
change the annotation to use the conventional builder() entry (remove or set
builderMethodName to "builder") on the TaskSearchRequest class (the
`@Builder`(builderMethodName = "from") declaration), and update all call sites
that use TaskSearchRequest.from() to TaskSearchRequest.builder()...build() so
both request types use the same builder naming.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/web/TaskController.java`:
- Around line 210-274: The deleteFile and deleteNamedFile handlers currently
call super.deleteFile/deleteNamedFile with requestBody.getParentTaskId() instead
of the path variable taskId, allowing mismatched parentTaskId to bypass
authorization; update both methods (deleteFile and deleteNamedFile) to validate
that requestBody.getParentTaskId() equals the `@PathVariable` taskId and if not
throw new ResponseStatusException(HttpStatus.FORBIDDEN) (add imports for
org.springframework.web.server.ResponseStatusException and
org.springframework.http.HttpStatus), or alternatively ignore
requestBody.getParentTaskId() and pass the path taskId into the super calls to
ensure the operation always targets the path taskId.
In
`@application-engine/src/main/java/com/netgrif/application/engine/workflow/web/WorkflowController.java`:
- Around line 244-249: The method getCaseResources currently builds a PagedModel
named resources, calls ResourceLinkAssembler.addLinks(resources, Case.class,
selfLink.getRel().toString()) to enrich it, but then returns a newly constructed
PagedModel from cases.stream() which loses those links; change the return to
return the already prepared resources (from assembler.toModel) so the
link-enriched resources produced by ResourceLinkAssembler are returned by
getCaseResources.
In `@application-engine/src/main/resources/application-dev.yaml`:
- Line 31: The YAML exposes /api/users/me and /api/users/preferences in
server-patterns but those endpoints require authentication; either remove those
two paths from the server-patterns entry so they are protected by default, or
explicitly annotate the controller methods handling these endpoints (e.g., the
methods that serve /api/users/me and /api/users/preferences) with
`@PreAuthorize`("isAuthenticated()") to make intent clear; if you choose to keep
them in server-patterns because an external filter performs auth, add a short
comment in application-dev.yaml documenting that external authentication is
expected for these routes.
In `@application-engine/src/main/resources/application.yaml`:
- Line 88: The server-patterns entry in application.yaml currently includes
/api/users/me and /api/users/preferences which makes them publicly accessible
via anonymous authentication; verify whether this is intentional and either
remove those two patterns from the server-patterns list (so they require normal
authentication) or explicitly document the intended public behavior and add
explicit access checks in the handlers for getUser (or controller handling
/api/users/me) and getUserPreferences (or controller handling
/api/users/preferences) to safe-guard anonymous responses; update
application.yaml and the endpoint documentation accordingly.
In
`@application-engine/src/test/groovy/com/netgrif/application/engine/workflow/TaskControllerTest.groovy`:
- Around line 117-118: Replace the use of Groovy's coercion "as Set<Authority>"
and "as Set<ProcessRole>" with explicit Set constructors to improve clarity and
type safety: construct authoritySet with a new HashSet<Authority> containing the
results of authorityService.getOrCreate(Authority.user) and
authorityService.getOrCreate(Authority.admin) (refer to the authoritySet: line
and authorityService.getOrCreate(...) calls) and construct processRoles with an
explicit new HashSet<ProcessRole>() (or an immutable Collections.emptySet() if
appropriate) so the types are explicit and readability is improved.
- Around line 110-124: Extract the repeated ActorTransformer.toLoggedUser(user)
call into a local variable (e.g., loggedUser) before creating the authentication
token, then use that variable when constructing the
UsernamePasswordAuthenticationToken and when calling getPassword() and
getAuthoritySet(); ensure the variable replaces all three original calls and
keep the SecurityContextHolder.getContext().setAuthentication(token) usage
unchanged.
In `@nae-spring-core-adapter/pom.xml`:
- Around line 92-101: Add explicit <version>${spring.boot.version}</version>
entries to the two new dependencies to match the existing pattern: update the
spring-boot-starter-tomcat and spring-boot-starter-validation dependency
declarations to include a <version>${spring.boot.version}</version> element so
they are consistent with other starters (e.g., spring-boot-starter-security,
spring-boot-starter-data-mongodb).
- Around line 102-107: The org.jetbrains:annotations dependency currently
hardcodes version 23.0.0; add a new parent POM property named
jetbrains.annotations.version with value 26.0.2-1 and update the dependency in
nae-spring-core-adapter to use ${jetbrains.annotations.version} instead of
23.0.0 (locate the dependency block with groupId org.jetbrains and artifactId
annotations and replace the <version> value to reference the new property).
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/AnonymousUser.java`:
- Around line 32-46: The AnonymousUser constructor currently calls new
ObjectId(ref.getId()) which will throw IllegalArgumentException if ref.getId()
is null; update the AnonymousUser(AnonymousUserRef ref) constructor to
defensively handle a null ref.getId() by validating ref and ref.getId() (or
throwing a clear IllegalArgumentException with a descriptive message) before
constructing the ObjectId, or supply a safe default behavior; reference the
constructor AnonymousUser(AnonymousUserRef) and the use of ObjectId to locate
where to add the null check and error handling.
- Around line 54-62: The two-arg constructor AnonymousUser(AnonymousUserRef ref,
Authority anonymousAuthority) redundantly reinitializes authoritySet after
calling this(ref); instead, remove the new HashSet assignment and reuse the
authoritySet populated by the single-arg constructor: call this(ref) as before,
then only if authoritySet is null or empty add anonymousAuthority (or add
ref.getAuthorities() if present), avoiding overwriting the field. Update
AnonymousUser(AnonymousUserRef, Authority) to rely on the single-arg
constructor's initialization of authoritySet and only mutate it when necessary.
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/AnonymousUserRef.java`:
- Line 9: Remove the unused import org.bson.types.ObjectId from the
AnonymousUserRef class; update the imports in AnonymousUserRef.java (or run your
IDE's optimize/organize imports) so only used imports remain.
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/filters/NetgrifOncePerRequestFilter.java`:
- Around line 36-44: The code in NetgrifOncePerRequestFilter casts
ServletRequest to NetgrifHttpServletRequest without a type check which can throw
ClassCastException if NetgrifHttpRequestTransformFilter didn't run; update the
request handling in the method that calls requestNotMatches(...) and
doFilterInternal(...) to first check if request instanceof
NetgrifHttpServletRequest, and if not log a warning with context (include
this.requestMatcher) and call filterChain.doFilter(request, response) (or
alternatively wrap the request with the same transformation used by
NetgrifHttpRequestTransformFilter), ensuring you reference
NetgrifHttpServletRequest, NetgrifOncePerRequestFilter, requestNotMatches,
doFilterInternal and the NetgrifHttpRequestTransformFilter ordering requirement.
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/filters/requests/NetgrifHttpServletRequest.java`:
- Around line 17-20: The constructor
NetgrifHttpServletRequest(HttpServletRequest request, Map<String, Object>
additionalParams) should defensively copy the provided Map instead of storing
the reference: in the NetgrifHttpServletRequest constructor, replace assigning
additionalParams directly with a safe copy (handle null by using
Collections.emptyMap() or new HashMap<>()) and consider wrapping it with
Collections.unmodifiableMap(...) if you want immutability; update any uses of
the field additionalParams accordingly.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/AnonymousUserRefServiceImpl.java`:
- Around line 25-28: getOrCreateRef in AnonymousUserRefServiceImpl currently
calls processRoleService.getAnonymousRole which throws if the role is missing;
add defensive handling by catching that exception (or checking for null) and
invoking a safe "getOrCreate" fallback on processRoleService (or creating the
anonymous role via the service) before saving the AnonymousUserRef, and/or
change injection to a lazy/ObjectProvider pattern so the role lookup is
deferred; additionally add an explicit bean ordering or dependency between
AnonymousUserRefServiceImpl and the runner that creates the role
(AnonymousRoleRunner/DefaultRealmRunner) using `@DependsOn` or documented
initialization requirements and update docs to state that `@RunnerOrder` alone is
not relied upon.
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/DefaultUserFactory.java`:
- Around line 10-11: Remove the unused imports lombok.Setter and
org.springframework.beans.factory.annotation.Autowired from DefaultUserFactory;
the class already uses `@RequiredArgsConstructor` for constructor injection, so
simply delete those import lines to clean up unused imports and avoid
IDE/compiler warnings.
Comment on lines
+25
to
+31
| @Override | ||
| public boolean hasAnyAuthority(String... authority) { | ||
| // TODO: impersonation | ||
| // LoggedUser loggedUser = userService.getLoggedUserFromContext().getSelfOrImpersonated(); | ||
| LoggedUser loggedUser = userService.getLoggedUserFromContext(); | ||
| return loggedUser.getAuthoritySet().stream().anyMatch(it -> Arrays.stream(authority).anyMatch(a -> it.getAuthority().equals(a))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider optimizing authority lookup with a Set.
The current implementation recreates Arrays.stream(authority) for each authority in the user's set, resulting in O(n×m) complexity. Converting the varargs to a Set first would improve this to O(n) with O(1) lookups.
Also, consider renaming the parameter to authorities (plural) for clarity with varargs.
♻️ Proposed optimization
`@Override`
public boolean hasAnyAuthority(String... authority) {
// TODO: impersonation
// LoggedUser loggedUser = userService.getLoggedUserFromContext().getSelfOrImpersonated();
LoggedUser loggedUser = userService.getLoggedUserFromContext();
- return loggedUser.getAuthoritySet().stream().anyMatch(it -> Arrays.stream(authority).anyMatch(a -> it.getAuthority().equals(a)));
+ Set<String> authoritySet = Set.of(authority);
+ return loggedUser.getAuthoritySet().stream().anyMatch(it -> authoritySet.contains(it.getAuthority()));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public boolean hasAnyAuthority(String... authority) { | |
| // TODO: impersonation | |
| // LoggedUser loggedUser = userService.getLoggedUserFromContext().getSelfOrImpersonated(); | |
| LoggedUser loggedUser = userService.getLoggedUserFromContext(); | |
| return loggedUser.getAuthoritySet().stream().anyMatch(it -> Arrays.stream(authority).anyMatch(a -> it.getAuthority().equals(a))); | |
| } | |
| `@Override` | |
| public boolean hasAnyAuthority(String... authority) { | |
| // TODO: impersonation | |
| // LoggedUser loggedUser = userService.getLoggedUserFromContext().getSelfOrImpersonated(); | |
| LoggedUser loggedUser = userService.getLoggedUserFromContext(); | |
| Set<String> authoritySet = Set.of(authority); | |
| return loggedUser.getAuthoritySet().stream().anyMatch(it -> authoritySet.contains(it.getAuthority())); | |
| } |
🤖 Prompt for AI Agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/service/AuthorizationService.java`
around lines 25 - 31, In AuthorizationService.hasAnyAuthority, convert the
incoming varargs (rename parameter from authority to authorities) into a
Set<String> once, then check membership against the logged user's authorities
using a contains lookup to avoid the nested Arrays.stream per-item scan;
specifically, build the Set from the authorities parameter and replace the
Arrays.stream(...).anyMatch(...) logic with a single pass that tests
loggedUser.getAuthoritySet() entries' getAuthority() against the Set (use
getLoggedUserFromContext() as currently used or restore impersonation call if
needed).
Comment on lines
+342
to
+351
| private LoggedUser resolveAuthenticationToken(Authentication auth) { | ||
| if (auth != null) { | ||
| return (LoggedUser) auth.getPrincipal(); | ||
| } | ||
| auth = SecurityContextHolder.getContext().getAuthentication(); | ||
| if (auth != null) { | ||
| return (LoggedUser) auth.getPrincipal(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all calls to resolveAuthenticationToken and their context
rg -n "resolveAuthenticationToken" --type java -B 5 -A 5Repository: netgrif/application-engine
Length of output: 4910
Remove redundant fallback to SecurityContextHolder.
Both usages of resolveAuthenticationToken() are in controller methods (getLoggedUser at line 113 and preferences at line 295) where Authentication is auto-injected by Spring. Based on established patterns in this codebase, the fallback to SecurityContextHolder on lines 346-349 is unnecessary—the injected Authentication parameter will always be populated, and auth.getPrincipal() always returns a valid LoggedUser instance. Simplify the method to remove the defensive fallback logic.
🤖 Prompt for AI Agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/auth/web/UserController.java`
around lines 342 - 351, The method resolveAuthenticationToken currently falls
back to SecurityContextHolder; remove that redundant fallback and simplify it to
just return the principal from the provided Authentication parameter (cast to
LoggedUser) or null if the injected auth is null. Update
resolveAuthenticationToken(Authentication auth) to only check auth and return
(LoggedUser) auth.getPrincipal() when non-null; remove any references to
SecurityContextHolder and its getContext() usage. This keeps callers like
getLoggedUser and preferences relying on the injected Authentication and removes
unnecessary defensive logic.
Comment on lines
422
to
+425
| public static class PublicProperties { | ||
|
|
||
| private boolean enabled = true; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add Javadoc for the enabled property.
Other properties in PublicProperties have Javadoc comments explaining their purpose. Consider adding documentation for the enabled field to maintain consistency.
📝 Suggested documentation
`@Data`
public static class PublicProperties {
+ /**
+ * Enables or disables all public web functionalities.
+ * Default value is true.
+ */
private boolean enabled = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static class PublicProperties { | |
| private boolean enabled = true; | |
| public static class PublicProperties { | |
| /** | |
| * Enables or disables all public web functionalities. | |
| * Default value is true. | |
| */ | |
| private boolean enabled = true; |
🤖 Prompt for AI Agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/SecurityConfigurationProperties.java`
around lines 422 - 425, Add a Javadoc comment for the PublicProperties.enabled
field: describe that it toggles whether the public/anonymous access is enabled
for the application, state the default value (true), and briefly note the effect
(e.g., when false public endpoints/authentication for anonymous users are
disabled). Place the Javadoc immediately above the private boolean enabled =
true; declaration inside the PublicProperties class.
Comment on lines
78
to
+85
| LinkedHashMap<String, Object> userMap = (LinkedHashMap<String, Object>) extractAllClaims(token).get("user"); | ||
|
|
||
| LoggedUser user = new LoggedUserImpl(); | ||
| user.setId(userMap.get("id").toString()); | ||
| user.setUsername(userMap.get("username").toString()); | ||
| user.setPassword(userMap.get("password").toString()); | ||
| user.setId((String) userMap.get("stringId")); | ||
| user.setUsername((String) userMap.get("username")); | ||
| user.setFirstName((String) userMap.get("firstName")); | ||
| user.setMiddleName((String) userMap.get("middleName")); | ||
| user.setLastName((String) userMap.get("lastName")); |
There was a problem hiding this comment.
Guard against older tokens missing the new stringId claim.
Switching from id to stringId will set user.id to null for JWTs minted before this change. If older tokens can still be presented after upgrade, this can break auth downstream. Consider a fallback or ensure token invalidation on release.
✅ Backward-compatible fallback
- user.setId((String) userMap.get("stringId"));
+ Object id = userMap.get("stringId");
+ if (id == null) {
+ id = userMap.get("id");
+ }
+ user.setId((String) id);🤖 Prompt for AI Agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/security/jwt/JwtService.java`
around lines 78 - 85, The JWT parsing code in JwtService uses
userMap.get("stringId") which will be null for older tokens; update the
LoggedUser population (in the block using LinkedHashMap<String,Object> userMap
and LoggedUser/LoggedUserImpl) to fall back to userMap.get("id") when "stringId"
is missing (convert to String if necessary) before calling user.setId(...), so
old tokens still populate user.id; keep the rest of fields the same.
Comment on lines
+22
to
+24
| public PrivateKey get(Resource resource) throws IOException, NoSuchAlgorithmException, InvalidKeySpecException { | ||
| FileInputStream fileInputStream = new FileInputStream(resource.getFile()); | ||
| byte[] keyBytes = IOUtils.toByteArray(fileInputStream); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PrivateKeyReader.java" -type fRepository: netgrif/application-engine
Length of output: 184
🏁 Script executed:
cat -n ./application-engine/src/main/java/com/netgrif/application/engine/configuration/security/jwt/PrivateKeyReader.javaRepository: netgrif/application-engine
Length of output: 1298
🏁 Script executed:
web_search
Spring Resource interface getInputStream vs getFile JavaDoc documentationRepository: netgrif/application-engine
Length of output: 165
🌐 Web query:
Spring Resource interface getInputStream vs getFile behavior classpath resources
💡 Result:
In Spring’s org.springframework.core.io.Resource abstraction:
-
getInputStream()is the portable way to read a resource’s bytes. It works for typical classpath resources, including those packaged inside a JAR/WAR, because Spring can open a stream from the underlying URL/handler. ([Resource Javadoc][1]) -
getFile()is not portable. It only works when the resource can be resolved to a realjava.io.Filein the default filesystem. For classpath resources inside an archive (e.g.,jar:file:...!/path), there is no correspondingFile, sogetFile()will typically fail (commonly withFileNotFoundException, and sometimesUnsupportedOperationExceptiondepending on the implementation). ([Resource Javadoc][1], [ClassPathResource Javadoc][2])
Practical guidance:
- Prefer
resource.getInputStream()(orresource.getContentAsByteArray()/getContentAsString(...)) for classpath resources. ([1]) - If you think it’s a filesystem resource, check
resource.isFile()first—truestrongly suggestsgetFile()may succeed. ([1]) - If an API requires a
Filebut you have a classpath resource in a JAR, you generally must copy the stream to a temp file yourself.
Sources
- Spring
ResourceJavadoc (spring-core 6.2.11) [1] - Spring
ClassPathResourceJavadoc (notesgetFile()assumption / failure) [2]
🏁 Script executed:
rg -t java "IOUtils.toByteArray" --max-count=5 -B 2 -A 2Repository: netgrif/application-engine
Length of output: 1017
Use Resource#getInputStream() with try‑with‑resources.
Resource#getFile() fails for classpath/JAR resources and the FileInputStream is never closed, risking file descriptor leaks. Use resource.getInputStream() with try‑with‑resources instead.
🔧 Proposed fix
public PrivateKey get(Resource resource) throws IOException, NoSuchAlgorithmException, InvalidKeySpecException {
- FileInputStream fileInputStream = new FileInputStream(resource.getFile());
- byte[] keyBytes = IOUtils.toByteArray(fileInputStream);
+ try (var in = resource.getInputStream()) {
+ byte[] keyBytes = IOUtils.toByteArray(in);
PKCS8EncodedKeySpec spec = new PKCS8EncodedKeySpec(keyBytes);
return keyFactory.generatePrivate(spec);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public PrivateKey get(Resource resource) throws IOException, NoSuchAlgorithmException, InvalidKeySpecException { | |
| FileInputStream fileInputStream = new FileInputStream(resource.getFile()); | |
| byte[] keyBytes = IOUtils.toByteArray(fileInputStream); | |
| public PrivateKey get(Resource resource) throws IOException, NoSuchAlgorithmException, InvalidKeySpecException { | |
| try (var in = resource.getInputStream()) { | |
| byte[] keyBytes = IOUtils.toByteArray(in); | |
| PKCS8EncodedKeySpec spec = new PKCS8EncodedKeySpec(keyBytes); | |
| return keyFactory.generatePrivate(spec); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@application-engine/src/main/java/com/netgrif/application/engine/configuration/security/jwt/PrivateKeyReader.java`
around lines 22 - 24, The get method in PrivateKeyReader currently opens a
FileInputStream from resource.getFile() and doesn't close it; change it to call
resource.getInputStream() and read the bytes inside a try-with-resources block
so the InputStream is always closed; update the logic in PrivateKeyReader.get
(and any local variables like fileInputStream/keyBytes) to use the InputStream
from resource.getInputStream() and IOUtils.toByteArray(inputStream) inside the
try-with-resources, preserving the same exceptions (IOException,
NoSuchAlgorithmException, InvalidKeySpecException).
| import com.netgrif.application.engine.objects.petrinet.domain.roles.ProcessRole; | ||
| import lombok.Data; | ||
| import org.bson.codecs.pojo.annotations.BsonIgnore; | ||
| import org.bson.types.ObjectId; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import.
The ObjectId import is not used in this file. Consider removing it.
-import org.bson.types.ObjectId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import org.bson.types.ObjectId; |
🤖 Prompt for AI Agents
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/auth/domain/AnonymousUserRef.java`
at line 9, Remove the unused import org.bson.types.ObjectId from the
AnonymousUserRef class; update the imports in AnonymousUserRef.java (or run your
IDE's optimize/organize imports) so only used imports remain.
Comment on lines
+36
to
+44
| if (requestNotMatches(request)) { | ||
| log.trace("Request did not match the required URIs: {}", this.requestMatcher); | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } | ||
|
|
||
| NetgrifHttpServletRequest typedRequest = (NetgrifHttpServletRequest) request; | ||
| doFilterInternal(typedRequest, response, filterChain); | ||
| } |
There was a problem hiding this comment.
Unsafe cast to NetgrifHttpServletRequest without type check.
Line 42 performs a direct cast assuming the request has already been wrapped by NetgrifHttpRequestTransformFilter. If the filter ordering is misconfigured or the transform filter is disabled/bypassed, this will throw a ClassCastException at runtime.
Consider adding a defensive check or documenting the filter ordering requirement.
🛡️ Proposed defensive check
`@Override`
protected void doFilterInternal(`@NotNull` HttpServletRequest request,
`@NotNull` HttpServletResponse response,
`@NotNull` FilterChain filterChain) throws ServletException, IOException {
if (requestNotMatches(request)) {
log.trace("Request did not match the required URIs: {}", this.requestMatcher);
filterChain.doFilter(request, response);
return;
}
- NetgrifHttpServletRequest typedRequest = (NetgrifHttpServletRequest) request;
+ if (!(request instanceof NetgrifHttpServletRequest typedRequest)) {
+ log.warn("Request is not a NetgrifHttpServletRequest. Ensure NetgrifHttpRequestTransformFilter runs before this filter.");
+ filterChain.doFilter(request, response);
+ return;
+ }
doFilterInternal(typedRequest, response, filterChain);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (requestNotMatches(request)) { | |
| log.trace("Request did not match the required URIs: {}", this.requestMatcher); | |
| filterChain.doFilter(request, response); | |
| return; | |
| } | |
| NetgrifHttpServletRequest typedRequest = (NetgrifHttpServletRequest) request; | |
| doFilterInternal(typedRequest, response, filterChain); | |
| } | |
| if (requestNotMatches(request)) { | |
| log.trace("Request did not match the required URIs: {}", this.requestMatcher); | |
| filterChain.doFilter(request, response); | |
| return; | |
| } | |
| if (!(request instanceof NetgrifHttpServletRequest typedRequest)) { | |
| log.warn("Request is not a NetgrifHttpServletRequest. Ensure NetgrifHttpRequestTransformFilter runs before this filter."); | |
| filterChain.doFilter(request, response); | |
| return; | |
| } | |
| doFilterInternal(typedRequest, response, filterChain); | |
| } |
🤖 Prompt for AI Agents
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/filters/NetgrifOncePerRequestFilter.java`
around lines 36 - 44, The code in NetgrifOncePerRequestFilter casts
ServletRequest to NetgrifHttpServletRequest without a type check which can throw
ClassCastException if NetgrifHttpRequestTransformFilter didn't run; update the
request handling in the method that calls requestNotMatches(...) and
doFilterInternal(...) to first check if request instanceof
NetgrifHttpServletRequest, and if not log a warning with context (include
this.requestMatcher) and call filterChain.doFilter(request, response) (or
alternatively wrap the request with the same transformation used by
NetgrifHttpRequestTransformFilter), ensuring you reference
NetgrifHttpServletRequest, NetgrifOncePerRequestFilter, requestNotMatches,
doFilterInternal and the NetgrifHttpRequestTransformFilter ordering requirement.
Comment on lines
+17
to
+20
| public NetgrifHttpServletRequest(HttpServletRequest request, Map<String, Object> additionalParams) { | ||
| super(request); | ||
| this.additionalParams = additionalParams; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defensive copy of additionalParams Map.
The constructor stores the provided Map reference directly. If the caller modifies the Map after constructing the wrapper, it will affect the wrapper's state. Consider creating a defensive copy.
♻️ Proposed fix
public NetgrifHttpServletRequest(HttpServletRequest request, Map<String, Object> additionalParams) {
super(request);
- this.additionalParams = additionalParams;
+ this.additionalParams = new HashMap<>(additionalParams);
}🤖 Prompt for AI Agents
In
`@nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/configuration/filters/requests/NetgrifHttpServletRequest.java`
around lines 17 - 20, The constructor
NetgrifHttpServletRequest(HttpServletRequest request, Map<String, Object>
additionalParams) should defensively copy the provided Map instead of storing
the reference: in the NetgrifHttpServletRequest constructor, replace assigning
additionalParams directly with a safe copy (handle null by using
Collections.emptyMap() or new HashMap<>()) and consider wrapping it with
Collections.unmodifiableMap(...) if you want immutability; update any uses of
the field additionalParams accordingly.
Comment on lines
25
to
28
| public AnonymousUserRef getOrCreateRef(String realmId) { | ||
| return repository.findByRealmId(realmId) | ||
| .orElseGet(() -> repository.save(new AnonymousUserRef(realmId))); | ||
| .orElseGet(() -> repository.save(new AnonymousUserRef(realmId, Set.of(authorityService.getOrCreate(Authority.anonymous)), Set.of(processRoleService.getAnonymousRole())))); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'ANONYMOUS_ROLE|anonymousRole|getAnonymousRole' --type java -A 3 -B 3 | head -100Repository: netgrif/application-engine
Length of output: 13681
🏁 Script executed:
# Find the processRoleService implementation and the getAnonymousRole method
rg -n 'class.*ProcessRoleService|def.*getAnonymousRole|public.*getAnonymousRole' --type java -A 10 | head -150Repository: netgrif/application-engine
Length of output: 5113
🏁 Script executed:
# Search for where AnonymousUserRef or getOrCreateRef is used/called
rg -n 'AnonymousUserRef|getOrCreateRef|AnonymousUserRefService' --type java -B 2 -A 2 | head -100Repository: netgrif/application-engine
Length of output: 12595
🏁 Script executed:
# Look for initialization order, `@Component`, `@Bean`, `@PostConstruct`, etc.
rg -n '@PostConstruct|@Component|@Service|InitializingBean|ApplicationRunner' --type java -A 5 | grep -E 'class|@PostConstruct|@Component|@Service|InitializingBean|ApplicationRunner' | head -80Repository: netgrif/application-engine
Length of output: 11764
🏁 Script executed:
# Check if AnonymousUserRefService is used anywhere in `@PostConstruct` or bean initialization
rg -n '@PostConstruct' --type java -A 10 | grep -E '@PostConstruct|enableAnonymUser|getOrCreateRef|getAnonymousRole' | head -50Repository: netgrif/application-engine
Length of output: 756
🏁 Script executed:
# Look at the full AnonymousRoleRunner implementation to understand timing
cat -n application-engine/src/main/java/com/netgrif/application/engine/startup/runner/AnonymousRoleRunner.javaRepository: netgrif/application-engine
Length of output: 2328
🏁 Script executed:
# Check if AnonymousUserRefServiceImpl has any initialization methods
cat -n nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/AnonymousUserRefServiceImpl.javaRepository: netgrif/application-engine
Length of output: 1757
🏁 Script executed:
# Check if RealmServiceImpl.enableAnonymUser is called during startup
rg -n 'enableAnonymUser' --type java -B 2 -A 2Repository: netgrif/application-engine
Length of output: 2056
🏁 Script executed:
# Check the runner order of DefaultRealmRunner and other runners
rg -n '@RunnerOrder' --type java -B 2 -A 1 | head -80Repository: netgrif/application-engine
Length of output: 7766
🏁 Script executed:
# Check if there are any `@PostConstruct` methods in beans that might call getOrCreateRef
rg -n 'getOrCreateRef|enableAnonymUser' --type java -B 5 | grep -E '@PostConstruct|@Bean|@Service|@Component|getOrCreateRef|enableAnonymUser' | head -60Repository: netgrif/application-engine
Length of output: 989
🏁 Script executed:
# Find DefaultRealmRunner's RunnerOrder
rg -n 'class DefaultRealmRunner' --type java -B 5 -A 20 | grep -E '@RunnerOrder|class DefaultRealmRunner'Repository: netgrif/application-engine
Length of output: 374
🏁 Script executed:
# Get the full DefaultRealmRunner to see its order
cat -n application-engine/src/main/java/com/netgrif/application/engine/startup/runner/DefaultRealmRunner.java | head -50Repository: netgrif/application-engine
Length of output: 1985
🏁 Script executed:
# Check if there's any documentation or guarantee about runner order execution
rg -n 'RunnerOrder|ApplicationEngineStartupRunner' --type java nae-user-ce nae-user-common -A 2 | head -40Repository: netgrif/application-engine
Length of output: 52
🏁 Script executed:
# See if there are any integration tests that verify initialization order
find . -name "*Test*.java" -type f | xargs rg -l 'AnonymousRoleRunner\|DefaultRealmRunner\|getOrCreateRef' | head -5Repository: netgrif/application-engine
Length of output: 52
Add defensive handling or document initialization order guarantee.
processRoleService.getAnonymousRole() throws IllegalStateException if the anonymous role doesn't exist. While AnonymousRoleRunner (order 50) currently runs before DefaultRealmRunner (order 69) which calls this method, the initialization order is implicit and relies on the @RunnerOrder annotation rather than Spring's dependency system. If the runner order changes, execution is skipped, or the database state becomes corrupted, getOrCreateRef() will fail at startup.
Consider adding:
- Defensive null-handling with lazy initialization fallback
- Explicit Spring bean ordering (
@DependsorObjectProviderinjection pattern) - Documentation of the initialization sequence requirement
🤖 Prompt for AI Agents
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/AnonymousUserRefServiceImpl.java`
around lines 25 - 28, getOrCreateRef in AnonymousUserRefServiceImpl currently
calls processRoleService.getAnonymousRole which throws if the role is missing;
add defensive handling by catching that exception (or checking for null) and
invoking a safe "getOrCreate" fallback on processRoleService (or creating the
anonymous role via the service) before saving the AnonymousUserRef, and/or
change injection to a lazy/ObjectProvider pattern so the role lookup is
deferred; additionally add an explicit bean ordering or dependency between
AnonymousUserRefServiceImpl and the runner that creates the role
(AnonymousRoleRunner/DefaultRealmRunner) using `@DependsOn` or documented
initialization requirements and update docs to state that `@RunnerOrder` alone is
not relied upon.
Comment on lines
+10
to
+11
| import lombok.Setter; | ||
| import org.springframework.beans.factory.annotation.Autowired; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused imports.
@Setter and @Autowired are imported but not used in this class. Constructor injection is handled via @RequiredArgsConstructor.
🧹 Proposed cleanup
import com.netgrif.application.engine.objects.auth.domain.User;
import lombok.RequiredArgsConstructor;
-import lombok.Setter;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;🤖 Prompt for AI Agents
In
`@nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/DefaultUserFactory.java`
around lines 10 - 11, Remove the unused imports lombok.Setter and
org.springframework.beans.factory.annotation.Autowired from DefaultUserFactory;
the class already uses `@RequiredArgsConstructor` for constructor injection, so
simply delete those import lines to clean up unused imports and avoid
IDE/compiler warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactor and reworks anonymous access to more common use concept, update the management of public endpoints, public resources and so.
Implements NAE-2241
Dependencies
No new dependencies were introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
There are no dependencies on other PR.
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
API Changes
/publicpaths in main controllers