Conversation
Introduced a domain model for describing and validating API endpoints, including their structure, allowed methods, and access control rules. Added EndpointDescriptor as a central immutable representation of endpoint metadata, including identifier, resource, allowed HTTP methods, required user groups, and mutability flag. Defensive copying ensures immutability of collections. Defined supporting enums: – EndpointId for unique endpoint identification – Method for HTTP methods – UserGroup for role-based access control – Resource for endpoint paths grouped by domain areas Implemented RequestTarget enum as a registry of all endpoints, binding together endpoint metadata and access rules. Each entry defines allowed methods, required roles, and whether the endpoint is mutating. Added logic to evaluate access permissions via allow(method, userGroups), including validation of HTTP method and user authorization. Public endpoints are handled via empty required group sets. This structure centralizes endpoint configuration, improves maintainability, and provides a consistent mechanism for access control and request validation across the application.
This commit introduces the foundational models and utilities for handling authenticated requests within the application. It adds Request and Authorization records to the domain model, alongside an AuthorizationHolder that uses ThreadLocal to manage security context per thread. Additionally, it defines RequestFactory and RequestHandler interfaces to standardize how requests are created and processed. These changes establish a consistent pattern for passing authorization data and payloads through the system's core layers.
This commit updates the project's groupId to `pl.vtt` and manages build dependencies by adding the maven-compiler-plugin version while removing the unused surefire property. The Authorization model and holder were refactored to use `credentials` instead of `value` , and a `clear()` method was introduced to support a new default `logout()` implementation in `LoginService`. Additionally, several utility and domain packages were added to the module-info exports to broaden API accessibility.
📝 WalkthroughWalkthroughAdds authentication domain types, a thread-local Authorization holder, request factory/handler abstractions, a LoginService implementation with login/logout, an endpoint/resource access model (RequestTarget, Resource, EndpointDescriptor, enums), module export updates, and Maven POM adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LoginServiceImpl as Login
participant RequestFactory as Factory
participant RequestHandler as Handler
participant AuthorizationHolder as Auth
Client->>Login: login(username, password)
Login->>Login: validate inputs
Login->>Auth: authorize("Basic", base64(username:password))
Login->>Factory: create(RequestTarget.AUTH, null)
Factory-->>Login: Request<Void>
Login->>Handler: handle(request)
Handler-->>Login: Credentials(username, token)
alt credentials present
Login->>Auth: authorize("Basic", base64(username:token))
Login-->>Client: return Credentials
else null
Login->>Auth: clear()
Login-->>Client: throw IncorrectUsernameOrPasswordException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pom.xml (1)
31-35: Consolidate compiler configuration using thereleaseproperty to avoid config drift.You already declare
maven.compiler.releaseat line 13, but the plugin also specifies separatesourceandtargetconfiguration. Usereleasein the plugin configuration and remove the duplicate settings.Suggested fix
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> <version>3.15.0</version> <configuration> - <source>25</source> - <target>25</target> + <release>${maven.compiler.release}</release> </configuration> </plugin>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 31 - 35, Replace the explicit <source> and <target> entries in the maven-compiler-plugin configuration with a single <release> element that uses the existing maven.compiler.release property; in other words, inside the plugin (the block containing <version>3.15.0</version>) remove <source>25</source> and <target>25</target> and add <release>${maven.compiler.release}</release> so the plugin honors the declared property and avoids duplicate compiler settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java`:
- Around line 28-31: The code sets thread-local Basic auth via
authorize(username, password) then calls getCredentials(); if getCredentials()
throws the thread-local remains set — update LoginServiceImpl to clear the
thread-local on failure by surrounding the
getCredentials()/authorize(credentials) sequence with try/catch/finally and call
the AuthorizationHolder clear/reset method (the same mechanism used to store
auth) in the catch or finally path so that if getCredentials() throws the
pre-set basic auth is removed before rethrowing; use the existing
authorize(username, password), getCredentials(), authorize(credentials) and
AuthorizationHolder APIs to locate where to add the cleanup.
- Around line 58-60: Replace the platform-default charset encoding in the encode
helper with explicit UTF-8 and use Base64.encodeToString to avoid extra
byte-array allocation: change the LoginServiceImpl.encode method to call
Base64.getEncoder().encodeToString(string.getBytes(StandardCharsets.UTF_8)).
Also update the analogous usage in LoginServiceImplTest (the test at/around line
42) to use the same StandardCharsets.UTF_8 + encodeToString approach so both
production and test code produce RFC‑7617-compliant Basic auth strings.
In `@src/main/java/pl/vtt/wpi/core/domain/model/Authorization.java`:
- Around line 3-4: The record Authorization(String type, String credentials)
currently uses the auto-generated toString() which prints credentials; override
Authorization::toString in the record body to avoid leaking sensitive data by
either omitting the credentials or replacing them with a redacted/masked value
(e.g., "<redacted>" or a fixed mask), while still including non-sensitive fields
like type; update any equals/hashCode only if you change semantics (not
required) and search for usages that rely on the old toString to ensure
logs/diagnostics now use the safe representation.
In `@src/main/java/pl/vtt/wpi/core/domain/model/endpoint/RequestTarget.java`:
- Around line 62-68: The access check crashes when userGroups is null; update
the logic so secured endpoints return false (deny) instead of throwing: either
make rejected(Set<UserGroup> groups) null-safe by returning true when groups is
null (e.g., if (groups == null) return true) or add a null check in allow(Method
method, Set<UserGroup> userGroups) before calling rejected() (e.g., if
(secured() && (userGroups == null || rejected(userGroups))) return false);
reference the rejected(...) method, allow(..., Set<UserGroup> userGroups), and
descriptor().requiredAnyGroups() when applying the change.
In `@src/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java`:
- Around line 35-39: Resource.url currently interpolates user-derived values
into the path without encoding, allowing malicious or malformed values to break
routing; update the url(String baseUrl, Object... args) implementation in class
Resource to percent-encode each path segment argument before calling formatted
on the path (i.e., encode the args used for path substitution rather than raw
values), and then concatenate baseUrl + path as before so encoded segments are
used when building the URL; ensure encoding is applied to all Object... args
(convert to string then encode) and preserves existing behavior when baseUrl is
null.
In
`@src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java`:
- Around line 101-130: The two tests null_username_exception and
null_password_exception currently use empty strings instead of nulls, so change
the local variables in each test to use null (username = null in
null_username_exception, password = null in null_password_exception) and keep
the rest of the setup (LoginServiceImpl constructor lambdas and assertThrows on
login) so the login method is exercised on actual null inputs; reference
LoginServiceImpl.login and the test methods
null_username_exception/null_password_exception when making the change.
---
Nitpick comments:
In `@pom.xml`:
- Around line 31-35: Replace the explicit <source> and <target> entries in the
maven-compiler-plugin configuration with a single <release> element that uses
the existing maven.compiler.release property; in other words, inside the plugin
(the block containing <version>3.15.0</version>) remove <source>25</source> and
<target>25</target> and add <release>${maven.compiler.release}</release> so the
plugin honors the declared property and avoids duplicate compiler settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d02161d8-55d0-4c15-b8e1-9bee8cf3e9c4
📒 Files selected for processing (15)
pom.xmlsrc/main/java/module-info.javasrc/main/java/pl/vtt/wpi/core/application/config/AuthorizationHolder.javasrc/main/java/pl/vtt/wpi/core/application/service/LoginService.javasrc/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/application/util/RequestFactory.javasrc/main/java/pl/vtt/wpi/core/application/util/RequestHandler.javasrc/main/java/pl/vtt/wpi/core/domain/model/Authorization.javasrc/main/java/pl/vtt/wpi/core/domain/model/Request.javasrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/EndpointDescriptor.javasrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/Method.javasrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/RequestTarget.javasrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.javasrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/UserGroup.javasrc/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java
src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java
Show resolved
Hide resolved
src/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java
Outdated
Show resolved
Hide resolved
src/test/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImplTest.java
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This commit strengthens the application's stability and security by implementing robust null safety across core components. The Authorization record now enforces default values for type and credentials and includes a custom toString() method to prevent leaking sensitive credentials in logs. In RequestTarget, a safety check was added to handle null user groups. Finally, LoginServiceImplTest was updated to properly use null values instead of empty strings, ensuring validation logic is correctly tested against null inputs.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java (1)
39-46:⚠️ Potential issue | 🟠 MajorUse
encodedArgsin the path formatting call.
encodedArgsis computed but not used; Line [45] still formats with rawargs, so path parameters remain unencoded.Suggested fix
- String formatted = path.formatted(args); + String formatted = path.formatted(encodedArgs);Based on learnings: classes from
java.base(likeURLEncoder,Arrays,StandardCharsets) are valid here without addingrequiresinmodule-info.java.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java` around lines 39 - 46, In Resource.url, encodedArgs is computed but not used; change the path formatting to use the encodedArgs array instead of the raw args so path parameters are URL-encoded (i.e., call path.formatted(...) with encodedArgs), ensuring the encodedArgs produced by Arrays.stream(...).toArray() are passed to formatted rather than args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java`:
- Around line 39-46: In Resource.url, encodedArgs is computed but not used;
change the path formatting to use the encodedArgs array instead of the raw args
so path parameters are URL-encoded (i.e., call path.formatted(...) with
encodedArgs), ensuring the encodedArgs produced by Arrays.stream(...).toArray()
are passed to formatted rather than args.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84fb2dfd-ec0b-4de7-96ef-1f728684f207
📒 Files selected for processing (2)
src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java
This commit improves data handling by ensuring consistent UTF-8 encoding across the application. In `LoginServiceImpl`, the Base64 encoding logic was updated to use `StandardCharsets.UTF_8` and the more efficient `encodeToString` method. Additionally, the `Resource` enum was enhanced to automatically URL-encode arguments passed to the `url()` method. This change prevents issues with special characters in generated paths and replaces "+" signs with `%20` for better compatibility. The logic now correctly handles base URL concatenation after argument formatting.
koder95
force-pushed
the
login-service-impl
branch
from
273083f to
583f9c8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java (1)
60-62: Consider renaming parameter for clarity.The parameter
credentialsis used for both the password (during initial login) and the token (after credential retrieval). A more descriptive name likesecretwould clarify its dual purpose.♻️ Optional naming improvement
- private static void authorize(String username, String credentials) { - AuthorizationHolder.authorize("Basic", encode(String.join(":", username, credentials))); + private static void authorize(String username, String secret) { + AuthorizationHolder.authorize("Basic", encode(String.join(":", username, secret))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java` around lines 60 - 62, Rename the parameter in LoginServiceImpl.authorize from credentials to a clearer name like secret to reflect it can be either a password or a token; update the method signature private static void authorize(String username, String secret) and replace the usages inside the method (AuthorizationHolder.authorize and encode calls) accordingly, then update all call sites of LoginServiceImpl.authorize to pass the renamed parameter and adjust any related javadoc/comments to use "secret" instead of "credentials".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.java`:
- Around line 60-62: Rename the parameter in LoginServiceImpl.authorize from
credentials to a clearer name like secret to reflect it can be either a password
or a token; update the method signature private static void authorize(String
username, String secret) and replace the usages inside the method
(AuthorizationHolder.authorize and encode calls) accordingly, then update all
call sites of LoginServiceImpl.authorize to pass the renamed parameter and
adjust any related javadoc/comments to use "secret" instead of "credentials".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e05a544-ee40-40aa-adf2-2409eea3a5c0
📒 Files selected for processing (3)
pom.xmlsrc/main/java/pl/vtt/wpi/core/application/service/impl/LoginServiceImpl.javasrc/main/java/pl/vtt/wpi/core/domain/model/endpoint/Resource.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
This was referenced Apr 10, 2026
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.
Summary by CodeRabbit
New Features
Tests
Chores