feat: lock gprc enpoints thought grpc bridge #12949
Conversation
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
📝 WalkthroughWalkthroughAdds gRPC audit support and bridge: protobuf definitions, gRPC server and servlet bridge, authorization interceptor, header/IP utilities, model/schema enrichments, REST endpoints now accept typed audit payloads, Jetty/Apache http2c proxy support, build/plugin updates, and refactors authorization interfaces to use bearerToken strings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mo-auto
added
comp-docs
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
…to jans-auth_old_deps
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
final methods Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
HTTP/2 standarts Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
jans-lock context path Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
|
|
|
|
|
|
|
Signed-off-by: Mustafa Baser <mbaser@mail.com>
|
yurem
requested review from
devrimyatar,
iromli,
pujavs,
yuriyz and
yuriyzz
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/inject/CedarlingPolicy.java (1)
19-26: Consider enhancing the javadoc with usage description.The javadoc currently only includes the author and date. Adding a brief description of the annotation's purpose and usage guidelines would improve code maintainability and help developers understand when to apply
@CedarlingPolicy.📝 Example javadoc enhancement
/** - * + * Qualifier annotation for CDI injection of Cedarling policy components. + * Apply to injection points that require policy-specific bean selection. + * * `@author` Yuriy Movchan Date: 10/08/2022 */jans-core/service/src/main/java/io/jans/service/security/api/ProtectedApi.java (1)
15-20: DocumentgrpcMethodNamecontract.This is part of the public API; please add a brief Javadoc indicating expected format (e.g., fully qualified service/method name) and when it should be empty.
✍️ Suggested Javadoc
/** * `@return` Scopes access token should have to access this endpoint */ String[] scopes() default {}; + /** + * `@return` gRPC method name mapped to this endpoint (empty if not applicable) + */ String grpcMethodName() default "";jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/CedarlingProtectionService.java (1)
164-185: Dead code:idvariable is computed but never used.The
idvariable at lines 166-167 is calculated but never referenced in the resulting map. If it's intended for the entity mapping, it should be used; otherwise, remove the dead code.Proposed fix (remove dead code)
private Map<String, Object> getCedarlingResource(CedarlingPermission requestedPermission) { HashMap<String, Object> map = new HashMap<String, Object>(); - int id = requestedPermission.hashCode(); - id = id > 0 ? id : -id; map.putAll(jans-lock/lock-server/service/src/main/java/io/jans/lock/service/openid/OpenIdProtectionService.java (1)
41-41: Unused import.The
HttpHeadersimport appears unused after the refactoring to acceptbearerTokendirectly. Consider removing it.Proposed fix
-import jakarta.ws.rs.core.HttpHeaders;jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/audit/AuditRestWebService.java (1)
72-88: Align Cedarlingpathwith bulk endpoints to prevent authorization failures.The bulk endpoints at
/audit/health/bulk,/audit/log/bulk, and/audit/telemetry/bulkeach declare a non-bulk path in their@ProtectedCedarlingApiannotations. Since this path is passed directly to the Cedarling authorization engine (inCedarlingProtectionService.getCedarlingResource()), path-based authorization policies will check against the wrong route, causing authorization failures or unintended access.Update the annotation paths to match the actual endpoint routes:
🔧 Suggested fix
- `@ProtectedCedarlingApi`(action = "Jans::Action::\"POST\"", resource = "Jans::HTTP_Request", id="lock_audit_health_write", path="/audit/health") + `@ProtectedCedarlingApi`(action = "Jans::Action::\"POST\"", resource = "Jans::HTTP_Request", id="lock_audit_health_write", path="/audit/health/bulk")- `@ProtectedCedarlingApi`(action = "Jans::Action::\"POST\"", resource = "Jans::HTTP_Request", id="lock_audit_log_write", path="/audit/log") + `@ProtectedCedarlingApi`(action = "Jans::Action::\"POST\"", resource = "Jans::HTTP_Request", id="lock_audit_log_write", path="/audit/log/bulk")- `@ProtectedCedarlingApi`(action = "Jans::Action::\"POST\"", resource = "Jans::HTTP_Request", id="lock_audit_telemetry_write", path="/audit/telemetry") + `@ProtectedCedarlingApi`(action = "Jans::Action::\"POST\"", resource = "Jans::HTTP_Request", id="lock_audit_telemetry_write", path="/audit/telemetry/bulk")Applies to: lines 72-88, 108-124, 144-160.
jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/audit/AuditRestWebServiceImpl.java (1)
200-212: Statistics reported before persistence confirmation.Statistics are reported in lines 203-205 before the bulk persistence operation (line 206). If persistence fails, statistics will still reflect the entries. Depending on requirements, you may want to report stats only after successful persistence, or this may be acceptable for tracking activity regardless of storage outcome.
jans-config-api/plugins/docs/lock-plugin-swagger.yaml (3)
24-26: Typo: "Rerquest" should be "Request".The summary and description contain "Rerquest" instead of "Request". Based on learnings, this Swagger file is auto-generated, so the fix should be applied in the source API code.
77-78: Typo: "Rerquest" should be "Request".Same typo appears in the log records endpoint. Fix in the source API code.
130-132: Inconsistent summary/description: "Request" vs "Rerquest".Line 130 correctly uses "Request" in the summary, but line 131 uses "Rerquest" in the description. Fix the typo in the source API code for consistency.
🤖 Fix all issues with AI agents
In
`@jans-core/service/src/main/java/io/jans/service/security/protect/BaseAuthorizationProtection.java`:
- Around line 12-13: Update the BaseAuthorizationProtection.processAuthorization
contract to explicitly document the expected bearerToken format and null/empty
handling: state whether bearerToken must be the raw access token (no "Bearer "
prefix) or may include the "Bearer " prefix and that implementations must strip
the prefix if present, and specify how null or empty values are treated (e.g.,
return a 401/unauthorized Response or throw a defined exception). Add this
Javadoc to the interface method processAuthorization(String bearerToken,
ResourceInfo resourceInfo) and ensure any implementations of
BaseAuthorizationProtection follow the documented behavior (strip prefix if
allowed, validate null/empty and return the documented Response).
In `@jans-linux-setup/jans_setup/schema/jans_schema.json`:
- Around line 4081-4085: The schema rename removed backward compatibility for
the attribute; restore it by adding the legacy name "severetyLevel" as an alias
(or include it in the "names" array) alongside the new "severityLevel" entry in
jans_schema.json so both names map to the same attribute definition (or,
alternatively, implement and reference an explicit migration step to transform
existing "severetyLevel" data to "severityLevel"); update the attribute
definition that currently lists "severityLevel" to include "severetyLevel" as
well so existing deployments continue to work.
In `@jans-linux-setup/jans_setup/setup_app/data/jetty_app_configuration.json`:
- Line 83: The jetty modules list currently enables cleartext HTTP/2 via
"http2c", exposing gRPC audit traffic; update configuration so http2c is not
enabled by default (remove or conditionally add "http2c" based on a new flag) OR
enable TLS by default in the GrpcConfiguration by setting useTls = true and
populating tlsCertChainFilePath and tlsPrivateKeyFilePath (and any related TLS
options) so BRIDGE-mode gRPC traffic is encrypted; reference the "modules" entry
(remove/guard "http2c") and the GrpcConfiguration fields useTls,
tlsCertChainFilePath to locate where to change settings and ensure documentation
notes the requirement if you choose to keep http2c behind an explicit opt-in.
In `@jans-linux-setup/jans_setup/setup_app/installers/jans_auth.py`:
- Around line 59-61: The jetty modules string in
self.jetty_app_configuration[self.service_name]['jetty']['modules'] is being
mutated with "+= ',http2c'", which can append duplicate "http2c" entries across
repeated install() calls; before appending, check the existing modules (split by
commas and strip whitespace) and only add "http2c" if it's not already present,
then join back into the modules string and proceed to call
self.install_jettyService(...).
In `@jans-linux-setup/jans_setup/templates/apache/https_jans.conf.mako`:
- Around line 166-167: In the apache template block guarded by
context.get('install_jans_lock') (the ProxyPass line in https_jans.conf.mako),
remove the stray trailing single-quote character at the end of the ProxyPass
directive so the URL token ends with /api/v1/configuration rather than
/api/v1/configuration'; update the ProxyPass entry for
/.well-known/lock-server-configuration to use the correct URL token (no trailing
quote) to avoid Apache parse errors.
- Around line 147-151: The current directives use both "Require all granted" and
"Require method GET POST", which are OR'd and make the method restriction
ineffective; either remove the "Require all granted" directive or wrap both in a
"<RequireAll>" container so that access is allowed only for GET and POST. Locate
the block containing "Require all granted" and "Require method GET POST" in
https_jans.conf.mako and apply one of the two fixes: delete the "Require all
granted" line if only method restriction is intended, or replace the two
standalone Require lines with a "<RequireAll>" container enclosing both
requirements to enforce both conditions.
In
`@jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/CedarlingProtectionService.java`:
- Around line 87-89: In CedarlingProtectionService (the block checking
requestedPermissions), replace the non-idiomatic check
"requestedPermissions.size() == 0" with the idiomatic collection check
"requestedPermissions.isEmpty()" so the conditional reads e.g. if
(requestedPermissions.isEmpty()) { return simpleResponse(INTERNAL_SERVER_ERROR,
"Access to operation is not correct"); } to improve readability and performance.
- Around line 82-83: The code in CedarlingProtectionService currently removes
the "Bearer" prefix using a case-sensitive regex and logs the full bearerToken
(log.debug("Validating token {}", bearerToken)), which risks auth failures and
credential leakage; update the prefix removal to use a case-insensitive pattern
like "(?i)Bearer\\s+" (matching other extractBearerToken implementations) and
remove or replace the debug call so it does not emit the full token (e.g., log
only that a token was received or log a masked/hashed token) while keeping the
variable name bearerToken and the validation flow intact.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/audit/HealthEntry.java`:
- Around line 131-147: The equals() and hashCode() implementations in
HealthEntry currently omit the inum field (which is annotated ignoreDuringUpdate
= true and likely serves as the unique identifier), so add inum to both equality
checks and the hash calculation to ensure entries with different inum are not
considered equal; specifically update HealthEntry.equals to include
Objects.equals(inum, that.inum) in the returned conjunction and update
HealthEntry.hashCode to include inum in the Objects.hash(...) call (or
alternatively, if content-only equality is desired, add a clear Javadoc on
HealthEntry explaining why inum is intentionally excluded).
- Around line 70-73: The engineStatus field in HealthEntry is missing an
explicit Jackson mapping; add the `@JsonProperty`("engineStatus") annotation to
the engineStatus field (which already has `@JsonObject` and `@AttributeName`(name =
"engineStatus")) so its JSON name is explicit and consistent with the other
fields in the HealthEntry class.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/BaseDnConfiguration.java`:
- Around line 1-7: Update the copyright year in the file containing class
BaseDnConfiguration from 2024 to the correct year (e.g., 2026) to reflect the PR
creation date; locate the header comment block at the top of the file (the
multi-line comment preceding the class declaration) and change the "(c) 2024,
Janssen Project" text to "(c) 2026, Janssen Project" (or the appropriate year),
keeping the existing license text intact.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/Conf.java`:
- Line 1: The file's top-of-file comment opener is invalid ("/*/*") causing a
compilation error; in Conf.java (the Conf class/source), replace the malformed
opener with a standard block comment opener (e.g., "/*") so the comment starts
correctly and no stray tokens remain—ensure the rest of the header comment and
closing "*/" remain intact.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/GrpcServerMode.java`:
- Around line 9-37: GrpcServerMode currently only defines `@JsonValue` getMode()
which serializes the enum but does not provide a way for Jackson to deserialize
strings like "bridge" or "plain_server"; add a static factory annotated with
`@JsonCreator` (e.g., public static GrpcServerMode fromString(String mode)) that
maps the incoming string to the correct enum constant (matching the mode field,
case- and null-safe, and throwing or returning a default if unmatched) so
DataMapperService.readValue() can deserialize JSON into GrpcServerMode
correctly.
In `@jans-lock/lock-server/service/pom.xml`:
- Around line 66-85: The protobuf-maven-plugin block hardcodes protoc and grpc
plugin versions (protocArtifact and pluginArtifact); change these to use
BOM-managed properties (e.g. refer to ${protobuf.version} and ${grpc.version} or
the existing BOM property used for protobuf-java) instead of literal "4.33.2"
and "1.78.0" in the protobuf-maven-plugin configuration, and add a
protobuf.version property to the jans-bom (or reuse the protobuf-java property
at line 827) so the plugin picks up the BOM-managed version; keep the pluginId
and execution (id "compile_proto") unchanged.
- Around line 164-172: The POM currently declares the javax.servlet-api
dependency which conflicts with grpc-servlet-jakarta and the project's Jakarta
APIs; remove the <artifactId>javax.servlet-api</artifactId> dependency
declaration and either replace it with the Jakarta servlet artifact
(<artifactId>jakarta.servlet-api</artifactId>) if you need a compile-time
provided stub or simply delete the dependency so the Jakarta EE runtime provides
it, ensuring grpc-servlet-jakarta is used consistently with the jakarta.servlet
namespace.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/filter/AuthorizationProcessingFilter.java`:
- Around line 98-99: The file is missing a blank line between method
definitions; add one empty line after the end of the extractBearerToken() method
so that there is a blank line before the declaration of
unprotectedApiResponse(String name) to match the file's formatting conventions.
- Around line 90-98: The Bearer token extraction logic in
AuthorizationProcessingFilter.extractBearerToken() is duplicated in
CedarlingAuthorizationProcessingFilter; update HeaderUtils.extractBearerToken()
so it performs the same case-insensitive "Bearer\\s+" removal but returns null
when the Authorization header is missing or doesn't start with a Bearer token,
then replace AuthorizationProcessingFilter.extractBearerToken() and the
corresponding method in CedarlingAuthorizationProcessingFilter to call
HeaderUtils.extractBearerToken() instead of duplicating the logic.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java`:
- Around line 94-123: The code calls extractResourceInfo(methodName) and passes
the possibly null ResourceInfo into
authorizationProtection.processAuthorization, which can cause NPEs or improper
authorization; update the interceptor to check the result of extractResourceInfo
and fail-closed when it's null: if ResourceInfo resourceInfo is null, log a
warning including methodName, perform audit logging as a denied request (use
AuditLogEntry and applicationAuditLogger.log with success=false), map an
appropriate HTTP-to-gRPC status and close the call with call.close(...), then
return an empty ServerCall.Listener; only call processAuthorization and
Contexts.interceptCall when resourceInfo is non-null. Ensure references:
extractResourceInfo, ResourceInfo, processAuthorization, AuditLogEntry,
applicationAuditLogger, mapHttpStatusToGrpcStatus, methodName, call, and next.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/server/GrpcServerStarter.java`:
- Around line 67-103: The startup ignores GrpcConfiguration.useTls; update
startGrpcServer to derive an effective mode (e.g., effectiveMode) using both
grpcConfiguration.getServerMode() and grpcConfiguration.isUseTls() so that
useTls=true forces TLS behavior even if serverMode is BRIDGE/default, then
branch on effectiveMode (GrpcServerMode.TLS_SERVER vs PLAIN_SERVER) when
building the server; refer to startGrpcServer,
GrpcConfiguration.getServerMode(), GrpcConfiguration.isUseTls(), and
GrpcServerMode to locate where to compute the effective mode and apply it to the
Netty vs ServerBuilder logic.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/servlet/GrpcAuditServlet.java`:
- Around line 71-74: The log message in GrpcAuditServlet.initializeGrpc() has a
typo ("initializion"); update the log.info call in the initializeGrpc method to
use the correct spelling (e.g., "gRPC adapter initialization") so the
initialization log is spelled correctly and consistent with other messages.
- Around line 165-177: The current URI normalization in GrpcAuditServlet can
yield an empty or missing-leading-slash requestURI/servletPath when stripping
the contextPath (from request.getRequestURI()), which breaks gRPC method
resolution; after computing this.requestURI (and before setting
this.servletPath) ensure the stripped value is non-empty and starts with a
leading '/' (e.g., if substring yields "" or does not startWith("/"), prefix a
"/"), then set this.servletPath from the normalized this.requestURI (otherwise
fall back to request.getServletPath()); update the logic around
request.getRequestURI(), request.getContextPath(), this.requestURI and
this.servletPath accordingly.
- Around line 153-163: The GrpcRequestWrapper currently uses locale-sensitive
toLowerCase for HTTP header normalization and lookups which can break ASCII
headers (e.g., Turkish locale); update header normalization and lookup calls to
use Locale.ROOT: change headers.put(name.toLowerCase(Locale.ROOT), values) in
the GrpcRequestWrapper constructor and change
headers.get(name.toLowerCase(Locale.ROOT)) wherever header lookups occur (e.g.,
the methods referencing the map at the two noted retrieval sites), and add the
import for java.util.Locale; this ensures locale-insensitive lowercasing for
header names.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/openid/OpenIdProtection.java`:
- Around line 13-16: OpenIdProtection currently redeclares processAuthorization
which is already inherited from BaseAuthorizationProtection; remove the
redundant method declaration (processAuthorization(String bearerToken,
ResourceInfo resourceInfo)) from the OpenIdProtection interface so it simply
extends BaseAuthorizationProtection without re-declaring the inherited method.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/openid/OpenIdProtectionService.java`:
- Line 93: The code in OpenIdProtectionService currently strips the bearer
prefix using a case-sensitive regex ("Bearer\\s+"), causing failure for
lowercase variations; change the replacement to use a case-insensitive pattern
(e.g., use the "(?i)Bearer\\s+" regex) when modifying bearerToken so it matches
"Bearer", "bearer", etc., to be consistent with
AuthorizationProcessingFilter.extractBearerToken().
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/audit/AuditRestWebServiceImpl.java`:
- Around line 366-368: There's a typo in the StatService method name: change
reportOpearation to reportOperation across the codebase; update the StatService
interface declaration, all implementing classes (e.g., classes implementing
StatService), and every call site such as AuditRestWebServiceImpl where
statService.reportOpearation(...) is invoked so they call
statService.reportOperation(...). Ensure method signatures remain the same aside
from the corrected name and run/adjust any package-level imports or references
affected by the rename.
- Around line 411-443: persistBulkAuditData currently iterates and calls
auditService.addLogEntry/addHealthEntry/addTelemetryEntry one-by-one, which is
non-atomic and inefficient; change this by either (A) adding batch methods on
AuditService (e.g. addLogEntries(List<LogEntry>),
addHealthEntries(List<HealthEntry>), addTelemetryEntries(List<TelemetryEntry>))
and have persistBulkAuditData call those batch methods instead of per-entry
calls, or (B) wrap the persistBulkAuditData operation in a transactional
boundary (e.g., annotate persistBulkAuditData or its caller with `@Transactional`)
and ensure AuditService implementations participate in the transaction so all
entries are committed or rolled back together. Ensure corresponding AuditService
interface and implementations are updated to support the chosen approach and
handle errors consistently.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/util/HeaderUtils.java`:
- Around line 62-74: The method in HeaderUtils that extracts bearer tokens
currently checks trimmed.toLowerCase().startsWith("bearer ") and then
redundantly checks trimmed.startsWith("Bearer "), and finally returns the header
as-is for non-bearer schemes; change it so you remove the second redundant
startsWith check (the toLowerCase check already covers case variants) and change
the non-bearer fallback to return null (not the original header) so only bearer
tokens are extracted (keep the existing substring(7).trim() logic when a bearer
prefix is detected).
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/util/ServerUtil.java`:
- Around line 141-170: In getGrpcClientIpAddress(io.grpc.ServerCall<?, ?> call,
Metadata headers) change the lookup order to prefer the socket address from
call.getAttributes() (io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR) before trusting
client-controllable headers (getIpFromHeaders), and only consult headers when a
trusted-proxy configuration is enabled; also make the socket-address extraction
null-safe by not calling InetSocketAddress.getAddress().getHostAddress()
directly—use InetSocketAddress.getHostString() as a safe fallback or check
getAddress() for null before calling getHostAddress() to avoid NPEs; update any
related logic in GrpcAuthorizationInterceptor that relies on this method to
ensure authorization uses the socket-derived IP unless proxy trust is explicitly
configured.
In `@jans-lock/lock-server/service/src/main/proto/grpc/audit.proto`:
- Around line 79-83: The shared AuditResponse message is too coarse for bulk
RPCs; update the proto definition for AuditResponse (used by RPCs handling
multiple records) to include granular bulk-processing fields such as int32
processed_count, int32 failed_count and repeated AuditFailure failures (define
AuditFailure with index/id and string error_message), and optionally a repeated
string failed_ids or map<string,string> failure_details so callers can
distinguish partial successes and inspect per-item errors while preserving
existing success/message fields.
- Around line 35-38: Replace the open string fields with proto enums: define
enums (e.g., EventType, SeverityLevel, DecisionResult, and EntryStatus) in this
proto and change the message fields event_type, severity_level, decision_result
and the status fields in HealthEntry/TelemetryEntry to use those enum types
instead of string; ensure each enum has explicit numeric values and update any
generated code or usages to refer to the new enum symbols (EventType,
SeverityLevel, DecisionResult, EntryStatus) so callers use the typed enum values
rather than freeform strings.
...core/service/src/main/java/io/jans/service/security/protect/BaseAuthorizationProtection.java
Show resolved
Hide resolved
| if Config.install_jans_lock: | ||
| self.jetty_app_configuration[self.service_name]['jetty']['modules'] += ',http2c' | ||
| self.install_jettyService(self.jetty_app_configuration[self.service_name], True) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential duplicate module addition on repeated calls.
The string concatenation += ',http2c' mutates jetty_app_configuration in-place. If install() is called multiple times (e.g., during retries or testing), http2c will be appended repeatedly, resulting in invalid module strings like ...,http2c,http2c.
Consider guarding against duplicate additions:
Proposed fix
self.make_pairwise_calculation_salt()
if Config.install_jans_lock:
- self.jetty_app_configuration[self.service_name]['jetty']['modules'] += ',http2c'
+ modules = self.jetty_app_configuration[self.service_name]['jetty']['modules']
+ if 'http2c' not in modules.split(','):
+ self.jetty_app_configuration[self.service_name]['jetty']['modules'] = modules + ',http2c'
self.install_jettyService(self.jetty_app_configuration[self.service_name], 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.
| if Config.install_jans_lock: | |
| self.jetty_app_configuration[self.service_name]['jetty']['modules'] += ',http2c' | |
| self.install_jettyService(self.jetty_app_configuration[self.service_name], True) | |
| if Config.install_jans_lock: | |
| modules = self.jetty_app_configuration[self.service_name]['jetty']['modules'] | |
| if 'http2c' not in modules.split(','): | |
| self.jetty_app_configuration[self.service_name]['jetty']['modules'] = modules + ',http2c' | |
| self.install_jettyService(self.jetty_app_configuration[self.service_name], True) |
🧰 Tools
🪛 Ruff (0.14.13)
61-61: Boolean positional value in function call
(FBT003)
🤖 Prompt for AI Agents
In `@jans-linux-setup/jans_setup/setup_app/installers/jans_auth.py` around lines
59 - 61, The jetty modules string in
self.jetty_app_configuration[self.service_name]['jetty']['modules'] is being
mutated with "+= ',http2c'", which can append duplicate "http2c" entries across
repeated install() calls; before appending, check the existing modules (split by
commas and strip whitespace) and only add "http2c" if it's not already present,
then join back into the modules string and proceed to call
self.install_jettyService(...).
...k-server/service/src/main/java/io/jans/lock/service/ws/rs/audit/AuditRestWebServiceImpl.java
Show resolved
Hide resolved
| // Check if it's a bearer token | ||
| if (trimmed.toLowerCase().startsWith("bearer ")) { | ||
| return trimmed.substring(7).trim(); // Remove "bearer " prefix | ||
| } | ||
|
|
||
| // Also accept "Bearer " with capital B | ||
| if (trimmed.startsWith("Bearer ")) { | ||
| return trimmed.substring(7).trim(); | ||
| } | ||
|
|
||
| // If it doesn't start with bearer, return as-is (might be basic auth or other) | ||
| return trimmed; | ||
| } |
There was a problem hiding this comment.
Redundant case check and questionable fallback behavior.
-
Lines 68-70 are redundant—the
toLowerCase().startsWith("bearer ")check at line 63 already covers "Bearer " with capital B. -
Lines 72-73 return the header as-is if it doesn't start with "Bearer". This means Basic auth or other schemes would pass through unchanged. If this method should only extract bearer tokens, it should return
nullfor non-bearer inputs instead.
Proposed fix
public static String extractBearerToken(String authHeader) {
if (authHeader == null || authHeader.trim().isEmpty()) {
return null;
}
// Remove any extra whitespace
String trimmed = authHeader.trim();
- // Check if it's a bearer token
- if (trimmed.toLowerCase().startsWith("bearer ")) {
- return trimmed.substring(7).trim(); // Remove "bearer " prefix
- }
-
- // Also accept "Bearer " with capital B
- if (trimmed.startsWith("Bearer ")) {
+ // Check if it's a bearer token (case-insensitive)
+ if (trimmed.toLowerCase().startsWith("bearer ")) {
return trimmed.substring(7).trim();
}
- // If it doesn't start with bearer, return as-is (might be basic auth or other)
- return trimmed;
+ // Not a bearer token
+ return null;
}📝 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.
| // Check if it's a bearer token | |
| if (trimmed.toLowerCase().startsWith("bearer ")) { | |
| return trimmed.substring(7).trim(); // Remove "bearer " prefix | |
| } | |
| // Also accept "Bearer " with capital B | |
| if (trimmed.startsWith("Bearer ")) { | |
| return trimmed.substring(7).trim(); | |
| } | |
| // If it doesn't start with bearer, return as-is (might be basic auth or other) | |
| return trimmed; | |
| } | |
| // Check if it's a bearer token (case-insensitive) | |
| if (trimmed.toLowerCase().startsWith("bearer ")) { | |
| return trimmed.substring(7).trim(); | |
| } | |
| // Not a bearer token | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/util/HeaderUtils.java`
around lines 62 - 74, The method in HeaderUtils that extracts bearer tokens
currently checks trimmed.toLowerCase().startsWith("bearer ") and then
redundantly checks trimmed.startsWith("Bearer "), and finally returns the header
as-is for non-bearer schemes; change it so you remove the second redundant
startsWith check (the toLowerCase check already covers case variants) and change
the non-bearer fallback to return null (not the original header) so only bearer
tokens are extracted (keep the existing substring(7).trim() logic when a bearer
prefix is detected).
| /** | ||
| * Get client IP address when ServerCall is available (e.g., in interceptor) | ||
| */ | ||
| public static String getGrpcClientIpAddress(io.grpc.ServerCall<?, ?> call, Metadata headers) { | ||
| try { | ||
| // Method 1: Try to get from metadata headers | ||
| String ipFromHeaders = getIpFromHeaders(headers); | ||
| if (ipFromHeaders != null) { | ||
| log.debug("Client IP from headers: {}", ipFromHeaders); | ||
| return ipFromHeaders; | ||
| } | ||
|
|
||
| // Method 2: Get from ServerCall attributes | ||
| if (call != null) { | ||
| io.grpc.Attributes attributes = call.getAttributes(); | ||
| java.net.SocketAddress remoteAddr = attributes.get(io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR); | ||
| if (remoteAddr instanceof java.net.InetSocketAddress) { | ||
| java.net.InetSocketAddress inetAddr = (java.net.InetSocketAddress) remoteAddr; | ||
| String ip = inetAddr.getAddress().getHostAddress(); | ||
| log.debug("Client IP from ServerCall attributes: {}", ip); | ||
| return ip; | ||
| } | ||
| } | ||
|
|
||
| log.warn("Could not determine client IP address"); | ||
| return "unknown"; | ||
|
|
||
| } catch (Exception e) { | ||
| log.error("Error getting client IP address", e); | ||
| return "unknown"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the ServerUtil.java file and examine the relevant method
find . -type f -name "ServerUtil.java" | head -5Repository: JanssenProject/jans
Length of output: 221
🏁 Script executed:
# Check the structure of the jans-lock directory
fd "ServerUtil.java" --type fRepository: JanssenProject/jans
Length of output: 217
🏁 Script executed:
# Let's search for the file in the jans-lock directory
find . -path "*/jans-lock/lock-server/service/src/main/java/io/jans/lock/util/ServerUtil.java" 2>/dev/nullRepository: JanssenProject/jans
Length of output: 142
🏁 Script executed:
# Let's take a broader search
git ls-files | grep -i serverutilRepository: JanssenProject/jans
Length of output: 298
🏁 Script executed:
# Search for getGrpcClientIpAddress method to understand the codebase
rg "getGrpcClientIpAddress" --type javaRepository: JanssenProject/jans
Length of output: 432
🏁 Script executed:
# Also search for getIpFromHeaders to understand what headers are being used
rg "getIpFromHeaders" --type java -A 10 -B 2Repository: JanssenProject/jans
Length of output: 3240
🏁 Script executed:
cat -n ./jans-lock/lock-server/service/src/main/java/io/jans/lock/util/ServerUtil.java | head -200Repository: JanssenProject/jans
Length of output: 8686
🏁 Script executed:
# Check the getIpFromHeaders method fully
rg "getIpFromHeaders" --type java -A 30 ./jans-lock/lock-server/service/src/main/java/io/jans/lock/util/ServerUtil.javaRepository: JanssenProject/jans
Length of output: 2110
🏁 Script executed:
# Check how clientIp is used in the interceptor
cat -n ./jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java | grep -A 5 -B 5 "getGrpcClientIpAddress"Repository: JanssenProject/jans
Length of output: 759
🌐 Web query:
InetSocketAddress.getAddress() null behavior Java documentation
💡 Result:
getAddress() returns the InetAddress for the socket address, or null if the InetSocketAddress is unresolved. Check isUnresolved() or createUnresolved(...) to produce an unresolved address (constructors try to resolve the hostname; failure flags it unresolved). [1][2]
Sources:
[1] Oracle Java SE InetSocketAddress Javadoc (getAddress / isUnresolved).
[2] OpenJDK / API summary for InetSocketAddress (getAddress text).
Fix null pointer dereference and reverse priority order for security.
Line 159: InetSocketAddress.getAddress() returns null for unresolved addresses, causing a NullPointerException when getHostAddress() is called without a null check. Use getHostString() as a safe fallback.
Additionally, the IP address lookup prioritizes client-controllable headers (X-Forwarded-For, X-Real-IP) over the trusted gRPC socket address. Since this IP is used for authorization decisions (line 103, GrpcAuthorizationInterceptor), header spoofing is a security issue unless you're behind a trusted proxy. Reverse the priority to prefer the socket address unless a trusted-proxy configuration is in place.
🔧 Suggested null-safe address extraction
- if (remoteAddr instanceof java.net.InetSocketAddress) {
- java.net.InetSocketAddress inetAddr = (java.net.InetSocketAddress) remoteAddr;
- String ip = inetAddr.getAddress().getHostAddress();
+ if (remoteAddr instanceof java.net.InetSocketAddress) {
+ java.net.InetSocketAddress inetAddr = (java.net.InetSocketAddress) remoteAddr;
+ java.net.InetAddress addr = inetAddr.getAddress();
+ String ip = (addr != null) ? addr.getHostAddress() : inetAddr.getHostString();
log.debug("Client IP from ServerCall attributes: {}", ip);
return ip;
}🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/util/ServerUtil.java`
around lines 141 - 170, In getGrpcClientIpAddress(io.grpc.ServerCall<?, ?> call,
Metadata headers) change the lookup order to prefer the socket address from
call.getAttributes() (io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR) before trusting
client-controllable headers (getIpFromHeaders), and only consult headers when a
trusted-proxy configuration is enabled; also make the socket-address extraction
null-safe by not calling InetSocketAddress.getAddress().getHostAddress()
directly—use InetSocketAddress.getHostString() as a safe fallback or check
getAddress() for null before calling getHostAddress() to avoid NPEs; update any
related logic in GrpcAuthorizationInterceptor that relies on this method to
ensure authorization uses the socket-derived IP unless proxy trust is explicitly
configured.
| string event_type = 5; | ||
| string severity_level = 6; | ||
| string action = 7; | ||
| string decision_result = 8; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using enums for constrained value fields.
Fields like event_type, severity_level, decision_result, and status (in HealthEntry/TelemetryEntry) likely represent a finite set of known values. Using proto enums instead of strings would provide:
- Compile-time type safety in generated code
- Self-documenting allowed values
- Smaller wire format
♻️ Example enum definitions
+enum SeverityLevel {
+ SEVERITY_LEVEL_UNSPECIFIED = 0;
+ DEBUG = 1;
+ INFO = 2;
+ WARN = 3;
+ ERROR = 4;
+ CRITICAL = 5;
+}
+
+enum DecisionResult {
+ DECISION_RESULT_UNSPECIFIED = 0;
+ ALLOW = 1;
+ DENY = 2;
+}
+
message LogEntry {
google.protobuf.Timestamp creation_date = 1;
google.protobuf.Timestamp event_time = 2;
string service = 3;
string node_name = 4;
string event_type = 5;
- string severity_level = 6;
+ SeverityLevel severity_level = 6;
string action = 7;
- string decision_result = 8;
+ DecisionResult decision_result = 8;
// ...
}📝 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.
| string event_type = 5; | |
| string severity_level = 6; | |
| string action = 7; | |
| string decision_result = 8; | |
| string event_type = 5; | |
| SeverityLevel severity_level = 6; | |
| string action = 7; | |
| DecisionResult decision_result = 8; |
🤖 Prompt for AI Agents
In `@jans-lock/lock-server/service/src/main/proto/grpc/audit.proto` around lines
35 - 38, Replace the open string fields with proto enums: define enums (e.g.,
EventType, SeverityLevel, DecisionResult, and EntryStatus) in this proto and
change the message fields event_type, severity_level, decision_result and the
status fields in HealthEntry/TelemetryEntry to use those enum types instead of
string; ensure each enum has explicit numeric values and update any generated
code or usages to refer to the new enum symbols (EventType, SeverityLevel,
DecisionResult, EntryStatus) so callers use the typed enum values rather than
freeform strings.
| // Common Response | ||
| message AuditResponse { | ||
| bool success = 1; | ||
| string message = 2; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding granularity for bulk operation responses.
The current AuditResponse is shared across single and bulk RPCs. For bulk operations, a binary success/failure doesn't indicate partial success scenarios (e.g., 98 of 100 entries processed). Consider adding fields to report processing details.
♻️ Proposed enhanced response
message AuditResponse {
bool success = 1;
string message = 2;
+ int32 processed_count = 3;
+ int32 failed_count = 4;
+ repeated string errors = 5; // Optional: per-entry error details
}📝 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.
| // Common Response | |
| message AuditResponse { | |
| bool success = 1; | |
| string message = 2; | |
| } | |
| // Common Response | |
| message AuditResponse { | |
| bool success = 1; | |
| string message = 2; | |
| int32 processed_count = 3; | |
| int32 failed_count = 4; | |
| repeated string errors = 5; // Optional: per-entry error details | |
| } |
🤖 Prompt for AI Agents
In `@jans-lock/lock-server/service/src/main/proto/grpc/audit.proto` around lines
79 - 83, The shared AuditResponse message is too coarse for bulk RPCs; update
the proto definition for AuditResponse (used by RPCs handling multiple records)
to include granular bulk-processing fields such as int32 processed_count, int32
failed_count and repeated AuditFailure failures (define AuditFailure with
index/id and string error_message), and optionally a repeated string failed_ids
or map<string,string> failure_details so callers can distinguish partial
successes and inspect per-item errors while preserving existing success/message
fields.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
jans-lock/lock-server/model/src/main/java/io/jans/lock/model/core/LockApiError.java (1)
58-60: Optional: Fix parameter name typo.The parameter name
messgeis a typo (should bemessage). While this pre-existing issue doesn't affect functionality, correcting it would improve code readability and maintainability.✏️ Suggested fix
- public ErrorBuilder withMessage(String messge) { - this.message = messge; + public ErrorBuilder withMessage(String message) { + this.message = message; return this; }jans-core/service/src/main/java/io/jans/service/security/api/ProtectedApi.java (1)
15-20: Add Javadoc for the new annotation element.Keeps the annotation self-describing alongside
scopes().✍️ Suggested doc addition
- String grpcMethodName() default ""; + /** + * `@return` gRPC method name (e.g., fully-qualified) for protection mapping. + */ + String grpcMethodName() default "";jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/AppConfiguration.java (1)
354-367: UpdatetoString()to includegrpcConfiguration.The new
grpcConfigurationfield is not included intoString(), which will make debugging and log analysis incomplete when gRPC configuration is involved.♻️ Proposed fix
`@Override` public String toString() { return "AppConfiguration [baseDN=" + baseDN + ", baseEndpoint=" + baseEndpoint + ", openIdIssuer=" + openIdIssuer + ", protectionMode=" + protectionMode + ", auditPersistenceMode=" + auditPersistenceMode - + ", cedarlingConfiguration=" + cedarlingConfiguration + ", statEnabled=" + statEnabled + + ", cedarlingConfiguration=" + cedarlingConfiguration + ", grpcConfiguration=" + grpcConfiguration + + ", statEnabled=" + statEnabled + ", statTimerIntervalInSeconds=" + statTimerIntervalInSeconds + ", tokenChannels=" + tokenChannels + ", clientId=" + clientId + ", clientPassword=" + clientPassword + ", disableJdkLogger="jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/filter/CedarlingAuthorizationProcessingFilter.java (1)
100-108: Remove unused methodsunprotectedApiResponseanddisabledApiResponse.These methods are defined but never called within this class. They appear to be leftover from copy-paste or template code. Remove them to reduce dead code in the codebase.
jans-lock/lock-server/service/src/main/java/io/jans/lock/service/openid/OpenIdProtectionService.java (1)
41-41: Consider removing unused import.The
HttpHeadersimport on line 41 appears to be unused after theprocessAuthorizationmethod signature was refactored to acceptString bearerTokendirectly instead ofHttpHeaders.♻️ Proposed fix
import jakarta.ws.rs.container.ResourceInfo; -import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.Response;jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/CedarlingProtectionService.java (1)
164-185: Remove unused variableid.The local variable
idcomputed fromrequestedPermission.hashCode()is never used in the method body. The actual ID value comes fromrequestedPermission.getId()on line 170.♻️ Proposed fix
private Map<String, Object> getCedarlingResource(CedarlingPermission requestedPermission) { HashMap<String, Object> map = new HashMap<String, Object>(); - int id = requestedPermission.hashCode(); - id = id > 0 ? id : -id; map.putAll( Map.of("cedar_entity_mapping", Map.of("entity_type", requestedPermission.getResource(), "id", requestedPermission.getId())jans-config-api/plugins/docs/lock-plugin-swagger.yaml (1)
841-867: Add a description fordisableExternalLoggerConfiguration.
The new flag is undocumented in the schema; adding a short description will keep the docs self-explanatory. If this file is generated, please update the source-of-truth that emits it.📝 Suggested doc tweak
disableExternalLoggerConfiguration: type: boolean + description: Disable loading external logger configuration from filesystem
🤖 Fix all issues with AI agents
In `@jans-bom/pom.xml`:
- Around line 27-30: The pom declares <grpc.version>1.78.0</grpc.version> while
resteasy-grpc-bom:1.0.0.Final manages protoc-gen-grpc-java at 1.73.0; to fix,
run a full build and runtime smoke tests to validate gRPC 1.78.0 with
resteasy-grpc-bom 1.0.0.Final, and if you observe classpath or plugin
incompatibilities, either align versions by setting the protoc-gen-grpc-java
plugin version to 1.78.0 (override the plugin managed by resteasy-grpc-bom) or
downgrade grpc.version to 1.73.0 so they match; locate these properties in the
pom (grpc.version, grpc-bridge.version, and the resteasy-grpc-bom import) and
update or add the plugin override accordingly, then re-run CI and integration
tests to confirm no conflicts.
In `@jans-lock/lock-server.yaml`:
- Around line 694-700: The policyStats schema repeats the same description
inside additionalProperties; remove the description field from the
additionalProperties block and keep the single description on the parent
policyStats object to eliminate redundancy (target symbols: policyStats and its
additionalProperties entry).
In
`@jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/config/BootstrapConfig.java`:
- Around line 1-5: The license header in class BootstrapConfig currently uses
the non-standard phrase "Apache License (2004)"; update the header comment to
use the standard reference "Apache License, Version 2.0" (or include the SPDX
identifier "Apache-2.0") and ensure the formatting matches other project source
files—replace the existing line in the top-of-file comment block in
BootstrapConfig.java accordingly.
In
`@jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/filter/CedarlingAuthorizationProcessingFilter.java`:
- Around line 90-98: extractBearerToken currently returns the raw Authorization
header when it doesn't contain a Bearer token (e.g., "Basic ..."), which then
propagates invalid values to processAuthorization; update extractBearerToken to
explicitly validate the Bearer scheme before returning a token by checking the
header matches the Bearer pattern (case-insensitive) or starts with "Bearer "
ignoring case, then extract and trim the token, otherwise return null so only
valid Bearer tokens reach processAuthorization.
In
`@jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/inject/CedarlingPolicy.java`:
- Around line 1-5: Update the file header in CedarlingPolicy.java to use the
standard license name "Apache License, Version 2.0" instead of "Apache License
(2004)" and adjust the copyright years to reflect the creation-to-current span
(e.g., "2022-2025" or "2022, 2025"); modify only the file comment block at the
top of CedarlingPolicy.java to make these textual changes so the license and
copyright statements are accurate and standard.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/audit/LogEntry.java`:
- Around line 217-242: The equals() and hashCode() implementations in LogEntry
currently omit the immutable persisted identifier inum (and the inherited DN),
causing distinct persisted entries to compare equal; update
LogEntry.equals(Object o) to include Objects.equals(inum, logEntry.inum) (and if
you rely on the parent Entry DN, include Objects.equals(getDn(),
logEntry.getDn()) or the dn field) in the chain of comparisons, and add inum
(and dn/getDn()) to the Objects.hash(...) call in LogEntry.hashCode() so
identity includes the persisted identifier.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/cedarling/LogLevel.java`:
- Around line 2-4: Update the license header in LogLevel.java to use the
standard Apache phrasing: replace "Apache License (2004)" with "Apache License,
Version 2.0" and use the canonical URL
"https://www.apache.org/licenses/LICENSE-2.0"; ensure the header matches the
project's standard template (same wording, copyright line format) for the
LogLevel class so future files are consistent.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/Conf.java`:
- Line 7: Remove the extra blank line after the license header in Conf.java so
the file follows Java formatting conventions; ensure there is a single blank
line (or none if header ends immediately) before the package declaration
"package io.jans.lock.model.config;" so the file header and package statement
are directly adjacent and consistently formatted.
In
`@jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/grpc/GrpcConfiguration.java`:
- Around line 27-29: The grpcPort field in GrpcConfiguration allows any int but
should be constrained to valid TCP ports (1–65535); add validation to prevent
invalid values by annotating the grpcPort field with
javax.validation.constraints.Min(1) and javax.validation.constraints.Max(65535)
and/or add a guard in the GrpcConfiguration.setGrpcPort(int) setter that throws
IllegalArgumentException for values outside 1–65535; update the field
documentation if needed so the schema and runtime validation both enforce the
valid port range.
- Around line 31-42: Add validation to GrpcConfiguration to enforce TLS config
consistency: when the boolean field useTls is true, ensure tlsCertChainFilePath
and tlsPrivateKeyFilePath are non-blank and throw a clear IllegalStateException
if either is missing; implement this as a public validate() method on the
GrpcConfiguration class (or annotate with Bean Validation if preferred) and call
validate() before gRPC server startup so fields useTls, tlsCertChainFilePath,
and tlsPrivateKeyFilePath are checked early.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/audit/GrpcAuditServiceImpl.java`:
- Line 56: Change the routine gRPC invocation logs in GrpcAuditServiceImpl from
INFO to DEBUG to reduce verbosity: replace log.info(...) with log.debug(...) in
the methods processHealth, processBulkHealth, processLog, processBulkLog,
processTelemetry, and processBulkTelemetry; keep INFO-level logs only for
significant events (errors or notable state changes) and ensure the same log
message text is preserved when switching to DEBUG.
- Around line 65-68: The catch blocks in GrpcAuditServiceImpl currently call
responseObserver.onError(e) with raw exceptions (and log.error(..., e)), which
can leak internal stack traces to gRPC clients; change those catch handlers to
log the exception internally (using log.error) but pass a wrapped
StatusRuntimeException to responseObserver.onError, e.g., create a
StatusRuntimeException via Status.INTERNAL.withDescription("Error processing
<operation>").withCause(null).asRuntimeException() or similar (include minimal
safe description), and replace direct responseObserver.onError(e) calls with
responseObserver.onError(wrappedException); apply the same pattern to every
catch block in GrpcAuditServiceImpl, ensuring you reference
responseObserver.onError and use Status.*.asRuntimeException() to create the
wrapper.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/audit/GrpcAuditServiceProvider.java`:
- Around line 39-48: The init() method in GrpcAuditServiceProvider should
validate injected dependencies before using them: check that
auditRestWebService, mapper, and log are not null and throw a clear
IllegalStateException (or log an explicit error) if any are null, then only
instantiate grpcAuditService = new GrpcAuditServiceImpl(...). Add these null
checks at the start of init() to fail-fast with a descriptive message
referencing the missing dependency names so the cause is clear when CDI
injection fails.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java`:
- Around line 65-66: The constant AUTHORIZATION_METADATA_KEY in
GrpcAuthorizationInterceptor is unused (the code calls
HeaderUtils.findAndExtractBearerToken(headers) instead); remove the unused field
declaration to clean up the class and imports, and ensure no other references
remain to AUTHORIZATION_METADATA_KEY after deletion so compilation is
unaffected.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/server/GrpcServerStarter.java`:
- Around line 140-165: The stopGrpcServer method leaves the grpcServer field
non-null after shutdown, which can cause stale state on later restarts; update
stopGrpcServer to set grpcServer = null after a successful shutdown path and
also after the InterruptedException handling (i.e., after forcing shutdown in
both the normal and catch paths) so that the grpcServer reference is cleared for
future reinitialization; locate the method stopGrpcServer and the grpcServer
field to add the null assignment in all exit paths.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/OpenIdService.java`:
- Around line 2-4: Update the file header in OpenIdService.java to use the
standardized Apache License, Version 2.0 header used elsewhere (e.g.,
ConfigurationService.java) and change the copyright year to 2025 (or use the
range "2022-2025" if you prefer); specifically replace the existing top-of-file
block comment with the current project license template and updated year(s) so
the class OpenIdService reflects the consistent license header format across the
codebase.
In `@jans-lock/lock-server/service/src/main/proto/grpc/audit.proto`:
- Around line 1-9: The proto declares package io.jans.lock.audit (and outer
classname AuditProto) but the file lives outside the expected io/jans/lock/audit
directory, causing Buf lint failures; fix by either moving this proto file into
the repository proto root subdirectory path io/jans/lock/audit so the filesystem
matches the declared package, or update the package declaration to match the
current directory layout (and update references to AuditProto accordingly);
alternatively, if you intentionally keep the current layout, add/configure
buf.yaml to allow this directory mapping so Buf won’t reject files with package
io.jans.lock.audit.
♻️ Duplicate comments (29)
jans-lock/lock-server/service/src/main/java/io/jans/lock/service/filter/AuthorizationProcessingFilter.java (1)
90-97: Validate Bearer prefix and reject non-token headers before passing downstream.Currently a non‑Bearer Authorization header (or whitespace-only) is forwarded unchanged, which can bypass expected token semantics and lead to inconsistent auth behavior. Ensure only Bearer tokens are accepted and blank tokens return null.
🔧 Proposed fix (strict Bearer parsing)
private String extractBearerToken() { String authHeader = httpHeaders.getHeaderString(HttpHeaders.AUTHORIZATION); - - if (StringUtils.isEmpty(authHeader)) { - return null; - } - - return authHeader.replaceFirst("(?i)Bearer\\s+", ""); + if (StringUtils.isBlank(authHeader)) { + return null; + } + if (!StringUtils.startsWithIgnoreCase(authHeader, "Bearer ")) { + return null; + } + String token = authHeader.substring(7).trim(); + return StringUtils.isBlank(token) ? null : token; }jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/Conf.java (1)
1-1: Fix invalid comment opener to avoid compilation failure.The malformed opener
/*/*on Line 1 will cause a Java syntax error: the first/*opens a block comment, the*/closes it immediately, leaving a stray*token that prevents compilation.🛠️ Proposed fix
-/*/* +/*jans-linux-setup/jans_setup/setup_app/installers/jans_auth.py (1)
59-60: Potential duplicate module addition on repeated calls.The string concatenation
+= ',http2c'mutatesjetty_app_configurationin-place. Ifinstall()is called multiple times,http2cwill be appended repeatedly, resulting in invalid module strings like...,http2c,http2c.Guard against duplicate additions by checking if
http2cis already present before appending.jans-linux-setup/jans_setup/setup_app/data/jetty_app_configuration.json (1)
82-84: Confirm h2c exposure is intentional for gRPC audit bridge, or enable TLS by default.
http2cenables cleartext HTTP/2, which is used as the transport for the gRPC audit servlet. This leaves audit traffic unencrypted when jans-lock is not behind a TLS-terminating proxy. Either document the requirement for external TLS termination or enable TLS in gRPC configuration by default.jans-linux-setup/jans_setup/templates/apache/https_jans.conf.mako (2)
147-151: Conflicting access control directives render method restriction ineffective.
Require all granted(line 148) unconditionally allows access, makingRequire method GET POST(line 151) redundant. In Apache 2.4, multipleRequiredirectives are OR'd by default.Remove
Require all grantedif method restriction is intended, or use a<RequireAll>container.
166-167: Syntax error: trailing single quote in ProxyPass directive.Line 167 has a trailing single quote (
') that will cause Apache configuration parsing to fail:ProxyPass /.well-known/lock-server-configuration http://localhost:${lock_host_port}/${lock_host_suffix}/api/v1/configuration'Remove the trailing quote.
jans-linux-setup/jans_setup/schema/jans_schema.json (1)
4081-4085: Preserve backward compatibility for the renamed attribute.Existing deployments that still use
severetyLevelwill break if the legacy alias is removed. Please keep both names.🩹 Proposed compatibility fix
"names": [ - "severityLevel" + "severityLevel", + "severetyLevel" ],jans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/BaseDnConfiguration.java (1)
2-4: Update the copyright year to the current range.The header still shows 2024; this PR is dated January 2026.
📅 Proposed fix
- * Copyright (c) 2024, Janssen Project + * Copyright (c) 2026, Janssen Projectjans-lock/lock-server/model/src/main/java/io/jans/lock/model/config/GrpcServerMode.java (1)
9-37: Add@JsonCreatorto support JSON deserialization.
@JsonValuehandles serialization only; without a creator or mapper settings, values like"bridge"may fail to deserialize.🛠️ Suggested fix
import com.fasterxml.jackson.annotation.JsonValue; +import com.fasterxml.jackson.annotation.JsonCreator; public enum GrpcServerMode { DISABLED("disabled"), BRIDGE("bridge"), PLAIN_SERVER("plain_server"), TLS_SERVER("tls_server"); @@ private final String mode; private GrpcServerMode(String mode) { this.mode = mode; } + + `@JsonCreator` + public static GrpcServerMode fromString(String value) { + if (value == null) { + return null; + } + for (GrpcServerMode mode : values()) { + if (mode.mode.equalsIgnoreCase(value)) { + return mode; + } + } + throw new IllegalArgumentException("Unsupported GrpcServerMode: " + value); + } @@ `@JsonValue` public String getMode() { return mode; } }#!/bin/bash # Check mapper settings for enum deserialization rg -n "READ_ENUMS_USING_TO_STRING|ACCEPT_CASE_INSENSITIVE_ENUMS|ObjectMapper" --type javajans-core/service/src/main/java/io/jans/service/security/protect/BaseAuthorizationProtection.java (1)
12-13: Clarify bearer token format and null/empty handling in the contract.
Add Javadoc specifying whetherbearerTokenis raw (no “Bearer ” prefix) and how null/empty should be handled so implementations are consistent.♻️ Suggested Javadoc
public interface BaseAuthorizationProtection { + /** + * `@param` bearerToken raw bearer token value (without "Bearer " prefix), or null/empty if absent + * `@param` resourceInfo JAX-RS resource info for the target endpoint + * `@return` authorization result + */ Response processAuthorization(String bearerToken, ResourceInfo resourceInfo); }jans-lock/lock-server/service/src/main/java/io/jans/lock/service/openid/OpenIdProtection.java (1)
13-16: Remove the redundant method declaration.
processAuthorizationis already inherited fromBaseAuthorizationProtection; re-declaring it is unnecessary and risks drift.♻️ Proposed cleanup
public interface OpenIdProtection extends BaseAuthorizationProtection { - - Response processAuthorization(String bearerToken, ResourceInfo resourceInfo); - public static Response simpleResponse(Response.Status status, String detail) { return Response.status(status).entity(detail).build(); }jans-lock/lock-server/service/src/main/java/io/jans/lock/util/HeaderUtils.java (1)
54-73: Return null for non-bearer schemes and drop the redundant check.
extractBearerTokencurrently returns the header as-is for non-bearer schemes and repeats a case check; that conflicts with its contract (“null if invalid”).🐛 Proposed fix
public static String extractBearerToken(String authHeader) { if (authHeader == null || authHeader.trim().isEmpty()) { return null; } // Remove any extra whitespace String trimmed = authHeader.trim(); - // Check if it's a bearer token - if (trimmed.toLowerCase().startsWith("bearer ")) { - return trimmed.substring(7).trim(); // Remove "bearer " prefix - } - - // Also accept "Bearer " with capital B - if (trimmed.startsWith("Bearer ")) { - return trimmed.substring(7).trim(); - } - - // If it doesn't start with bearer, return as-is (might be basic auth or other) - return trimmed; + // Check if it's a bearer token (case-insensitive) + if (trimmed.toLowerCase().startsWith("bearer ")) { + return trimmed.substring(7).trim(); + } + + // Not a bearer token + return null; }jans-lock/lock-server/service/src/main/java/io/jans/lock/util/ServerUtil.java (1)
144-172: Fix null pointer dereference and reverse IP lookup priority for security.This is a duplicate of a previous review comment that remains unaddressed:
NPE risk (Line 159):
inetAddr.getAddress()returnsnullfor unresolved addresses, causingNullPointerExceptionon.getHostAddress().Security issue: The method prioritizes client-controllable headers (
X-Forwarded-For,X-Real-IP) over the trusted socket address. Since this IP is used for authorization inGrpcAuthorizationInterceptor, header spoofing is a security risk unless behind a trusted proxy.🔧 Suggested fix for null-safety and secure priority
public static String getGrpcClientIpAddress(io.grpc.ServerCall<?, ?> call, Metadata headers) { try { - // Method 1: Try to get from metadata headers - String ipFromHeaders = getIpFromHeaders(headers); - if (ipFromHeaders != null) { - log.debug("Client IP from headers: {}", ipFromHeaders); - return ipFromHeaders; - } - - // Method 2: Get from ServerCall attributes + // Method 1: Prefer trusted socket address if (call != null) { io.grpc.Attributes attributes = call.getAttributes(); java.net.SocketAddress remoteAddr = attributes.get(io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR); if (remoteAddr instanceof java.net.InetSocketAddress) { java.net.InetSocketAddress inetAddr = (java.net.InetSocketAddress) remoteAddr; - String ip = inetAddr.getAddress().getHostAddress(); + java.net.InetAddress addr = inetAddr.getAddress(); + String ip = (addr != null) ? addr.getHostAddress() : inetAddr.getHostString(); log.debug("Client IP from ServerCall attributes: {}", ip); return ip; } } + // Method 2: Fallback to headers (only if trusted proxy is configured) + String ipFromHeaders = getIpFromHeaders(headers); + if (ipFromHeaders != null) { + log.debug("Client IP from headers: {}", ipFromHeaders); + return ipFromHeaders; + } + log.warn("Could not determine client IP address"); return "unknown";jans-lock/lock-server/model/src/main/java/io/jans/lock/model/audit/HealthEntry.java (2)
70-73: Missing@JsonPropertyannotation onengineStatusfield.Unlike other fields in this class,
engineStatuslacks the@JsonPropertyannotation. While Jackson uses the field name by default, adding the annotation maintains consistency and explicit API contract.Suggested fix
// Details: cedarEngineStatus, cedarPolicyStatus, tokenDataStatus. etc.. + `@JsonProperty`("engineStatus") `@JsonObject` `@AttributeName`(name = "engineStatus") private Map<String, String> engineStatus;
131-147: Consider includinginuminequals()andhashCode().The
inumfield is excluded from equality checks. If this is intentional (content-based equality), consider documenting this design decision with a comment. Otherwise, two entries with differentinumvalues but identical content will be considered equal.jans-lock/lock-server/service/src/main/proto/grpc/audit.proto (2)
35-38: Consider using enums for constrained value fields.Fields like
severity_level,decision_result, andstatusrepresent finite known values. Using proto enums would provide compile-time type safety and self-documenting allowed values. This was flagged in a previous review.
79-83: Consider adding granularity for bulk operation responses.The current
AuditResponseis shared across single and bulk RPCs. For bulk operations, adding fields likeprocessed_count,failed_count, and optional error details would help clients handle partial success scenarios. This was flagged in a previous review.jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ws/rs/audit/AuditRestWebServiceImpl.java (2)
366-368: Typo in method name:reportOpearation.The method
statService.reportOpearationcontains a typo ("Opearation" instead of "Operation"). This was flagged in a previous review.
411-443: Non-atomic bulk persistence may leave data in inconsistent state.The bulk persistence methods iterate entries individually without transactional wrapping. If any entry fails, preceding entries remain persisted. This was flagged in a previous review.
Consider either adding batch methods to
AuditServiceor wrapping in a transaction boundary.jans-lock/lock-server/service/pom.xml (2)
164-172: Servlet API namespace conflict:grpc-servlet-jakartarequires Jakarta namespace.This configuration declares
javax.servlet-apiwhile usinggrpc-servlet-jakarta(line 241). Thegrpc-servlet-jakartaartifact is compiled againstjakarta.servletnamespace (Jakarta EE 9+), notjavax.servlet. Mixing these namespaces will cause runtime class loading failures when gRPC servlet integration attempts to use Jakarta servlet types.The comment at line 164-166 acknowledges the concern but the fix is incorrect—remove
javax.servlet-apiand rely on the Jakarta servlet API already provided by the container (or explicitly declarejakarta.servlet-apiif needed at compile time).🔧 Proposed fix
<!-- Servlet --> - <!-- Note: using javax.servlet-api for compatibility with grpc-servlet; avoid mixing Jakarta and javax servlet APIs --> - - <!-- Compatibility: some gRPC/servlet libs compiled against javax.servlet --> - <dependency> - <groupId>javax.servlet</groupId> - <artifactId>javax.servlet-api</artifactId> - <version>4.0.1</version> - <scope>provided</scope> - </dependency> + <dependency> + <groupId>jakarta.servlet</groupId> + <artifactId>jakarta.servlet-api</artifactId> + <scope>provided</scope> + </dependency>,
71-73: Use BOM-managed version properties for protoc and grpc-java plugin.Hardcoding
4.33.2for protoc and1.78.0for grpc-java creates version drift risk when the BOM is updated. Reference the existingprotobuf-javaversion from the BOM (line 827 declares 4.33.2) and add/use agrpc.versionproperty.♻️ Proposed fix
<configuration> - <protocArtifact>com.google.protobuf:protoc:4.33.2:exe:${os.detected.classifier}</protocArtifact> + <protocArtifact>com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}</protocArtifact> <pluginId>grpc-java</pluginId> - <pluginArtifact>io.grpc:protoc-gen-grpc-java:1.78.0:exe:${os.detected.classifier}</pluginArtifact> + <pluginArtifact>io.grpc:protoc-gen-grpc-java:${grpc.version}:exe:${os.detected.classifier}</pluginArtifact> </configuration>,
jans-lock/lock-server/service/src/main/java/io/jans/lock/service/openid/OpenIdProtectionService.java (1)
93-93: Inconsistent case-sensitivity in Bearer prefix removal.The regex
"Bearer\\s+"is case-sensitive, while other extractors in the codebase (e.g.,AuthorizationProcessingFilter.extractBearerToken()) use case-insensitive"(?i)Bearer\\s+". This could cause failures when clients send lowercasebearer.Proposed fix
- bearerToken = bearerToken.replaceFirst("Bearer\\s+",""); + bearerToken = bearerToken.replaceFirst("(?i)Bearer\\s+","");jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/CedarlingProtectionService.java (2)
82-83: Security: Avoid logging bearer tokens and fix case-insensitive prefix removal.
- Line 83 logs the full bearer token at DEBUG level, risking credential leakage in logs.
- Line 82 uses case-sensitive regex
"Bearer\\s+", inconsistent with other extractors that use"(?i)Bearer\\s+".Proposed fix
- bearerToken = bearerToken.replaceFirst("Bearer\\s+",""); - log.debug("Validating token {}", bearerToken); + bearerToken = bearerToken.replaceFirst("(?i)Bearer\\s+",""); + log.debug("Validating bearer token");
87-89: UseisEmpty()instead ofsize() == 0.Prefer idiomatic collection emptiness check for readability.
Proposed fix
- if (requestedPermissions.size() == 0) { + if (requestedPermissions.isEmpty()) {jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java (1)
94-103: Add null guard: deny access whenResourceInfois not found.
extractResourceInfo()can returnnullwhen the gRPC method name format is invalid (line 174) or when no@ProtectedApimapping exists (line 186). The current code proceeds to callprocessAuthorization(token, null), which may cause NPE or silent authorization bypass depending on the protection service implementation.Fail-closed by rejecting requests when resource metadata cannot be resolved.
Suggested fix
ResourceInfo resourceInfo = extractResourceInfo(methodName); + if (resourceInfo == null) { + log.warn("No ResourceInfo mapping for gRPC method: {}", methodName); + + // Audit the denied access + String clientIp = ServerUtil.getGrpcClientIpAddress(call, headers); + AuditLogEntry auditLogEntry = new AuditLogEntry(clientIp, AuditActionType.GRPC_AUTHZ_FILTER); + applicationAuditLogger.log(auditLogEntry, false); + + call.close(Status.PERMISSION_DENIED.withDescription("Authorization error: unknown method"), new Metadata()); + return new ServerCall.Listener<ReqT>() {}; + } log.debug("gRPC call requires access to: {}", resourceInfo);jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/servlet/GrpcAuditServlet.java (3)
71-74: Fix typo in initialization log message."initializion" should be "initialization".
✏️ Proposed fix
- log.info("gRPC adapter initializion"); + log.info("gRPC adapter initialization");
156-163: UseLocale.ROOTfor case-insensitive header name normalization.The default
String#toLowerCase()is locale-sensitive. Turkish locale transforms 'I' to 'ı', corrupting ASCII header names. UseLocale.ROOTfor protocol identifiers.Proposed fix
+import java.util.Locale;- headers.put(name.toLowerCase(), values); + headers.put(name.toLowerCase(Locale.ROOT), values);Also update header lookups at lines 222 and 228:
- List<String> values = headers.get(name.toLowerCase()); + List<String> values = headers.get(name.toLowerCase(Locale.ROOT));
165-178: Normalize stripped URI to keep a leading "/".Edge cases (e.g.,
contextPathequals/) can produce an empty or slash-less path after stripping, breaking gRPC method resolution.🛠️ Proposed fix
if (contextPath != null && !contextPath.isEmpty() && originalURI.startsWith(contextPath)) { - this.requestURI = originalURI.substring(contextPath.length()); - this.servletPath = this.requestURI; + String stripped = originalURI.substring(contextPath.length()); + if (stripped.isEmpty()) { + stripped = "/"; + } else if (!stripped.startsWith("/")) { + stripped = "/" + stripped; + } + this.requestURI = stripped; + this.servletPath = stripped; } else {jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/server/GrpcServerStarter.java (1)
68-103:useTlsflag is ignored when deciding TLS vs plain.
IfuseTls=truebutserverModeis left at its default (e.g.,BRIDGE/PLAIN_SERVER), the server still starts without TLS. This makes theuseTlsfield misleading.🛠️ Suggested fix (derive an effective mode)
- GrpcConfiguration grpcConfiguration = appConfiguration.getGrpcConfiguration(); - if (grpcConfiguration == null || grpcConfiguration.getServerMode() == null || - !(GrpcServerMode.PLAIN_SERVER == grpcConfiguration.getServerMode() || GrpcServerMode.TLS_SERVER == grpcConfiguration.getServerMode())) { + GrpcConfiguration grpcConfiguration = appConfiguration.getGrpcConfiguration(); + if (grpcConfiguration == null || grpcConfiguration.getServerMode() == null) { log.info("gRPC inproc server was disabled in configuration"); return; } - if (GrpcServerMode.TLS_SERVER == grpcConfiguration.getServerMode()) { + GrpcServerMode effectiveMode = grpcConfiguration.getServerMode(); + if (grpcConfiguration.isUseTls() && effectiveMode == GrpcServerMode.PLAIN_SERVER) { + log.warn("useTls=true overrides serverMode=PLAIN_SERVER; starting TLS server"); + effectiveMode = GrpcServerMode.TLS_SERVER; + } + + if (!(GrpcServerMode.PLAIN_SERVER == effectiveMode || GrpcServerMode.TLS_SERVER == effectiveMode)) { + log.info("gRPC inproc server was disabled in configuration"); + return; + } + + if (GrpcServerMode.TLS_SERVER == effectiveMode) { // TLS path ... - } else { + } else { // Plain path ... }
| <resteasy.version>6.2.12.Final</resteasy.version> | ||
| <grpc.version>1.78.0</grpc.version> | ||
| <grpc-bridge.version>1.0.0.Final</grpc-bridge.version> | ||
| <richfaces.version>4.5.19-gluu.Final</richfaces.version> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
resteasy-grpc-bom 1.0.0.Final compatibility RESTEasy 6.2.12
💡 Result:
Short answer: Yes — resteasy-grpc-bom 1.0.0.Final is compatible with RESTEasy 6.2.12.Final.
Why: the resteasy-grpc-bom:1.0.0.Final BOM manages jakarta.ws.rs 3.1.x and other Jakarta 3 / RESTEasy 6-era artifacts and explicitly includes dev.resteasy.grpc:grpc-bridge:1.0.0.Final, so it is intended to be used with RESTEasy 6.x (RESTEasy 6.2.12.Final is a 6.x release). See the BOM managed deps and the RESTEasy 6.2.12.Final release announcement. [1][2][3]
Sources:
- resteasy-grpc-bom:1.0.0.Final (managed deps showing jakarta.ws.rs 3.1.x, grpc-bridge 1.0.0.Final). [1]
- RESTEasy 6.2.12.Final release notes. [2]
- RESTEasy 6.2.12.Final artifact / BOM listing. [3]
🏁 Script executed:
# Let's also check the pom.xml structure to understand how these dependencies are used
find . -name "pom.xml" -path "*/jans-bom/*" | head -5Repository: JanssenProject/jans
Length of output: 81
🏁 Script executed:
# Find and examine the jans-bom/pom.xml file
cat -n jans-bom/pom.xml | head -100Repository: JanssenProject/jans
Length of output: 4514
🏁 Script executed:
# Check lines around 694-710 where this also applies
sed -n '690,720p' jans-bom/pom.xmlRepository: JanssenProject/jans
Length of output: 770
🌐 Web query:
grpc-bom 1.78.0 grpc-bridge 1.0.0.Final compatibility
💡 Result:
Short answer: there’s no published incompatibility between io.grpc:grpc-bom:1.78.0 and the resteasy-grpc "grpc-bridge" 1.0.0.Final series, but resteasy-grpc does not declare or guarantee a specific grpc-java BOM version in its public docs — so you should test your build/runtime combination. Sources: grpc-bom artifact page and the resteasy-grpc project docs. [1][2]
What I recommend
- Try building your project with io.grpc:grpc-bom:1.78.0 and grpc-bridge 1.0.0.Final; run your unit/integration tests (particularly any generated bridging code and runtime startup).
- If you hit dependency conflicts at build time, prefer importing grpc-bom (to control io.grpc versions) and adjust exclusions or alignment in your POM/Gradle. See the grpc BOM and resteasy-grpc docs for usage. [1][2]
Sources
- io.grpc:grpc-bom (Maven Central / mvnrepository). [3]
- resteasy-grpc / grpc-bridge user docs and blog posts (RESTEasy project). [4][5]
If you want, I can: (a) check the grpc-bridge 1.0.0.Final POM for its io.grpc dependency versions, or (b) run a quick sample Maven/Gradle dependency resolution to show exactly which versions would be selected. Which would you prefer?
🌐 Web query:
grpc-bridge 1.0.0.Final POM managed dependencies grpc-java version
💡 Result:
The RESTEasy gRPC BOM (which includes grpc-bridge 1.0.0.Final) manages io.grpc's protoc-gen-grpc-java at version 1.73.0. [1]
Source: resteasy-grpc-bom (dev.resteasy.grpc:resteasy-grpc-bom:1.0.0.Final) managed-dependencies. [1]
🏁 Script executed:
# Search for any test or usage patterns in the repo that might show this combination working
rg -i "grpc.*1\.78" --type xml
rg -i "grpc-bridge.*1\.0\.0" --type xmlRepository: JanssenProject/jans
Length of output: 345
Verify and test gRPC 1.78.0 with resteasy-grpc-bom 1.0.0.Final.
While resteasy-grpc-bom 1.0.0.Final is compatible with RESTEasy 6.2.12.Final, note that resteasy-grpc-bom manages protoc-gen-grpc-java at version 1.73.0, whereas the BOM uses gRPC 1.78.0 (which includes protoc-gen-grpc-java 1.78.0). Although gRPC maintains backward compatibility across patch/minor versions, this version mismatch should be validated via build and runtime testing to rule out classpath conflicts.
🤖 Prompt for AI Agents
In `@jans-bom/pom.xml` around lines 27 - 30, The pom declares
<grpc.version>1.78.0</grpc.version> while resteasy-grpc-bom:1.0.0.Final manages
protoc-gen-grpc-java at 1.73.0; to fix, run a full build and runtime smoke tests
to validate gRPC 1.78.0 with resteasy-grpc-bom 1.0.0.Final, and if you observe
classpath or plugin incompatibilities, either align versions by setting the
protoc-gen-grpc-java plugin version to 1.78.0 (override the plugin managed by
resteasy-grpc-bom) or downgrade grpc.version to 1.73.0 so they match; locate
these properties in the pom (grpc.version, grpc-bridge.version, and the
resteasy-grpc-bom import) and update or add the plugin override accordingly,
then re-run CI and integration tests to confirm no conflicts.
| policyStats: | ||
| type: object | ||
| additionalProperties: | ||
| type: string | ||
| baseDn: | ||
| type: string | ||
| inum: | ||
| type: string | ||
| memoryUsage: | ||
| type: string | ||
| type: integer | ||
| description: Additional policy statistics as key-value pairs | ||
| format: int64 | ||
| description: Additional policy statistics as key-value pairs |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Redundant nested description in policyStats.
The additionalProperties block has its own description which is also repeated in the parent. This redundancy is harmless but could be simplified.
policyStats:
type: object
additionalProperties:
type: integer
format: int64
description: Additional policy statistics as key-value pairs🤖 Prompt for AI Agents
In `@jans-lock/lock-server.yaml` around lines 694 - 700, The policyStats schema
repeats the same description inside additionalProperties; remove the description
field from the additionalProperties block and keep the single description on the
parent policyStats object to eliminate redundancy (target symbols: policyStats
and its additionalProperties entry).
| /* | ||
| * Copyright [2025] [Janssen Project] | ||
| * Janssen Project software is available under the Apache License (2004). See http://www.apache.org/licenses/ for full text. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * Copyright (c) 2025, Janssen Project | ||
| */ |
There was a problem hiding this comment.
Non-standard Apache License reference.
The phrase "Apache License (2004)" is unconventional. The standard reference is "Apache License, Version 2.0" or the SPDX identifier "Apache-2.0". The "(2004)" refers to the release year, not the version, which could cause confusion during license audits.
Suggested fix
/*
- * Janssen Project software is available under the Apache License (2004). See http://www.apache.org/licenses/ for full text.
+ * Janssen Project software is available under the Apache License, Version 2.0. See http://www.apache.org/licenses/LICENSE-2.0 for full text.
*
* Copyright (c) 2025, Janssen Project
*/📝 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.
| /* | |
| * Copyright [2025] [Janssen Project] | |
| * Janssen Project software is available under the Apache License (2004). See http://www.apache.org/licenses/ for full text. | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * You may obtain a copy of the License at | |
| * | |
| * http://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| * Copyright (c) 2025, Janssen Project | |
| */ | |
| /* | |
| * Janssen Project software is available under the Apache License, Version 2.0. See http://www.apache.org/licenses/LICENSE-2.0 for full text. | |
| * | |
| * Copyright (c) 2025, Janssen Project | |
| */ |
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/config/BootstrapConfig.java`
around lines 1 - 5, The license header in class BootstrapConfig currently uses
the non-standard phrase "Apache License (2004)"; update the header comment to
use the standard reference "Apache License, Version 2.0" (or include the SPDX
identifier "Apache-2.0") and ensure the formatting matches other project source
files—replace the existing line in the top-of-file comment block in
BootstrapConfig.java accordingly.
| private String extractBearerToken() { | ||
| String authHeader = httpHeaders.getHeaderString(HttpHeaders.AUTHORIZATION); | ||
|
|
||
| if (StringUtils.isEmpty(authHeader)) { | ||
| return null; | ||
| } | ||
|
|
||
| return authHeader.replaceFirst("(?i)Bearer\\s+", ""); | ||
| } |
There was a problem hiding this comment.
Token extraction returns raw header value for non-Bearer schemes.
If the Authorization header exists but doesn't contain a Bearer token (e.g., "Basic xyz" or malformed data), replaceFirst returns the original string unchanged. This passes potentially invalid or unexpected values to processAuthorization.
Consider returning null when the header doesn't match the expected Bearer format:
🔧 Proposed fix
private String extractBearerToken() {
String authHeader = httpHeaders.getHeaderString(HttpHeaders.AUTHORIZATION);
if (StringUtils.isEmpty(authHeader)) {
return null;
}
- return authHeader.replaceFirst("(?i)Bearer\\s+", "");
+ if (!authHeader.regionMatches(true, 0, "Bearer ", 0, 7)) {
+ return null;
+ }
+ return authHeader.substring(7).trim();
}Or using regex with explicit match check:
private String extractBearerToken() {
String authHeader = httpHeaders.getHeaderString(HttpHeaders.AUTHORIZATION);
if (StringUtils.isEmpty(authHeader)) {
return null;
}
- return authHeader.replaceFirst("(?i)Bearer\\s+", "");
+ if (authHeader.toLowerCase().startsWith("bearer ")) {
+ return authHeader.substring(7).trim();
+ }
+ return null;
}📝 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.
| private String extractBearerToken() { | |
| String authHeader = httpHeaders.getHeaderString(HttpHeaders.AUTHORIZATION); | |
| if (StringUtils.isEmpty(authHeader)) { | |
| return null; | |
| } | |
| return authHeader.replaceFirst("(?i)Bearer\\s+", ""); | |
| } | |
| private String extractBearerToken() { | |
| String authHeader = httpHeaders.getHeaderString(HttpHeaders.AUTHORIZATION); | |
| if (StringUtils.isEmpty(authHeader)) { | |
| return null; | |
| } | |
| if (!authHeader.regionMatches(true, 0, "Bearer ", 0, 7)) { | |
| return null; | |
| } | |
| return authHeader.substring(7).trim(); | |
| } |
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/filter/CedarlingAuthorizationProcessingFilter.java`
around lines 90 - 98, extractBearerToken currently returns the raw Authorization
header when it doesn't contain a Bearer token (e.g., "Basic ..."), which then
propagates invalid values to processAuthorization; update extractBearerToken to
explicitly validate the Bearer scheme before returning a token by checking the
header matches the Bearer pattern (case-insensitive) or starts with "Bearer "
ignoring case, then extract and trim the token, otherwise return null so only
valid Bearer tokens reach processAuthorization.
| /* | ||
| * Copyright [2025] [Janssen Project] | ||
| * Janssen Project software is available under the Apache License (2004). See http://www.apache.org/licenses/ for full text. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * Copyright (c) 2025, Janssen Project | ||
| */ |
There was a problem hiding this comment.
Non-standard Apache License reference.
The text "Apache License (2004)" is unusual—2004 is the year Apache License 2.0 was released, not part of the license name. The standard reference is "Apache License, Version 2.0".
Also, the copyright year (2025) doesn't reflect the original creation date (2022 per the @author tag). Consider using "2022, 2025" or "2022-2025" to indicate the full copyright span.
Suggested fix
/*
- * Janssen Project software is available under the Apache License (2004). See http://www.apache.org/licenses/ for full text.
+ * Janssen Project software is available under the Apache License, Version 2.0. See https://www.apache.org/licenses/LICENSE-2.0 for full text.
*
- * Copyright (c) 2025, Janssen Project
+ * Copyright (c) 2022-2025, Janssen Project
*/🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/cedarling/src/main/java/io/jans/lock/cedarling/service/inject/CedarlingPolicy.java`
around lines 1 - 5, Update the file header in CedarlingPolicy.java to use the
standard license name "Apache License, Version 2.0" instead of "Apache License
(2004)" and adjust the copyright years to reflect the creation-to-current span
(e.g., "2022-2025" or "2022, 2025"); modify only the file comment block at the
top of CedarlingPolicy.java to make these textual changes so the license and
copyright statements are accurate and standard.
| @PostConstruct | ||
| public void init() { | ||
| log.debug("Initializing GrpcAuditServiceProvider"); | ||
|
|
||
| // Create the gRPC service implementation | ||
| // This avoids CDI trying to proxy the class with final methods | ||
| grpcAuditService = new GrpcAuditServiceImpl(auditRestWebService, mapper, log); | ||
|
|
||
| log.info("GrpcAuditServiceImpl created successfully"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding null validation for injected dependencies.
If CDI injection fails for any dependency, the @PostConstruct method will throw a NullPointerException at line 45 with an unclear error message. Adding explicit checks would provide clearer diagnostics.
♻️ Proposed defensive check
`@PostConstruct`
public void init() {
log.debug("Initializing GrpcAuditServiceProvider");
+ if (auditRestWebService == null || mapper == null) {
+ log.error("Required dependencies not injected: auditRestWebService={}, mapper={}",
+ auditRestWebService, mapper);
+ throw new IllegalStateException("GrpcAuditServiceProvider dependencies not properly injected");
+ }
+
// Create the gRPC service implementation
// This avoids CDI trying to proxy the class with final methods
grpcAuditService = new GrpcAuditServiceImpl(auditRestWebService, mapper, log);🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/audit/GrpcAuditServiceProvider.java`
around lines 39 - 48, The init() method in GrpcAuditServiceProvider should
validate injected dependencies before using them: check that
auditRestWebService, mapper, and log are not null and throw a clear
IllegalStateException (or log an explicit error) if any are null, then only
instantiate grpcAuditService = new GrpcAuditServiceImpl(...). Add these null
checks at the start of init() to fail-fast with a descriptive message
referencing the missing dependency names so the cause is clear when CDI
injection fails.
| private static final Metadata.Key<String> AUTHORIZATION_METADATA_KEY = | ||
| Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused constant AUTHORIZATION_METADATA_KEY.
This constant is defined but never used; the code uses HeaderUtils.findAndExtractBearerToken(headers) instead of directly accessing the authorization metadata key.
♻️ Proposed fix
- private static final Metadata.Key<String> AUTHORIZATION_METADATA_KEY =
- Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER);
-
`@Override`
public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(📝 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.
| private static final Metadata.Key<String> AUTHORIZATION_METADATA_KEY = | |
| Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER); | |
| `@Override` | |
| public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall( |
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java`
around lines 65 - 66, The constant AUTHORIZATION_METADATA_KEY in
GrpcAuthorizationInterceptor is unused (the code calls
HeaderUtils.findAndExtractBearerToken(headers) instead); remove the unused field
declaration to clean up the class and imports, and ensure no other references
remain to AUTHORIZATION_METADATA_KEY after deletion so compilation is
unaffected.
| @PreDestroy | ||
| public void stopGrpcServer() { | ||
| if (grpcServer != null) { | ||
| log.info("Stopping gRPC server..."); | ||
| grpcServer.shutdown(); | ||
|
|
||
| try { | ||
| if (!grpcServer.awaitTermination(30, TimeUnit.SECONDS)) { | ||
| log.warn("gRPC server did not terminate gracefully, forcing shutdown"); | ||
| grpcServer.shutdownNow(); | ||
|
|
||
| if (!grpcServer.awaitTermination(10, TimeUnit.SECONDS)) { | ||
| log.error("gRPC server did not terminate"); | ||
| } | ||
| } | ||
|
|
||
| log.info("gRPC server stopped"); | ||
| } catch (InterruptedException e) { | ||
| log.error("gRPC server shutdown was interrupted", e); | ||
| // Restore interrupt status | ||
| Thread.currentThread().interrupt(); | ||
| // Force shutdown | ||
| grpcServer.shutdownNow(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider nulling grpcServer after shutdown to allow future restarts.
This prevents stale state if the server is stopped and later reinitialized (tests, dynamic reloads, etc.).
♻️ Suggested tweak
} catch (InterruptedException e) {
log.error("gRPC server shutdown was interrupted", e);
Thread.currentThread().interrupt();
grpcServer.shutdownNow();
}
+ grpcServer = null;
}📝 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.
| @PreDestroy | |
| public void stopGrpcServer() { | |
| if (grpcServer != null) { | |
| log.info("Stopping gRPC server..."); | |
| grpcServer.shutdown(); | |
| try { | |
| if (!grpcServer.awaitTermination(30, TimeUnit.SECONDS)) { | |
| log.warn("gRPC server did not terminate gracefully, forcing shutdown"); | |
| grpcServer.shutdownNow(); | |
| if (!grpcServer.awaitTermination(10, TimeUnit.SECONDS)) { | |
| log.error("gRPC server did not terminate"); | |
| } | |
| } | |
| log.info("gRPC server stopped"); | |
| } catch (InterruptedException e) { | |
| log.error("gRPC server shutdown was interrupted", e); | |
| // Restore interrupt status | |
| Thread.currentThread().interrupt(); | |
| // Force shutdown | |
| grpcServer.shutdownNow(); | |
| } | |
| } | |
| } | |
| `@PreDestroy` | |
| public void stopGrpcServer() { | |
| if (grpcServer != null) { | |
| log.info("Stopping gRPC server..."); | |
| grpcServer.shutdown(); | |
| try { | |
| if (!grpcServer.awaitTermination(30, TimeUnit.SECONDS)) { | |
| log.warn("gRPC server did not terminate gracefully, forcing shutdown"); | |
| grpcServer.shutdownNow(); | |
| if (!grpcServer.awaitTermination(10, TimeUnit.SECONDS)) { | |
| log.error("gRPC server did not terminate"); | |
| } | |
| } | |
| log.info("gRPC server stopped"); | |
| } catch (InterruptedException e) { | |
| log.error("gRPC server shutdown was interrupted", e); | |
| // Restore interrupt status | |
| Thread.currentThread().interrupt(); | |
| // Force shutdown | |
| grpcServer.shutdownNow(); | |
| } | |
| grpcServer = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/server/GrpcServerStarter.java`
around lines 140 - 165, The stopGrpcServer method leaves the grpcServer field
non-null after shutdown, which can cause stale state on later restarts; update
stopGrpcServer to set grpcServer = null after a successful shutdown path and
also after the InterruptedException handling (i.e., after forcing shutdown in
both the normal and catch paths) so that the grpcServer reference is cleared for
future reinitialization; locate the method stopGrpcServer and the grpcServer
field to add the null assignment in all exit paths.
| * Janssen Project software is available under the Apache License (2004). See http://www.apache.org/licenses/ for full text. | ||
| * | ||
| * Copyright (c) 2020, Janssen Project | ||
| * Copyright (c) 2022, Janssen Project |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for inconsistent license headers in jans-lock module
# Search for MIT license headers
echo "=== Files with MIT license ==="
rg -l "MIT License|MIT \(" jans-lock/ 2>/dev/null | head -20
# Search for Apache license headers
echo -e "\n=== Files with Apache license ==="
rg -l "Apache License" jans-lock/ 2>/dev/null | head -20
# Check specific file
echo -e "\n=== License headers in OpenIdService.java ==="
head -10 jans-lock/lock-server/service/src/main/java/io/jans/lock/service/OpenIdService.java
# Check other files in the same package
echo -e "\n=== Other files in the same package ==="
find jans-lock/lock-server/service/src/main/java/io/jans/lock/service/ -name "*.java" -type f | head -10 | while read file; do
echo "File: $file"
head -5 "$file" | grep -E "Copyright|License|Apache|MIT" || echo " (no license header found)"
done
# Check copyright year distribution in jans-lock
echo -e "\n=== Copyright year distribution in jans-lock ==="
rg "Copyright \(c\) [0-9]{4}" jans-lock/ -o 2>/dev/null | sed 's/.*Copyright (c) //' | sort | uniq -c | sort -rn | head -20Repository: JanssenProject/jans
Length of output: 4404
Update copyright year and standardize license header format.
The copyright year is set to 2022, but the predominant copyright year across the jans-lock module is 2025. Update to 2025 (or use a range "2022-2025" if the file is being modified now). Additionally, the license header format is outdated; align it with the standard Apache License, Version 2.0 format used in other recent files (e.g., ConfigurationService.java, ConfigurationFactory.java) for consistency across the codebase.
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/OpenIdService.java`
around lines 2 - 4, Update the file header in OpenIdService.java to use the
standardized Apache License, Version 2.0 header used elsewhere (e.g.,
ConfigurationService.java) and change the copyright year to 2025 (or use the
range "2022-2025" if you prefer); specifically replace the existing top-of-file
block comment with the current project license template and updated year(s) so
the class OpenIdService reflects the consistent license header format across the
codebase.
| syntax = "proto3"; | ||
|
|
||
| package io.jans.lock.audit; | ||
|
|
||
| option java_multiple_files = true; | ||
| option java_package = "io.jans.lock.model.audit.grpc"; | ||
| option java_outer_classname = "AuditProto"; | ||
|
|
||
| import "google/protobuf/timestamp.proto"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Proto package directory mismatch.
Static analysis (Buf) reports: Files with package io.jans.lock.audit must be within a directory io/jans/lock/audit relative to root but were in directory jans-lock/lock-server/service/src/main/proto/grpc.
This is a Buf lint recommendation. If using Buf for CI, either relocate the file or configure Buf to allow this directory structure.
🧰 Tools
🪛 Buf (1.63.0)
3-3: Files with package "io.jans.lock.audit" must be within a directory "io/jans/lock/audit" relative to root but were in directory "jans-lock/lock-server/service/src/main/proto/grpc".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In `@jans-lock/lock-server/service/src/main/proto/grpc/audit.proto` around lines 1
- 9, The proto declares package io.jans.lock.audit (and outer classname
AuditProto) but the file lives outside the expected io/jans/lock/audit
directory, causing Buf lint failures; fix by either moving this proto file into
the repository proto root subdirectory path io/jans/lock/audit so the filesystem
matches the declared package, or update the package declaration to match the
current directory layout (and update references to AuditProto accordingly);
alternatively, if you intentionally keep the current layout, add/configure
buf.yaml to allow this directory mapping so Buf won’t reject files with package
io.jans.lock.audit.
Signed-off-by: Yuriy <Yuriy.Movchan@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh`:
- Around line 11-15: Replace the hardcoded placeholders (HOST, GRPC_ADDR,
CLIENT_ID, CLIENT_SECRET, INVALID_TOKEN) with environment-driven values: read
each from process/env (e.g., HOST=${HOST:-} style) and enforce required checks
so the script exits with a clear error if any required variable (CLIENT_ID,
CLIENT_SECRET, HOST) is missing; allow an optional override for INVALID_TOKEN
and GRPC_ADDR, and document sourcing a local .env or exporting env vars in the
script header so secrets are never committed.
- Around line 171-239: The failure handlers currently echo the raw command and
output (see functions check_http_success, check_http_should_fail,
check_grpc_success, check_grpc_should_fail), which can leak Authorization:
Bearer tokens; sanitize before printing by constructing a safe_cmd that pipes
the original cmd through a redaction filter (e.g., sed to replace
'Authorization: Bearer <token>' with 'Authorization: Bearer <redacted>' and/or
remove '--header "Authorization: Bearer ..."' portions) and use safe_cmd in all
"Command:" logs, and similarly run the printed output through a redaction step
(e.g., safe_output=$(echo "$output" | sed -E 's/(Authorization:
Bearer)[[:space:]]+[^[:space:]]+/\1 <redacted>/gi')) before echoing "Output:" so
no bearer tokens appear in logs; update all places that echo $cmd or echo
"$output" in those four functions to use the redacted variables.
- Around line 202-218: The two bare redirections in the helper functions (the
standalone “> res.txt” lines immediately before the if eval blocks in the
functions such as check_grpc_should_fail) should be replaced with an explicit
no-op command to silence ShellCheck SC2188; update those lines to use a no-op
like : > res.txt (or true > res.txt) so the intent to truncate the file is
explicit while keeping the rest of the function (the eval "$cmd" >res.txt 2>&1
logic and the function names check_grpc_should_fail / the corresponding
pass-check function) unchanged.
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java`:
- Around line 212-236: The inner class GrpcResourceInfo currently holds an
implicit reference to its enclosing GrpcAuthorizationInterceptor instance;
declare it as a static nested class (change "class GrpcResourceInfo" to "static
class GrpcResourceInfo") so it no longer captures the outer instance, keeping
constructors and methods (getResourceMethod, getResourceClass, toString)
unchanged; ensure imports and usages inside GrpcAuthorizationInterceptor still
compile after making GrpcResourceInfo static.
- Around line 169-201: extractResourceInfo currently only inspects
AuditRestWebService.class and is therefore not extensible; change it to accept
or obtain a collection of service classes (e.g., pass a List<Class<?>> or use a
ServiceLoader/registry) and iterate over those when calling
getProtectionApiMethod so new gRPC service classes can be discovered without
modifying this method. In getProtectionApiMethod, replace
Optional.ofNullable(grpcResourceInfo) with Optional.of(grpcResourceInfo) because
grpcResourceInfo is never null at that point; keep referencing the methods
extractResourceInfo, getProtectionApiMethod, AuditRestWebService, and
GrpcResourceInfo to locate the changes.
- Around line 123-131: The catch block in GrpcAuthorizationInterceptor that logs
and closes the call on exceptions should also record an audit entry like the
existing authorization-failure path: call the same audit logging routine used
for authorization failures (use the same audit service/method invoked for
failures) with the methodName, the exception message and exception details
before or immediately after log.error and before call.close; ensure the audit
entry marks it as an exception/internal error and includes sufficient context
(methodName and exception) so it appears in the security audit trail.
♻️ Duplicate comments (3)
jans-lock/lock-server/service/src/main/java/io/jans/lock/util/ServerUtil.java (1)
146-173: Fix null pointer dereference and reverse IP resolution priority for security.This code has two issues flagged in a previous review that remain unaddressed:
NPE risk (line 161):
inetAddr.getAddress()returnsnullfor unresolved addresses, causingNullPointerExceptionwhengetHostAddress()is called.Security concern (lines 148-153 vs 155-165): Client-controllable headers (
X-Forwarded-For,X-Real-IP) are checked before the trusted socket address. Since this IP is used for authorization decisions inGrpcAuthorizationInterceptor, header spoofing is a security risk unless behind a trusted proxy. Reverse the priority to prefer socket address.🔧 Proposed fix for null-safety and priority order
public static String getGrpcClientIpAddress(io.grpc.ServerCall<?, ?> call, Metadata headers) { try { - // Method 1: Try to get from metadata headers - String ipFromHeaders = getIpFromHeaders(headers); - if (ipFromHeaders != null) { - log.debug("Client IP from headers: {}", ipFromHeaders); - return ipFromHeaders; - } - - // Method 2: Get from ServerCall attributes + // Method 1: Get from ServerCall attributes (trusted source) if (call != null) { io.grpc.Attributes attributes = call.getAttributes(); java.net.SocketAddress remoteAddr = attributes.get(io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR); if (remoteAddr instanceof java.net.InetSocketAddress) { java.net.InetSocketAddress inetAddr = (java.net.InetSocketAddress) remoteAddr; - String ip = inetAddr.getAddress().getHostAddress(); + java.net.InetAddress addr = inetAddr.getAddress(); + String ip = (addr != null) ? addr.getHostAddress() : inetAddr.getHostString(); log.debug("Client IP from ServerCall attributes: {}", ip); return ip; } } + // Method 2: Fall back to headers (only if behind trusted proxy) + String ipFromHeaders = getIpFromHeaders(headers); + if (ipFromHeaders != null) { + log.debug("Client IP from headers: {}", ipFromHeaders); + return ipFromHeaders; + } + log.warn("Could not determine client IP address"); return UNKNOWN; } catch (Exception e) {jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java (2)
65-66: Remove unused constantAUTHORIZATION_METADATA_KEY.This constant is declared but never referenced. The code uses
HeaderUtils.findAndExtractBearerToken(headers)at line 103 instead.♻️ Proposed fix
- private static final Metadata.Key<String> AUTHORIZATION_METADATA_KEY = - Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER); - `@Override` public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
94-103: Add null guard: deny access whenResourceInfocannot be resolved.
extractResourceInfo()returnsnullwhen the gRPC method name format is invalid (line 174) or when no@ProtectedApimapping exists (line 186). PassingnulltoprocessAuthorization()will causeNullPointerExceptionin bothOpenIdProtectionServiceandCedarlingProtectionServicewhen they callresourceInfo.getResourceMethod().Fail-closed by rejecting requests when resource metadata cannot be resolved.
🔧 Proposed fix
try { // Extract method-specific ResourceInfo ResourceInfo resourceInfo = extractResourceInfo(methodName); + if (resourceInfo == null) { + log.warn("No ResourceInfo mapping for gRPC method: {}", methodName); + + String clientIp = ServerUtil.getGrpcClientIpAddress(call, headers); + AuditLogEntry auditLogEntry = new AuditLogEntry(clientIp, AuditActionType.GRPC_AUTHZ_FILTER); + applicationAuditLogger.log(auditLogEntry, false); + + call.close(Status.PERMISSION_DENIED.withDescription("Authorization error: unknown method"), new Metadata()); + return new ServerCall.Listener<ReqT>() {}; + } log.debug("gRPC call requires access to: {}", resourceInfo);
| HOST="https://server.jans.info" | ||
| GRPC_ADDR="server.jans.info:443" | ||
| CLIENT_ID="2200...." | ||
| CLIENT_SECRET="..." | ||
| INVALID_TOKEN="this-is-definitely-not-a-valid-token" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Externalize host and credentials to avoid accidental secret commits.
Hardcoded placeholders invite local edits that can leak into commits. Prefer env overrides with required checks.
💡 Proposed refactor
-HOST="https://server.jans.info"
-GRPC_ADDR="server.jans.info:443"
-CLIENT_ID="2200...."
-CLIENT_SECRET="..."
+HOST="${HOST:-https://server.jans.info}"
+GRPC_ADDR="${GRPC_ADDR:-server.jans.info:443}"
+: "${CLIENT_ID:?Set CLIENT_ID in env}"
+: "${CLIENT_SECRET:?Set CLIENT_SECRET in env}"📝 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.
| HOST="https://server.jans.info" | |
| GRPC_ADDR="server.jans.info:443" | |
| CLIENT_ID="2200...." | |
| CLIENT_SECRET="..." | |
| INVALID_TOKEN="this-is-definitely-not-a-valid-token" | |
| HOST="${HOST:-https://server.jans.info}" | |
| GRPC_ADDR="${GRPC_ADDR:-server.jans.info:443}" | |
| : "${CLIENT_ID:?Set CLIENT_ID in env}" | |
| : "${CLIENT_SECRET:?Set CLIENT_SECRET in env}" | |
| INVALID_TOKEN="this-is-definitely-not-a-valid-token" |
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh`
around lines 11 - 15, Replace the hardcoded placeholders (HOST, GRPC_ADDR,
CLIENT_ID, CLIENT_SECRET, INVALID_TOKEN) with environment-driven values: read
each from process/env (e.g., HOST=${HOST:-} style) and enforce required checks
so the script exits with a clear error if any required variable (CLIENT_ID,
CLIENT_SECRET, HOST) is missing; allow an optional override for INVALID_TOKEN
and GRPC_ADDR, and document sourcing a local .env or exporting env vars in the
script header so secrets are never committed.
| check_http_success() { | ||
| local cmd="$1" | ||
| local desc="$2" | ||
| local status | ||
| status=$(eval "$cmd" -s -o /dev/null -w "%{http_code}") | ||
| if [[ $status -ge 200 && $status -lt 300 ]]; then | ||
| echo "✅ $desc (HTTP $status)" | ||
| else | ||
| echo "❌ $desc → HTTP $status" | ||
| echo "Command: $cmd" | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| check_http_should_fail() { | ||
| local cmd="$1" | ||
| local desc="$2" | ||
| local status | ||
| status=$(eval "$cmd" -s -o /dev/null -w "%{http_code}") | ||
| if [[ $status -ge 401 && $status -le 403 ]]; then | ||
| echo "✅ (expected failure) $desc (HTTP $status)" | ||
| else | ||
| echo "❌ UNEXPECTED success → $desc (HTTP $status)" | ||
| echo "Command: $cmd" | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| check_grpc_success() { | ||
| local cmd="$1" | ||
| local desc="$2" | ||
| > res.txt | ||
| if eval "$cmd" >res.txt 2>&1; then | ||
| echo "✅ $desc" | ||
| else | ||
| echo "❌ $desc" | ||
| echo "Command: $cmd" | ||
| echo "Output:" | ||
| cat res.txt | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| check_grpc_should_fail() { | ||
| local cmd="$1" | ||
| local desc="$2" | ||
| > res.txt | ||
| if eval "$cmd" >res.txt 2>&1; then | ||
| echo "❌ UNEXPECTED success → $desc" | ||
| echo "Command: $cmd" | ||
| echo "Output:" | ||
| cat res.txt | ||
| return 1 | ||
| else | ||
| local output | ||
| output=$(cat res.txt) | ||
| if echo "$output" | grep -q -i "PermissionDenied" || echo "$output" | grep -q -i "Invalid token" || echo "$output" | grep -q -i "unauthenticated"; then | ||
| local code | ||
| code=$(echo "$output" | grep -i "Code:" | head -n 1 | sed -E 's/.*Code:[[:space:]]*([^[:space:],().]+).*/\1/i') | ||
| echo "✅ (expected failure) $desc with status: $code" | ||
| else | ||
| echo "❌ UNEXPECTED gRPC error → $desc" | ||
| echo "Command: $cmd" | ||
| echo "Output:" | ||
| echo "$output" | ||
| return 1 | ||
| fi | ||
| fi | ||
| } |
There was a problem hiding this comment.
Redact bearer tokens before printing failed commands.
On failures, the script prints full commands including Authorization: Bearer …, which leaks credentials to logs/terminals.
🔒 Proposed fix
+redact_cmd() {
+ sed -E 's/(Authorization: Bearer|authorization: bearer)[[:space:]]+[A-Za-z0-9._-]+/\1 <redacted>/g' <<<"$1"
+}
+
check_http_success() {
local cmd="$1"
local desc="$2"
local status
status=$(eval "$cmd" -s -o /dev/null -w "%{http_code}")
if [[ $status -ge 200 && $status -lt 300 ]]; then
echo "✅ $desc (HTTP $status)"
else
echo "❌ $desc → HTTP $status"
- echo "Command: $cmd"
+ echo "Command: $(redact_cmd "$cmd")"
return 1
fi
}
@@
if [[ $status -ge 401 && $status -le 403 ]]; then
echo "✅ (expected failure) $desc (HTTP $status)"
else
echo "❌ UNEXPECTED success → $desc (HTTP $status)"
- echo "Command: $cmd"
+ echo "Command: $(redact_cmd "$cmd")"
return 1
fi
}
@@
if eval "$cmd" >res.txt 2>&1; then
echo "✅ $desc"
else
echo "❌ $desc"
- echo "Command: $cmd"
+ echo "Command: $(redact_cmd "$cmd")"
echo "Output:"
cat res.txt
return 1
fi
}
@@
if eval "$cmd" >res.txt 2>&1; then
echo "❌ UNEXPECTED success → $desc"
- echo "Command: $cmd"
+ echo "Command: $(redact_cmd "$cmd")"
echo "Output:"
cat res.txt
return 1
else
@@
else
echo "❌ UNEXPECTED gRPC error → $desc"
- echo "Command: $cmd"
+ echo "Command: $(redact_cmd "$cmd")"
echo "Output:"
echo "$output"
return 1
fi
fi
}📝 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.
| check_http_success() { | |
| local cmd="$1" | |
| local desc="$2" | |
| local status | |
| status=$(eval "$cmd" -s -o /dev/null -w "%{http_code}") | |
| if [[ $status -ge 200 && $status -lt 300 ]]; then | |
| echo "✅ $desc (HTTP $status)" | |
| else | |
| echo "❌ $desc → HTTP $status" | |
| echo "Command: $cmd" | |
| return 1 | |
| fi | |
| } | |
| check_http_should_fail() { | |
| local cmd="$1" | |
| local desc="$2" | |
| local status | |
| status=$(eval "$cmd" -s -o /dev/null -w "%{http_code}") | |
| if [[ $status -ge 401 && $status -le 403 ]]; then | |
| echo "✅ (expected failure) $desc (HTTP $status)" | |
| else | |
| echo "❌ UNEXPECTED success → $desc (HTTP $status)" | |
| echo "Command: $cmd" | |
| return 1 | |
| fi | |
| } | |
| check_grpc_success() { | |
| local cmd="$1" | |
| local desc="$2" | |
| > res.txt | |
| if eval "$cmd" >res.txt 2>&1; then | |
| echo "✅ $desc" | |
| else | |
| echo "❌ $desc" | |
| echo "Command: $cmd" | |
| echo "Output:" | |
| cat res.txt | |
| return 1 | |
| fi | |
| } | |
| check_grpc_should_fail() { | |
| local cmd="$1" | |
| local desc="$2" | |
| > res.txt | |
| if eval "$cmd" >res.txt 2>&1; then | |
| echo "❌ UNEXPECTED success → $desc" | |
| echo "Command: $cmd" | |
| echo "Output:" | |
| cat res.txt | |
| return 1 | |
| else | |
| local output | |
| output=$(cat res.txt) | |
| if echo "$output" | grep -q -i "PermissionDenied" || echo "$output" | grep -q -i "Invalid token" || echo "$output" | grep -q -i "unauthenticated"; then | |
| local code | |
| code=$(echo "$output" | grep -i "Code:" | head -n 1 | sed -E 's/.*Code:[[:space:]]*([^[:space:],().]+).*/\1/i') | |
| echo "✅ (expected failure) $desc with status: $code" | |
| else | |
| echo "❌ UNEXPECTED gRPC error → $desc" | |
| echo "Command: $cmd" | |
| echo "Output:" | |
| echo "$output" | |
| return 1 | |
| fi | |
| fi | |
| } | |
| redact_cmd() { | |
| sed -E 's/(Authorization: Bearer|authorization: bearer)[[:space:]]+[A-Za-z0-9._-]+/\1 <redacted>/g' <<<"$1" | |
| } | |
| check_http_success() { | |
| local cmd="$1" | |
| local desc="$2" | |
| local status | |
| status=$(eval "$cmd" -s -o /dev/null -w "%{http_code}") | |
| if [[ $status -ge 200 && $status -lt 300 ]]; then | |
| echo "✅ $desc (HTTP $status)" | |
| else | |
| echo "❌ $desc → HTTP $status" | |
| echo "Command: $(redact_cmd "$cmd")" | |
| return 1 | |
| fi | |
| } | |
| check_http_should_fail() { | |
| local cmd="$1" | |
| local desc="$2" | |
| local status | |
| status=$(eval "$cmd" -s -o /dev/null -w "%{http_code}") | |
| if [[ $status -ge 401 && $status -le 403 ]]; then | |
| echo "✅ (expected failure) $desc (HTTP $status)" | |
| else | |
| echo "❌ UNEXPECTED success → $desc (HTTP $status)" | |
| echo "Command: $(redact_cmd "$cmd")" | |
| return 1 | |
| fi | |
| } | |
| check_grpc_success() { | |
| local cmd="$1" | |
| local desc="$2" | |
| > res.txt | |
| if eval "$cmd" >res.txt 2>&1; then | |
| echo "✅ $desc" | |
| else | |
| echo "❌ $desc" | |
| echo "Command: $(redact_cmd "$cmd")" | |
| echo "Output:" | |
| cat res.txt | |
| return 1 | |
| fi | |
| } | |
| check_grpc_should_fail() { | |
| local cmd="$1" | |
| local desc="$2" | |
| > res.txt | |
| if eval "$cmd" >res.txt 2>&1; then | |
| echo "❌ UNEXPECTED success → $desc" | |
| echo "Command: $(redact_cmd "$cmd")" | |
| echo "Output:" | |
| cat res.txt | |
| return 1 | |
| else | |
| local output | |
| output=$(cat res.txt) | |
| if echo "$output" | grep -q -i "PermissionDenied" || echo "$output" | grep -q -i "Invalid token" || echo "$output" | grep -q -i "unauthenticated"; then | |
| local code | |
| code=$(echo "$output" | grep -i "Code:" | head -n 1 | sed -E 's/.*Code:[[:space:]]*([^[:space:],().]+).*/\1/i') | |
| echo "✅ (expected failure) $desc with status: $code" | |
| else | |
| echo "❌ UNEXPECTED gRPC error → $desc" | |
| echo "Command: $(redact_cmd "$cmd")" | |
| echo "Output:" | |
| echo "$output" | |
| return 1 | |
| fi | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 202-202: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
[warning] 217-217: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh`
around lines 171 - 239, The failure handlers currently echo the raw command and
output (see functions check_http_success, check_http_should_fail,
check_grpc_success, check_grpc_should_fail), which can leak Authorization:
Bearer tokens; sanitize before printing by constructing a safe_cmd that pipes
the original cmd through a redaction filter (e.g., sed to replace
'Authorization: Bearer <token>' with 'Authorization: Bearer <redacted>' and/or
remove '--header "Authorization: Bearer ..."' portions) and use safe_cmd in all
"Command:" logs, and similarly run the printed output through a redaction step
(e.g., safe_output=$(echo "$output" | sed -E 's/(Authorization:
Bearer)[[:space:]]+[^[:space:]]+/\1 <redacted>/gi')) before echoing "Output:" so
no bearer tokens appear in logs; update all places that echo $cmd or echo
"$output" in those four functions to use the redacted variables.
| > res.txt | ||
| if eval "$cmd" >res.txt 2>&1; then | ||
| echo "✅ $desc" | ||
| else | ||
| echo "❌ $desc" | ||
| echo "Command: $cmd" | ||
| echo "Output:" | ||
| cat res.txt | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| check_grpc_should_fail() { | ||
| local cmd="$1" | ||
| local desc="$2" | ||
| > res.txt | ||
| if eval "$cmd" >res.txt 2>&1; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and examine the relevant lines
if [ -f "jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh" ]; then
echo "=== File found. Showing lines 200-220 ==="
sed -n '200,220p' jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh | cat -n
else
echo "File not found at expected path"
fiRepository: JanssenProject/jans
Length of output: 639
🏁 Script executed:
#!/bin/bash
# Run Shellcheck to identify SC2188 warnings
shellcheck -f gcc jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh 2>&1 | grep -i "SC2188" || echo "No SC2188 warnings found, or checking all warnings:"Repository: JanssenProject/jans
Length of output: 444
🏁 Script executed:
#!/bin/bash
# Get full shellcheck output focusing on the specific lines mentioned
shellcheck jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh 2>&1Repository: JanssenProject/jans
Length of output: 611
Fix bare redirections flagged by Shellcheck (SC2188).
Lines 202 and 217 contain bare redirections (> res.txt) without a command. Use a no-op command to make intent explicit and silence the warning.
🧹 Proposed fix
- > res.txt
+ : > res.txt🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 202-202: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
[warning] 217-217: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/server/src/test/manual/automatic_audit_enpoints_tests.sh`
around lines 202 - 218, The two bare redirections in the helper functions (the
standalone “> res.txt” lines immediately before the if eval blocks in the
functions such as check_grpc_should_fail) should be replaced with an explicit
no-op command to silence ShellCheck SC2188; update those lines to use a no-op
like : > res.txt (or true > res.txt) so the intent to truncate the file is
explicit while keeping the rest of the function (the eval "$cmd" >res.txt 2>&1
logic and the function names check_grpc_should_fail / the corresponding
pass-check function) unchanged.
| } catch (Exception e) { | ||
| log.error("Error during gRPC authorization for '{}'", methodName, e); | ||
|
|
||
| call.close(Status.INTERNAL | ||
| .withDescription("Authorization error: " + e.getMessage()) | ||
| .withCause(e), new Metadata()); | ||
|
|
||
| return new ServerCall.Listener<ReqT>() {}; | ||
| } |
There was a problem hiding this comment.
Add audit logging for exception cases.
Authorization failures at line 108 are audited, but exceptions caught here are not. For complete security audit trails, exception cases should also be logged.
🔧 Proposed fix
} catch (Exception e) {
log.error("Error during gRPC authorization for '{}'", methodName, e);
+ // Audit the failed authorization attempt
+ String clientIp = ServerUtil.getGrpcClientIpAddress(call, headers);
+ AuditLogEntry auditLogEntry = new AuditLogEntry(clientIp, AuditActionType.GRPC_AUTHZ_FILTER);
+ applicationAuditLogger.log(auditLogEntry, false);
+
call.close(Status.INTERNAL
.withDescription("Authorization error: " + e.getMessage())
.withCause(e), new Metadata());🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java`
around lines 123 - 131, The catch block in GrpcAuthorizationInterceptor that
logs and closes the call on exceptions should also record an audit entry like
the existing authorization-failure path: call the same audit logging routine
used for authorization failures (use the same audit service/method invoked for
failures) with the methodName, the exception message and exception details
before or immediately after log.error and before call.close; ensure the audit
entry marks it as an exception/internal error and includes sufficient context
(methodName and exception) so it appears in the security audit trail.
| private ResourceInfo extractResourceInfo(String methodName) { | ||
| // Parse method name: "package.Service/Method" | ||
| String[] parts = methodName.split("/"); | ||
| if (parts.length != 2) { | ||
| log.warn("Invalid gRPC method name format: {}", methodName); | ||
| return null; | ||
| } | ||
|
|
||
| String method = parts[1]; | ||
|
|
||
| // Map gRPC methods to ResourceInfo (same as REST API) | ||
| Optional<ResourceInfo> resourceInfo = getProtectionApiMethod(AuditRestWebService.class, method); | ||
| if (resourceInfo.isPresent()) { | ||
| return resourceInfo.get(); | ||
| } | ||
|
|
||
| log.warn("No ResourceInfo found for gRPC method: {}", methodName); | ||
| return null; | ||
| } | ||
|
|
||
| private Optional<ResourceInfo> getProtectionApiMethod(Class<?> clazz, String grpcMethodName) { | ||
| for (Method method : clazz.getMethods()) { | ||
| Optional<ProtectedApi> protectedApi = getProtectedApiAnnotation(method); | ||
| if (protectedApi.isPresent()) { | ||
| if (grpcMethodName.equals(protectedApi.get().grpcMethodName())) { | ||
| GrpcResourceInfo grpcResourceInfo = new GrpcResourceInfo(clazz, method); | ||
| return Optional.ofNullable(grpcResourceInfo); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making service class discovery extensible.
extractResourceInfo only searches AuditRestWebService.class (line 180). If additional gRPC services are added later, this method requires modification. Consider accepting a list of service classes or using service discovery.
Also, line 195 uses Optional.ofNullable(grpcResourceInfo) but grpcResourceInfo is never null at that point—Optional.of() would be more precise.
♻️ Proposed minor fix for Optional usage
if (protectedApi.isPresent()) {
if (grpcMethodName.equals(protectedApi.get().grpcMethodName())) {
GrpcResourceInfo grpcResourceInfo = new GrpcResourceInfo(clazz, method);
- return Optional.ofNullable(grpcResourceInfo);
+ return Optional.of(grpcResourceInfo);
}
}🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java`
around lines 169 - 201, extractResourceInfo currently only inspects
AuditRestWebService.class and is therefore not extensible; change it to accept
or obtain a collection of service classes (e.g., pass a List<Class<?>> or use a
ServiceLoader/registry) and iterate over those when calling
getProtectionApiMethod so new gRPC service classes can be discovered without
modifying this method. In getProtectionApiMethod, replace
Optional.ofNullable(grpcResourceInfo) with Optional.of(grpcResourceInfo) because
grpcResourceInfo is never null at that point; keep referencing the methods
extractResourceInfo, getProtectionApiMethod, AuditRestWebService, and
GrpcResourceInfo to locate the changes.
| class GrpcResourceInfo implements ResourceInfo { | ||
|
|
||
| private Class<?> clazz; | ||
| private Method method; | ||
|
|
||
| public GrpcResourceInfo( Class<?> clazz, Method method) { | ||
| this.clazz = clazz; | ||
| this.method = method; | ||
| } | ||
|
|
||
| @Override | ||
| public Method getResourceMethod() { | ||
| return method; | ||
| } | ||
|
|
||
| @Override | ||
| public Class<?> getResourceClass() { | ||
| return clazz; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "GrpcResourceInfo [clazz=" + clazz + ", method=" + method + "]"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make inner class static.
GrpcResourceInfo doesn't access any instance members of GrpcAuthorizationInterceptor, so it should be declared static. This avoids the implicit reference to the enclosing instance and is the recommended pattern for inner classes that don't need it.
♻️ Proposed fix
- class GrpcResourceInfo implements ResourceInfo {
+ static class GrpcResourceInfo implements ResourceInfo {📝 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.
| class GrpcResourceInfo implements ResourceInfo { | |
| private Class<?> clazz; | |
| private Method method; | |
| public GrpcResourceInfo( Class<?> clazz, Method method) { | |
| this.clazz = clazz; | |
| this.method = method; | |
| } | |
| @Override | |
| public Method getResourceMethod() { | |
| return method; | |
| } | |
| @Override | |
| public Class<?> getResourceClass() { | |
| return clazz; | |
| } | |
| @Override | |
| public String toString() { | |
| return "GrpcResourceInfo [clazz=" + clazz + ", method=" + method + "]"; | |
| } | |
| } | |
| static class GrpcResourceInfo implements ResourceInfo { | |
| private Class<?> clazz; | |
| private Method method; | |
| public GrpcResourceInfo( Class<?> clazz, Method method) { | |
| this.clazz = clazz; | |
| this.method = method; | |
| } | |
| `@Override` | |
| public Method getResourceMethod() { | |
| return method; | |
| } | |
| `@Override` | |
| public Class<?> getResourceClass() { | |
| return clazz; | |
| } | |
| `@Override` | |
| public String toString() { | |
| return "GrpcResourceInfo [clazz=" + clazz + ", method=" + method + "]"; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@jans-lock/lock-server/service/src/main/java/io/jans/lock/service/grpc/security/GrpcAuthorizationInterceptor.java`
around lines 212 - 236, The inner class GrpcResourceInfo currently holds an
implicit reference to its enclosing GrpcAuthorizationInterceptor instance;
declare it as a static nested class (change "class GrpcResourceInfo" to "static
class GrpcResourceInfo") so it no longer captures the outer instance, keeping
constructors and methods (getResourceMethod, getResourceClass, toString)
unchanged; ensure imports and usages inside GrpcAuthorizationInterceptor still
compile after making GrpcResourceInfo static.
##13062
Closes #13062,
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.