[ES-2740] [ES-2741] [ES-2742] Validations of Open-Id profile configurations #1573
Conversation
WalkthroughAdds server-profile driven OpenID feature support: new DB table and migrations, JPA entity and repository, SPI and helper to load profile features, threading of profile-derived config into authorization/OIDC/token flows, schema/config updates, and related tests and migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthSvc as AuthorizationServiceImpl
participant AuthHelper as AuthorizationHelperService
participant Repo as ServerProfileRepository
participant DB as Database
Client->>AuthSvc: /authorize request
activate AuthSvc
AuthSvc->>AuthSvc: read mosip.esignet.server.profile
AuthSvc->>AuthHelper: getFeaturesByProfileName(profile)
activate AuthHelper
AuthHelper->>Repo: findByProfileName(profileName)
activate Repo
Note over Repo: `@Cacheable` "serverprofile"
Repo->>DB: query openid_profile
DB-->>Repo: list of ServerProfile rows
Repo-->>AuthHelper: ServerProfile list
deactivate Repo
AuthHelper-->>AuthSvc: Map<String,String> features
deactivate AuthHelper
AuthSvc->>AuthSvc: setAdditionalConfigInOidcTransaction(transaction, features)
AuthSvc-->>Client: OAuthDetailResponse with configured OIDCTransaction
deactivate AuthSvc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (1)
63-63: Remove duplicate constant declaration.
REQUIRE_PARis already defined inConstants.java(line 67) and imported via the wildcard import on line 51. This local declaration shadows the imported constant.Apply this diff to remove the duplicate:
- public static final String REQUIRE_PAR= "require_pushed_authorization_requests"; -
🧹 Nitpick comments (8)
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java (4)
17-18: Test wiring for OpenIdProfileService is fine; consider minor cleanupThe new import of
OpenIdProfileService,ReflectionTestUtils, and the@Mock OpenIdProfileService openIdProfileServicefield are all appropriate for injecting the profile service intoClientManagementServiceImplin tests. To reduce duplication you could set theopenIdProfileServiceandopenidProfilefields once in a@BeforeEachinstead of per test, but that’s optional.Also applies to: 31-32, 49-50
361-394: createClientV3_withProfileFeatures_appliesAdditionalConfig does not assert profile behaviorThis test currently only asserts
clientIdandstatus. It does not verify that:
openIdProfileService.getFeaturesByProfileName("fapi2.0")is actually invoked, or- the additionalConfig flags (e.g., PAR/DPoP/PKCE) are modified/enforced based on the profile.
That means the test can pass even if the new profile-based logic is accidentally removed.
Consider:
- Adding
Mockito.verify(openIdProfileService).getFeaturesByProfileName("fapi2.0");- Capturing the
ClientDetailpassed toclientDetailRepository.save(...)and asserting the serialized additional config reflects the expected feature enforcement for the active profile.
396-427: updateClientV3_withProfileFeatures_appliesAdditionalConfig also lacks assertions on additionalConfigSame as the create path, this test only checks
clientIdandstatus. It doesn’t ensure:
openIdProfileServiceis used during update, or- the stored additional config respects the profile’s required features.
Recommend mirroring the create‑test improvements here: add a
Mockito.verifyongetFeaturesByProfileNameand assertions (via an argument captor orargThat) on the savedClientDetail’s additional config to prove the profile logic is applied on update.
429-455: Helper methods look good; consider aligning keys with shared constants
getClientDetail()andgetAdditionalConfig()are fine for test setup and clearly express the required defaults. If there are central constants for keys like"require_pushed_authorization_requests","dpop_bound_access_tokens", and"require_pkce", using them here would reduce the chance of drift from production code, but it’s not essential.openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java (1)
36-38: Consider using a defined error constant instead of a dynamic message.
EsignetExceptionis designed to accept error codes fromErrorConstants. Passing a dynamic message like"No features found for openid profile: " + profileNamemay cause issues with error handling/localization downstream that expects standardized codes.Consider defining a new error constant (e.g.,
INVALID_OPENID_PROFILE) inErrorConstantsand using it here:if (profiles == null || profiles.isEmpty()) { - throw new EsignetException("No features found for openid profile: " + profileName); + throw new EsignetException(ErrorConstants.INVALID_OPENID_PROFILE); }Alternatively, if dynamic context is needed, consider logging the profile name separately and throwing with a constant error code.
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (2)
42-47: Redundant static imports.Line 42 uses wildcard import
import static io.mosip.esignet.core.constants.Constants.*;which already imports all constants. The subsequent explicit imports on lines 43-47 are redundant.Apply this diff to remove redundant imports:
-import static io.mosip.esignet.core.constants.Constants.*; -import static io.mosip.esignet.core.constants.Constants.DPOP_BOUND_ACCESS_TOKENS; -import static io.mosip.esignet.core.constants.Constants.NISDSP; -import static io.mosip.esignet.core.constants.Constants.REQUIRE_PAR; -import static io.mosip.esignet.core.constants.Constants.REQUIRE_PKCE; -import static io.mosip.esignet.core.constants.Constants.USERINFO_RESPONSE_TYPE; +import static io.mosip.esignet.core.constants.Constants.*;
339-360: Refactor duplicate code and fix misleading error message.
- The two
setAdditionalConfigmethods are nearly identical - only the parameter type differs.- The error message "fapiCompatibility" is misleading since this handles general OpenID profile configuration.
- Silently catching exceptions may hide configuration errors.
Consider extracting the common logic and improving error handling:
- private void setAdditionalConfig(ClientDetailCreateRequestV3 clientDetailCreateRequestV3, ClientDetail clientDetail) { - try { - JsonNode additionalConfigNode = objectMapper.valueToTree(clientDetailCreateRequestV3.getAdditionalConfig()); - updateAdditionalConfig(clientDetail, additionalConfigNode); - } catch (Exception e) { - log.error("Error while setting fapiCompatibility to additional config", e); - } - } - - private void setAdditionalConfig(ClientDetailUpdateRequestV3 clientDetailUpdateRequestV3, ClientDetail clientDetail) { - try { - JsonNode additionalConfigNode = objectMapper.valueToTree(clientDetailUpdateRequestV3.getAdditionalConfig()); - updateAdditionalConfig(clientDetail, additionalConfigNode); - } catch (Exception e) { - log.error("Error while setting fapiCompatibility to additional config", e); - } - } + private void setAdditionalConfig(Object additionalConfig, ClientDetail clientDetail) { + try { + JsonNode additionalConfigNode = objectMapper.valueToTree(additionalConfig); + updateAdditionalConfig(clientDetail, additionalConfigNode); + } catch (Exception e) { + log.error("Error while setting openid profile config to additional config", e); + throw new EsignetException(ErrorConstants.UNKNOWN_ERROR); + } + }Then update callers:
setAdditionalConfig(clientDetailCreateRequestV3.getAdditionalConfig(), clientDetail); setAdditionalConfig(clientDetailUpdateRequestV3.getAdditionalConfig(), clientDetail);pom.xml (1)
65-75: Verify GCS Maven Central mirror is necessary and reliable.The new GCS mirror repository is configured for production Maven Central access. Ensure this URL is stable and provides benefits (e.g., improved availability, reduced load on Maven Central) over existing repository configurations.
Could you clarify the operational rationale for adding this mirror? If it's for improved resilience or performance, consider documenting this in the repository's infrastructure or deployment guides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
db_scripts/mosip_esignet/dml/esignet-openid_profile.csvis excluded by!**/*.csv
📒 Files selected for processing (24)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java(6 hunks)client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java(4 hunks)db_scripts/mosip_esignet/ddl.sql(1 hunks)db_scripts/mosip_esignet/ddl/esignet-openid_profile.sql(1 hunks)db_scripts/mosip_esignet/dml.sql(1 hunks)db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_rollback.sql(1 hunks)db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql(1 hunks)docker-compose/init.sql(1 hunks)esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java(1 hunks)esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java(1 hunks)esignet-core/src/main/java/io/mosip/esignet/core/spi/OpenIdProfileService.java(1 hunks)esignet-core/src/test/resources/additional_config_request_schema.json(1 hunks)esignet-service/pom.xml(1 hunks)esignet-service/src/main/resources/additional_config_request_schema.json(1 hunks)esignet-service/src/main/resources/application-default.properties(4 hunks)esignet-service/src/test/resources/additional_config_request_schema.json(1 hunks)oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java(10 hunks)openid-profile-service/pom.xml(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/repository/OpenIdProfileRepository.java(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java(1 hunks)openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java(1 hunks)pom.xml(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.javaesignet-service/src/main/resources/application-default.propertiesesignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.javaesignet-service/src/main/resources/additional_config_request_schema.jsonclient-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-28T05:36:42.921Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:127-130
Timestamp: 2025-11-28T05:36:42.921Z
Learning: In the MOSIP esignet codebase, the `mosip.esignet.discovery.issuer-id` property is configured as `${mosip.esignet.domain.url}${server.servlet.path}`, where `server.servlet.path` typically equals `/v1/esignet`. Therefore, the `discoveryIssuerId` field already contains the `/v1/esignet` prefix, and appending `/oauth/token` for the non-V2 token endpoint correctly produces the full path `/v1/esignet/oauth/token`.
Applied to files:
esignet-service/src/main/resources/application-default.properties
📚 Learning: 2025-11-17T05:56:18.589Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1535
File: api-test/src/main/resources/esignet/PAR/OauthParNegativeScenarios/OauthParNegativeScenarios.yml:162-162
Timestamp: 2025-11-17T05:56:18.589Z
Learning: In the esignet PAR (Pushed Authorization Requests) implementation, when an invalid client_id is provided, the error response uses "error": "invalid_request" with "error_description": "invalid_client_id", rather than "error": "invalid_client". The test expectations in OauthParNegativeScenarios.yml reflect this implementation behavior.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧬 Code graph analysis (6)
esignet-core/src/main/java/io/mosip/esignet/core/spi/OpenIdProfileService.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/exception/EsignetException.java (1)
EsignetException(11-26)
openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/exception/EsignetException.java (1)
EsignetException(11-26)
openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/exception/EsignetException.java (1)
EsignetException(11-26)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-74)
openid-profile-service/src/main/java/io/mosip/esignet/repository/OpenIdProfileRepository.java (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java (1)
OpenIdProfileId(11-37)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (2)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-74)esignet-integration-api/src/main/java/io/mosip/esignet/api/util/ErrorConstants.java (1)
ErrorConstants(8-30)
🔇 Additional comments (23)
esignet-service/src/main/resources/additional_config_request_schema.json (1)
69-71: Schema update is correct and consistent.The new
require_pkceboolean property is correctly added to the additional config schema, follows existing naming conventions, and maintains schema integrity with proper comma placement andadditionalProperties: falseconstraint.esignet-service/src/test/resources/additional_config_request_schema.json (1)
69-71: Test schema properly mirrors production schema.The test schema is kept in sync with the production schema, ensuring schema validation tests will correctly cover the new
require_pkceproperty.esignet-service/src/main/resources/application-default.properties (2)
38-39: Feature caching TTL mirrors kbispec pattern.The TTL value (18000 seconds) and naming convention follow the established kbispec pattern, which is appropriate for caching profile-feature mappings. This is consistent with the project's caching strategy.
194-194: Cache configuration additions are consistent and well-structured.The
featurescache namespace has been properly integrated across all required configuration maps:
- Added to cache names list (line 194)
- Sized appropriately at 1 entry (line 225) for profile/feature metadata
- TTL correctly references the features-specific property (line 248)
All additions follow existing patterns and naming conventions.
Also applies to: 224-225, 247-248
esignet-core/src/main/java/io/mosip/esignet/core/spi/OpenIdProfileService.java (1)
11-18: LGTM!The interface is clean and well-documented. The method signature appropriately returns a list of feature names and declares the
EsignetExceptionfor error handling. The Javadoc provides helpful examples of profile names.openid-profile-service/src/main/java/io/mosip/esignet/repository/OpenIdProfileRepository.java (1)
14-15: LGTM!The repository follows Spring Data JPA conventions correctly. The
findByProfileNamemethod will properly derive the query from theprofileNamefield in the composite key.esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java (1)
82-84: LGTM!The new boolean fields
requirePushedAuthorizationRequestsandrequirePKCEare appropriately placed alongside related authentication configuration fields. The defaultfalsevalues for primitives ensure backward compatibility during deserialization of existing transactions.db_scripts/mosip_esignet/dml.sql (1)
5-12: Ensure the referenced CSV fileesignet-openid_profile.csvexists at the expected path.The DML script correctly truncates the
openid_profiletable with CASCADE before data load, uses consistent\COPYsyntax with proper DELIMITER and HEADER options, and documents the expected CSV column order. However, the data load will fail if the CSV file is not present at./dml/esignet-openid_profile.csvrelative to the execution directory.docker-compose/init.sql (1)
168-187: LGTM!The new
openid_profiletable and seed data correctly implement the profile-feature mappings as specified in the PR requirements. The composite primary key on(profile_name, feature)properly enforces uniqueness.client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (1)
381-403: Profile handling is partially hardcoded, limiting extensibility.The method fetches features from the database via
openIdProfileService.getFeaturesByProfileName(), but then hardcodes profile-specific behavior for onlyFAPI2andNISDSP. If a new profile is added to the database (e.g., a custom profile), features will be fetched but not applied because of theisFapi || isNisdspguard.Consider whether this is intentional. If new profiles should automatically apply their features, the logic could be generalized.
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
65-73: LGTM!The new constants for PKCE, PAR requirements, and OpenID profile names are properly defined and follow the existing naming conventions.
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java (1)
12-27: LGTM!The entity is well-defined with proper JPA annotations. The
@Dataannotation from Lombok generates the necessary getters, setters, equals, and hashCode methods. The@IdClass(OpenIdProfileId.class)correctly references the composite key class.oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (1)
424-433: Logic correctly enforces PAR requirement based on profile.The method appropriately checks:
- For
NONEprofile: uses client'sadditionalConfigsetting- For
FAPI2/NISDSP: checks if PAR feature is in the profile's feature listThis ensures PAR enforcement aligns with the configured OpenID profile.
db_scripts/mosip_esignet/ddl/esignet-openid_profile.sql (1)
17-21: Verify schema context for table creation.The DDL uses unqualified table name
openid_profile, whiledocker-compose/init.sqluses schema-qualifiedesignet.openid_profile. Ensure that when this script executes, the search path or schema context is properly set toesignet, otherwise the table may be created in the wrong schema.openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java (1)
50-68: Verify that OpenIdProfileServiceImpl throws exceptions with expected message content.The test assertions check
ex.getMessage()for descriptive text like "No features found for openid profile: nonexistent". Standard Java exceptions support this pattern. However, verify thatOpenIdProfileServiceImpl.getFeaturesByProfileName()actually throwsEsignetExceptionwith these exact message strings when no profile is found or null is provided; otherwise, the assertions may fail if the implementation passes different message content.openid-profile-service/pom.xml (1)
1-41: Module structure and dependencies look appropriate.The openid-profile-service module follows esignet conventions with proper parent reference, Java 21 compatibility, and clean test dependencies.
pom.xml (1)
114-114: Module addition correctly ordered.The openid-profile-service module is properly added to the build sequence, maintaining logical dependency ordering (core components first, then service integrations).
esignet-core/src/test/resources/additional_config_request_schema.json (1)
68-71: Schema addition for require_pkce is well-placed and syntactically correct.The new boolean property aligns with the PR objective and maintains schema consistency.
Ensure require_pkce validation is consistently applied across client-management and OIDC service implementations that reference this schema.
db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_rollback.sql (1)
7-9: Rollback script correctly mirrors upgrade with idempotent DDL.Both table drops are protected by IF EXISTS, ensuring rollback is safe and can be re-run without errors.
esignet-service/pom.xml (2)
82-87: openid-profile-service dependency integration is correct.The new module is properly integrated with compile scope and project version reference, maintaining consistency with other internal module dependencies.
78-81: jackson-annotations dependency is missing explicit version.The jackson-annotations dependency has no version specified. If the parent POM (esignet-parent 1.8.0-SNAPSHOT) does not manage this version in its dependencyManagement section, this creates version resolution risk and potential incompatibility with other Jackson libraries used in the project.
Verify that esignet-parent defines jackson-annotations in dependencyManagement. If not, add an explicit version matching the Jackson libraries (databind, core) already in use.
db_scripts/mosip_esignet/ddl.sql (1)
11-11: Verify that esignet-openid_profile.sql exists in the ddl/ directory.The \ir directive syntax is correct and follows PostgreSQL conventions. Confirm the referenced file
esignet-openid_profile.sqlexists indb_scripts/mosip_esignet/ddl/and contains the openid_profile table definition before merging.db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql (1)
49-57: Verify seed data population and service initialization logic for openid_profile table.The table schema is well-designed, but the upgrade script creates an empty table without initial profile-feature mappings. Before deployment, confirm that initial profiles (e.g., FAPI2.0, NISDSP) are seeded via:
- Separate DML migration scripts executed after this schema migration
- Application initialization logic on startup
- Manual initialization documented for operators
Additionally, verify that dependent services (OpenIdProfileService, AuthorizationServiceImpl, ClientManagementServiceImpl) handle gracefully when profiles are not yet configured at upgrade time.
esignet-service/src/main/resources/application-default.properties
Outdated
Show resolved
Hide resolved
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
Outdated
Show resolved
Hide resolved
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java
Outdated
Show resolved
Hide resolved
openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java (1)
16-41: Optionally addserialVersionUIDfor theSerializableID classNot mandatory, but adding an explicit
serialVersionUIDavoids default‑UID fragility if the class shape changes later.-public class OpenIdProfileId implements Serializable { - private String profileName; - private String feature; +public class OpenIdProfileId implements Serializable { + private static final long serialVersionUID = 1L; + + private String profileName; + private String feature;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java (1)
Data(14-28)
🔇 Additional comments (2)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java (2)
8-18: Ensure Lombok is correctly configured for this moduleThis class depends on Lombok’s
@Getterand@Setter; please verify that the module’s build (pom/gradle) includes Lombok and that annotation processing is enabled, or replace these with explicit accessors if Lombok is not desired.
16-36: Id class structure is appropriate for use with@IdClassPublic class,
Serializableimplementation, matching fields (profileName,feature), no‑arg and all‑args constructors, and a field‑basedequals/hashCodesatisfy typical JPA@IdClassrequirements and align withOpenIdProfile.
Md-Humair-KK
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java (1)
14-28: Consider adding audit fields for change tracking.While the current structure is sufficient for a simple lookup table, adding standard audit fields would enhance traceability and align with common enterprise practices. Consider adding:
createdBy/updatedBy(String)createdDtimes/updatedDtimes(Timestamp)This would help track configuration changes over time, which can be valuable for compliance and troubleshooting.
Example addition:
@Column(name = "cr_by", length = 256, nullable = false) private String createdBy; @Column(name = "cr_dtimes", nullable = false) private LocalDateTime createdDtimes; @Column(name = "upd_by", length = 256) private String updatedBy; @Column(name = "upd_dtimes") private LocalDateTime updatedDtimes;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
🔇 Additional comments (1)
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java (1)
1-29: LGTM! Well-structured JPA entity with proper composite key implementation.The entity correctly implements a composite primary key using
@IdClass, with appropriate column constraints and Lombok annotations for boilerplate reduction. The simple structure is well-suited for a profile-to-feature mapping table.
...gement-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
Outdated
Show resolved
Hide resolved
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java (1)
35-38: Optional: Use Mockito JUnit 5 extension instead of manualopenMocks.
MockitoAnnotations.openMocks(this)works, but you can simplify by annotating the class with@ExtendWith(MockitoExtension.class)and dropping thesetUpmethod; the extension will manage mock initialization and lifecycle for you.client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (1)
56-70: LGTM! Consider validating the configuration property value.The autowiring of
OpenIdProfileServiceand the configuration property are correctly implemented. However, consider adding validation to ensuremosip.esignet.openid.profilecontains only valid values (fapi2.0, nisdsp, none) at startup to fail fast if misconfigured.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java(6 hunks)client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java(4 hunks)openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java(1 hunks)openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java(1 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
🧬 Code graph analysis (1)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-74)
🔇 Additional comments (6)
pom.xml (1)
103-103: Module addition is properly configured and integrated.All verification checks pass:
openid-profile-service/pom.xmlexists with correct parent declaration (io.mosip.esignet:esignet-parent:1.8.0-SNAPSHOT)esignet-servicecorrectly declares the dependency onopenid-profile-servicewith proper scope- Module placement in root pom is correct and follows the naming conventions
- No circular dependencies exist
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java (1)
15-42: IdClass implementation is JPA-friendly and consistent.Serializable, no-arg constructor, Lombok-generated accessors, and equals/hashCode over both key fields make this a solid
@IdClassimplementation forOpenIdProfile. No issues from a correctness or JPA-usage standpoint.openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java (1)
40-73: Tests cover happy-path and key error scenarios forgetFeaturesByProfileName.The three tests together validate feature extraction, behavior on empty repository results, and null input handling (including error message content). This gives good confidence in the service behavior around OpenID profile feature resolution.
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (1)
322-332: LGTM! Consistent profile feature application.The integration of
setAdditionalConfigin both create and update flows ensures consistent profile-based feature injection across client management operations.client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java (2)
49-51: LGTM!Mock setup for
OpenIdProfileServiceis appropriate for testing the profile-based feature injection.
429-455: LGTM! Good test data helpers.The helper methods improve test readability and maintainability by centralizing test data creation.
...gement-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
Outdated
Show resolved
Hide resolved
...gement-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
Outdated
Show resolved
Hide resolved
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (2)
66-72: Minor formatting inconsistency: missing spaces before=.Several constants have inconsistent spacing around the
=operator compared to the rest of the codebase (e.g.,REQUIRE_PAR=vsREQUIRE_PKCE =).- public static final String REQUIRE_PKCE = "require_pkce"; - public static final String REQUIRE_PAR= "require_pushed_authorization_requests"; + public static final String REQUIRE_PKCE = "require_pkce"; + public static final String REQUIRE_PAR = "require_pushed_authorization_requests"; //OpenId profile names - public static final String FAPI2= "fapi2.0"; - public static final String NISDSP= "nisdsp"; - public static final String NONE= "none"; + public static final String FAPI2 = "fapi2.0"; + public static final String NISDSP = "nisdsp"; + public static final String NONE = "none";
74-78: Formatting inconsistency continues in feature constants.- public static final String FEATURE_PAR= "PAR"; - public static final String FEATURE_DPOP= "DPOP"; - public static final String FEATURE_JWE= "JWE"; - public static final String FEATURE_PKCE= "PKCE"; - public static final String FEATURE_JWS= "JWS"; + public static final String FEATURE_PAR = "PAR"; + public static final String FEATURE_DPOP = "DPOP"; + public static final String FEATURE_JWE = "JWE"; + public static final String FEATURE_PKCE = "PKCE"; + public static final String FEATURE_JWS = "JWS";client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java (1)
454-467: Consider test coverage for other profiles and edge cases.Currently only the
fapi2.0profile is tested. Per the PR description:
fapi2.0: enables PAR, DPOP, JWEnisdsp: enables PAR, DPOP, JWE, PKCEnone: retains RP-provided flags without overridingConsider adding tests for:
nisdspprofile to verify all four features are enablednoneprofile to verify RP flags are preserved unchanged- Edge case when
openIdProfileService.getFeaturesByProfileName()returns an empty list
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java(6 hunks)client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java(5 hunks)esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java
🔇 Additional comments (2)
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java (2)
47-48: LGTM: Mock setup for OpenIdProfileService.The mock declaration follows the existing pattern in the test class.
441-452: LGTM: Helper method provides consistent test fixture.The
getClientDetail()helper provides a clean, reusable setup for tests.
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java
Outdated
Show resolved
Hide resolved
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (3)
68-70: Missingprivatemodifier on autowired field.For consistency with other autowired fields in this class (e.g.,
clientManagementService,cacheUtilService), add theprivatemodifier.@Autowired - OpenIdProfileService openIdProfileService; + private OpenIdProfileService openIdProfileService;
424-433: UseFEATURE_PARconstant instead of hardcoded string.Line 428 uses the hardcoded string
"PAR"whilesetAdditionalConfigInOidcTransactionusesFEATURE_PARconstant. Use the constant for consistency and maintainability.private void assertPARRequiredIsFalse(ClientDetail clientDetail, List<String> features) throws EsignetException { boolean isParRequired = (openidProfile == null || NONE.equalsIgnoreCase(openidProfile)) ? clientDetail.getAdditionalConfig(REQUIRE_PAR, false) : ((FAPI2.equalsIgnoreCase(openidProfile) || NISDSP.equalsIgnoreCase(openidProfile)) - && features.contains("PAR")); + && features.contains(FEATURE_PAR)); if (isParRequired) { log.error("Pushed Authorization Request (PAR) flow is mandated for clientId: {}", clientDetail.getId()); throw new EsignetException(ErrorConstants.INVALID_REQUEST); } }
646-651: Javadoc uses incorrect generic syntax.The
@param featurestag uses{@link List<String>}which won't render correctly. Use{@link List}or justList<String>without the link tag for generics./** * Set additional config in OIDC transaction based on openid profile and client additional config * @param oidcTransaction {@link OIDCTransaction} * @param clientDetailDto {@link ClientDetail} - * @param features {@link List<String>} + * @param features List of feature flags for the OpenID profile */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-17T05:56:18.589Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1535
File: api-test/src/main/resources/esignet/PAR/OauthParNegativeScenarios/OauthParNegativeScenarios.yml:162-162
Timestamp: 2025-11-17T05:56:18.589Z
Learning: In the esignet PAR (Pushed Authorization Requests) implementation, when an invalid client_id is provided, the error response uses "error": "invalid_request" with "error_description": "invalid_client_id", rather than "error": "invalid_client". The test expectations in OauthParNegativeScenarios.yml reflect this implementation behavior.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧬 Code graph analysis (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-79)
🔇 Additional comments (2)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (2)
652-676: FAPI2 profile leavesrequirePKCEunset while other properties are explicitly configured.For the FAPI2 profile,
userInfoResponseType,dpopBoundAccessToken, andrequirePushedAuthorizationRequestsare explicitly set, butrequirePKCEis not. This asymmetry meansrequirePKCEretains whatever default/previous value exists onOIDCTransaction, which could lead to unexpected behavior.If FAPI2 intentionally doesn't enforce PKCE (per your requirements), consider explicitly setting it to
falsefor clarity and to prevent any unintended state carryover.if (isFapi || isNisdsp) { oidcTransaction.setUserInfoResponseType(jwe ? FEATURE_JWE : FEATURE_JWS); oidcTransaction.setDpopBoundAccessToken(dpop); oidcTransaction.setRequirePushedAuthorizationRequests(par); } + if (isFapi) { + oidcTransaction.setRequirePKCE(false); + } if (isNisdsp) { oidcTransaction.setRequirePKCE(pkce); }Please verify whether FAPI2 should explicitly disable PKCE or if leaving it unset is the intended behavior.
136-142: LGTM for feature integration ingetOauthDetails.The integration of
OpenIdProfileServiceto fetch features and pass them through toassertPARRequiredIsFalseandcheckAndBuildOIDCTransactionis clean and follows the established pattern.
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (1)
52-52: Duplicate import still present despite being marked as resolved.The explicit import of
REQUIRE_PARat line 52 is redundant because line 51 already imports all constants fromConstantswith a wildcard. This was flagged in a previous review and marked as "resolved," but the code still contains the duplicate import.Apply this diff to remove the redundant import:
import static io.mosip.esignet.core.constants.Constants.*; -import static io.mosip.esignet.core.constants.Constants.REQUIRE_PAR; import static io.mosip.esignet.core.spi.TokenService.ACR;
🧹 Nitpick comments (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (1)
650-674: Consider explicitly setting requirePKCE to false for FAPI2 profile.The current implementation sets
requirePKCEfor NISDSP (line 671) and uses client config for "none" profile (line 663), but never explicitly sets it for FAPI2. Per the requirements and learnings, FAPI2 should NOT require PKCE. While the code may work ifOIDCTransactiondefaults to false, explicitly setting it improves clarity and prevents issues if the default changes or if transaction objects are reused.Apply this diff to explicitly set requirePKCE for FAPI2:
if (isFapi || isNisdsp) { oidcTransaction.setUserInfoResponseType(jwe ? FEATURE_JWE : FEATURE_JWS); oidcTransaction.setDpopBoundAccessToken(dpop); oidcTransaction.setRequirePushedAuthorizationRequests(par); } if (isNisdsp) { oidcTransaction.setRequirePKCE(pkce); + } else if (isFapi) { + oidcTransaction.setRequirePKCE(false); }Based on learnings, FAPI2 should not enable PKCE while NISDSP should.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java(10 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
📚 Learning: 2025-12-11T11:13:52.059Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-17T05:56:18.589Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1535
File: api-test/src/main/resources/esignet/PAR/OauthParNegativeScenarios/OauthParNegativeScenarios.yml:162-162
Timestamp: 2025-11-17T05:56:18.589Z
Learning: In the esignet PAR (Pushed Authorization Requests) implementation, when an invalid client_id is provided, the error response uses "error": "invalid_request" with "error_description": "invalid_client_id", rather than "error": "invalid_client". The test expectations in OauthParNegativeScenarios.yml reflect this implementation behavior.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧬 Code graph analysis (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-79)
🔇 Additional comments (4)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (4)
66-67: LGTM!The OpenIdProfileService dependency and openidProfile property are properly wired with a sensible default value of "fapi2.0" as specified in the requirements.
Also applies to: 130-131
422-431: LGTM!The PAR validation logic correctly distinguishes between profile-driven configuration (FAPI2/NISDSP) and client-driven configuration ("none"/null), using the features list to determine PAR requirements for profiles.
434-434: LGTM!The method signatures are properly updated to thread the features list through the authorization flow, ensuring profile-driven configuration is applied consistently when building OIDC transactions.
Also applies to: 486-486, 502-504
136-137: No changes needed—the code is null-safe.The
getFeaturesByProfileName()method inOpenIdProfileServiceImplthrows anEsignetExceptionif profiles are null or empty, guaranteeing that a non-nullList<String>is always returned. This meansfeatures.contains()calls at lines 651-654 and throughout the code are safe and will never cause aNullPointerException.
* updating project version to 1.8.0 Signed-off-by: Sachin Rana <sacrana324@gmail.com> * corrected license header text Signed-off-by: Sachin Rana <sacrana324@gmail.com> --------- Signed-off-by: Sachin Rana <sacrana324@gmail.com> Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Ivanmeneges <ivan.anil016@gmail.com> Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
…le the profile specific configurations Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Md-Humair-KK
force-pushed
the
ES-2742A
branch
from
f106159 to
369c232
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
esignet-service/src/test/resources/additional_config_request_schema.json (1)
69-71: Consistent with esignet-core changes.Both test schema files have been updated identically with the new
require_pkceproperty, which is good for consistency across modules.client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java (1)
359-399: Align FAPI2.0 feature mocks with requirements and strengthen override assertionsTwo points across the new profile tests and helper:
- FAPI2.0 should not expose PKCE in its feature list
For both tests (Lines 364–365 and 406–407) you mock:
Mockito.when(openIdProfileService.getFeaturesByProfileName("fapi2.0")) .thenReturn(Arrays.asList("PAR", "DPOP", "JWE", "PKCE"));Per the documented behavior,
"fapi2.0"enables only PAR, DPOP, and JWE, while"nisdsp"additionally enables PKCE; PKCE should not be part of FAPI2.0 feature lists or assertions. Consider adjusting the mocks to:.thenReturn(Arrays.asList("PAR", "DPOP", "JWE"));and reserving
"PKCE"for a dedicatednisdsp-profile test that verifiesrequire_pkceis applied. Based on learnings, FAPI2.0 tests should not reference PKCE.
- Fixtures don’t fully prove PAR/DPOP overrides
getAdditionalConfig()(Lines 454–467) pre-populates:additionalConfig.put("userinfo_response_type", "JWS"); additionalConfig.put("require_pushed_authorization_requests", true); additionalConfig.put("dpop_bound_access_tokens", true); additionalConfig.put("require_pkce", true);Your assertions check that PAR/DPOP are
trueanduserinfo_response_typeis"JWE". Since PAR/DPOP are alreadytruein the fixture, the tests don’t confirm they were set by the profile features—they would pass even ifapplyProfileFeaturesonly toucheduserinfo_response_type.To exercise the overrides more explicitly, you could initialize conflicting or neutral defaults for the FAPI2.0 tests, e.g.:
- additionalConfig.put("userinfo_response_type", "JWS"); + additionalConfig.put("userinfo_response_type", "JWS"); - additionalConfig.put("require_pushed_authorization_requests", true); - additionalConfig.put("dpop_bound_access_tokens", true); + additionalConfig.put("require_pushed_authorization_requests", false); + additionalConfig.put("dpop_bound_access_tokens", false); - additionalConfig.put("require_pkce", true); + additionalConfig.put("require_pkce", false);and keep the existing assertions, so the tests demonstrate that FAPI2.0 profile logic is what turns PAR/DPOP on and flips JWS→JWE, while PKCE remains untouched for FAPI2.0 and can be covered separately for
nisdsp.Also applies to: 401-439, 454-467
🧹 Nitpick comments (5)
db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql (2)
17-38: Audit timestamp columns lack default values and constraint enforcement.The
ca_cert_storetable includes soft-delete and audit timestamp columns (cr_dtimes,upd_dtimes,del_dtimes) without DEFAULT values or constraints to ensure consistency.Consider adding DEFAULT values for auto-managed timestamp columns and a CHECK constraint to enforce that
del_dtimesis populated whenis_deleted=TRUE:ALTER TABLE ca_cert_store ALTER COLUMN cr_dtimes SET DEFAULT CURRENT_TIMESTAMP, ALTER COLUMN upd_dtimes SET DEFAULT CURRENT_TIMESTAMP, ADD CONSTRAINT chk_delete_timestamp CHECK ( (is_deleted = FALSE AND del_dtimes IS NULL) OR (is_deleted = TRUE AND del_dtimes IS NOT NULL) );Alternatively, if your application manages these fields programmatically, document this requirement clearly so future developers understand the expected behavior.
40-44: Composite key design allows efficient profile lookups but may require additional indexes.The composite primary key on
(profile_name, feature)is semantically appropriate for storing unique feature entries per profile. However, query patterns should be considered:If the application frequently queries for "all features associated with a profile name" without iterating features, the current design is optimal. If queries often search by feature across all profiles, consider adding a secondary index:
CREATE INDEX idx_openid_profile_feature ON openid_profile(feature);This would optimize reverse-lookups (e.g., "which profiles support REQUIRE_PAR?") without changing the current table structure.
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (3)
26-27: OpenIdProfileService wiring and profile property are fine; minor consistency nits.The new
OpenIdProfileServiceinjection andmosip.esignet.openid.profileproperty (defaulting tofapi2.0) match the PR intent and look correct. For consistency/readability only, you could:
- Make
openIdProfileServiceprivatelike other injected fields.- Either rely on the wildcard constants import or drop it and keep only the explicit
REQUIRE_PARimport, to avoid redundant static imports.Example (optional):
- @Autowired - OpenIdProfileService openIdProfileService; + @Autowired + private OpenIdProfileService openIdProfileService;-import static io.mosip.esignet.core.constants.Constants.*; -import static io.mosip.esignet.core.constants.Constants.REQUIRE_PAR; +import static io.mosip.esignet.core.constants.Constants.*;Also applies to: 52-52, 66-67, 130-131
422-431: Use feature constant instead of hard-coded"PAR"in PAR requirement check.The logic for blocking non-PAR endpoints when PAR is required is sound, but the literal
"PAR"is brittle given there is already aFEATURE_PARconstant and you use it elsewhere.Consider:
- ? clientDetail.getAdditionalConfig(REQUIRE_PAR, false) - : ((FAPI2.equalsIgnoreCase(openidProfile) || NISDSP.equalsIgnoreCase(openidProfile)) - && features.contains("PAR")); + ? clientDetail.getAdditionalConfig(REQUIRE_PAR, false) + : ((FAPI2.equalsIgnoreCase(openidProfile) || NISDSP.equalsIgnoreCase(openidProfile)) + && features.contains(FEATURE_PAR));This keeps the method aligned with
setAdditionalConfigInOidcTransactionand avoids drift if feature keys ever change.
644-674: Feature-to-flag mapping matches profile requirements; consider explicit handling for unexpected profile values.This helper cleanly encodes the intended behaviour:
- For
openidProfile == nullornone, all flags (userinfo_response_type, DPoP binding, PAR, PKCE) are taken from the client'sadditionalConfig.- For
fapi2.0andnisdsp, you derive JWE/DPOP/PAR (and PKCE for NISDSP) directly from thefeatureslist, overriding client config. This aligns with the documented mapping of FAPI2.0 → PAR/DPOP/JWE and NISDSP → PAR/DPOP/JWE/PKCE. Based on learnings, this matches the expectations used in tests.One small hardening improvement you might consider:
- If
mosip.esignet.openid.profileis set to an unexpected value andOpenIdProfileServicedoes not already reject it, theelsebranch withisFapi/isNisdspwill skip all assignments, leaving these flags at their defaults. You could instead either:
- Fall back to the
"none"behaviour (use clientadditionalConfig), or- Fail fast (e.g., log and throw an
EsignetException) to avoid silently weakening security under misconfiguration.For example, falling back to client config:
- } else { - if (isFapi || isNisdsp) { + } else if (isFapi || isNisdsp) { + // Profile-managed behaviour oidcTransaction.setUserInfoResponseType(jwe ? FEATURE_JWE : FEATURE_JWS); oidcTransaction.setDpopBoundAccessToken(dpop); oidcTransaction.setRequirePushedAuthorizationRequests(par); - } - if (isNisdsp) { + if (isNisdsp) { oidcTransaction.setRequirePKCE(pkce); } + } else { + // Fallback for unexpected profile names: behave like "none" + oidcTransaction.setUserInfoResponseType(clientDetailDto.getAdditionalConfig(USERINFO_RESPONSE_TYPE,FEATURE_JWS)); + oidcTransaction.setDpopBoundAccessToken(clientDetailDto.getAdditionalConfig(DPOP_BOUND_ACCESS_TOKENS, false)); + oidcTransaction.setRequirePushedAuthorizationRequests(clientDetailDto.getAdditionalConfig(REQUIRE_PAR, false)); + oidcTransaction.setRequirePKCE(clientDetailDto.getAdditionalConfig(REQUIRE_PKCE, false)); }Adjust as appropriate if
OpenIdProfileServicealready enforces valid profiles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
db_scripts/mosip_esignet/dml/esignet-openid_profile.csvis excluded by!**/*.csv
📒 Files selected for processing (34)
binding-service-impl/pom.xml(1 hunks)client-management-service-impl/pom.xml(1 hunks)client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java(6 hunks)client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java(5 hunks)consent-service-impl/pom.xml(1 hunks)db_scripts/mosip_esignet/ddl.sql(1 hunks)db_scripts/mosip_esignet/ddl/esignet-openid_profile.sql(1 hunks)db_scripts/mosip_esignet/dml.sql(1 hunks)db_upgrade_script/mosip_esignet/sql/1.7.0_to_1.7.1_rollback.sql(1 hunks)db_upgrade_script/mosip_esignet/sql/1.7.0_to_1.7.1_upgrade.sql(1 hunks)db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_rollback.sql(1 hunks)db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql(1 hunks)deploy/keycloak/install.sh(1 hunks)docker-compose/init.sql(1 hunks)esignet-core/pom.xml(1 hunks)esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java(1 hunks)esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java(1 hunks)esignet-core/src/main/java/io/mosip/esignet/core/spi/OpenIdProfileService.java(1 hunks)esignet-core/src/test/resources/additional_config_request_schema.json(1 hunks)esignet-integration-api/pom.xml(1 hunks)esignet-service/pom.xml(2 hunks)esignet-service/src/main/resources/additional_config_request_schema.json(1 hunks)esignet-service/src/main/resources/application-default.properties(4 hunks)esignet-service/src/test/resources/additional_config_request_schema.json(1 hunks)esignet-with-plugins/pom.xml(1 hunks)oidc-service-impl/pom.xml(1 hunks)oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java(10 hunks)openid-profile-service/pom.xml(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/repository/OpenIdProfileRepository.java(1 hunks)openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java(1 hunks)openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java(1 hunks)pom.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- db_scripts/mosip_esignet/ddl.sql
- openid-profile-service/pom.xml
- esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java
- docker-compose/init.sql
- esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java
- db_scripts/mosip_esignet/ddl/esignet-openid_profile.sql
- esignet-core/src/main/java/io/mosip/esignet/core/spi/OpenIdProfileService.java
- pom.xml
- esignet-service/src/main/resources/additional_config_request_schema.json
- esignet-service/src/main/resources/application-default.properties
- esignet-service/pom.xml
- openid-profile-service/src/main/java/io/mosip/esignet/repository/OpenIdProfileRepository.java
- openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
📚 Learning: 2025-12-04T10:54:26.400Z
Learnt from: Ivanmeneges
Repo: mosip/esignet PR: 1565
File: deploy/keycloak/install.sh:28-29
Timestamp: 2025-12-04T10:54:26.400Z
Learning: The Docker image mosipid/postgresql with tags like 14.2.0-debian-10-r70 is available in the MOSIP ID registry and is used for PostgreSQL deployments in MOSIP projects.
Applied to files:
deploy/keycloak/install.sh
📚 Learning: 2025-12-11T11:13:52.059Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Applied to files:
esignet-core/src/test/resources/additional_config_request_schema.jsondb_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sqlopenid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.javaopenid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.javaclient-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.javaopenid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.javaesignet-service/src/test/resources/additional_config_request_schema.jsonoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
esignet-core/src/test/resources/additional_config_request_schema.jsonclient-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.javaclient-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-12-11T11:13:45.913Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:45.913Z
Learning: In esignet OpenID profile tests under client-management-service-impl, enforce that the fapi2.0 profile feature assertions include only PAR, DPOP, and JWE (exclude PKCE). The nisdsp profile should include PAR, DPOP, JWE, and PKCE. PKCE must not appear in feature lists or assertions for any fapi2.0 profile tests.
Applied to files:
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java
📚 Learning: 2025-11-17T05:56:18.589Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1535
File: api-test/src/main/resources/esignet/PAR/OauthParNegativeScenarios/OauthParNegativeScenarios.yml:162-162
Timestamp: 2025-11-17T05:56:18.589Z
Learning: In the esignet PAR (Pushed Authorization Requests) implementation, when an invalid client_id is provided, the error response uses "error": "invalid_request" with "error_description": "invalid_client_id", rather than "error": "invalid_client". The test expectations in OauthParNegativeScenarios.yml reflect this implementation behavior.
Applied to files:
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧬 Code graph analysis (3)
openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/exception/EsignetException.java (1)
EsignetException(11-26)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java (2)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-79)esignet-core/src/main/java/io/mosip/esignet/core/constants/ErrorConstants.java (1)
ErrorConstants(8-90)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (2)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-79)esignet-core/src/main/java/io/mosip/esignet/core/constants/ErrorConstants.java (1)
ErrorConstants(8-90)
🔇 Additional comments (23)
binding-service-impl/pom.xml (1)
11-11: Version bump aligns with coordinated ecosystem release.The parent version bump from 1.7.1-SNAPSHOT to 1.8.0-SNAPSHOT is consistent with the PR's OpenID profile feature additions and broader module coordination. The change correctly flows through to
esignet.core.versionvia property inheritance.esignet-core/pom.xml (1)
12-12: Parent version update aligns with multi-module OpenID profile enhancements.The parent POM version bump from 1.7.1-SNAPSHOT to 1.8.0-SNAPSHOT is appropriate and aligns with the broader feature additions across the repository (new openid-profile-service module, profile-driven feature application, etc.). Dependencies inherit versions from the parent BOM, so no direct dependency changes are needed in this file.
db_upgrade_script/mosip_esignet/sql/1.7.0_to_1.7.1_rollback.sql (1)
1-1: Verify placeholder variable substitution and clarify rollback implications.The message uses placeholder variables (
$CURRENT_VERSIONand$UPGRADE_VERSION) that may not be substituted at runtime depending on your deployment/execution framework. If these variables are not replaced by a preprocessing step, the message will appear cryptic to database operators.Additionally, the message states rollback "not required" but doesn't clarify the implications of this design decision. For database operators, it would be helpful to document:
- Why rollback is not required for this transition (e.g., new tables can't be cleanly removed, or the design intentionally supports forward-only migrations)
- What manual steps (if any) are needed to revert to the previous version
Please verify the following:
- Confirm whether the deployment/build process substitutes these variables before executing the script. If not, consider replacing them with literal version numbers or removing the placeholders.
- Document the rationale for making the 1.7.0→1.7.1 transition non-reversible (especially given the removal of the previous ca_cert_store table drop operation).
consent-service-impl/pom.xml (1)
12-12: Parent version updated to 1.8.0-SNAPSHOT, consistent with root pom.The parent version bump enables this module to consume esignet-core at the new version, maintaining the proper version inheritance chain via
${project.parent.version}.db_upgrade_script/mosip_esignet/sql/1.7.0_to_1.7.1_upgrade.sql (1)
1-1: This upgrade script is correct as written.The 1.7.0_to_1.7.1 transition requires no schema changes, so the echo statement is the appropriate placeholder. The
ca_cert_storetable is not being removed—it is introduced later in the 1.7.1_to_1.8.0 upgrade. The shell variable substitution in the echo statement is standard practice, handled by external wrapper scripts at runtime.Likely an incorrect or invalid review comment.
esignet-core/src/test/resources/additional_config_request_schema.json (1)
69-71: require_pkce property is consistently added across all schema files and properly aligned with Java DTO fields.Verification confirms the
require_pkceboolean property has been consistently updated in all threeadditional_config_request_schema.jsonfiles (esignet-core test, esignet-service main, and esignet-service test resources), and the naming aligns with the Java constant definitionREQUIRE_PKCE = "require_pkce"throughout the codebase.oidc-service-impl/pom.xml (1)
9-9: No action required on dependencies.The version bump to 1.8.0-SNAPSHOT is correct. The
oidc-service-implmodule properly depends onesignet-corefor theOpenIdProfileServiceSPI interface; theopenid-profile-serviceimplementation module is correctly scoped as a dependency only toesignet-service. This separation of concerns is the correct architectural pattern.Likely an incorrect or invalid review comment.
esignet-with-plugins/pom.xml (1)
11-11: Version bump to 1.8.0-SNAPSHOT is consistent between parent and module versions.Both the parent (line 11) and module (line 15) versions are updated consistently to
1.8.0-SNAPSHOT. No further verification needed for this file.Note: The root
pom.xmluses independent versioning (currently2.3.6.RELEASE) and does not need to be synchronized with the esignet-with-plugins module version.esignet-integration-api/pom.xml (2)
18-59: Dependencies are appropriately scoped and versioned.The dependency declarations are well-structured:
- Provided scope correctly applied to lombok
- Test scope correctly applied to spring-boot-starter-test
- commons-text explicitly pinned to 1.14.0 (good practice for reproducibility)
- All versions appear stable with no obvious security concerns
No changes needed.
9-9: Parent version update is consistent across all modules.The parent version bump to 1.8.0-SNAPSHOT in esignet-integration-api (line 9) aligns with the PR and has been applied consistently across all modules referencing esignet-parent.
deploy/keycloak/install.sh (1)
28-29: No issues identified. Keycloak Helm chart version 7.1.18 correctly uses thepostgresql.image.repositoryandpostgresql.image.tagconfiguration paths as documented in the Bitnami PostgreSQL subchart specification. The code is correct.db_scripts/mosip_esignet/dml.sql (2)
5-5: LGTM! Truncate statement follows existing pattern.The TRUNCATE statement is correctly formatted and uses CASCADE to handle dependencies.
9-12: The CSV file exists and contains the expected profile-feature mappings. The COPY command syntax is correct.openid-profile-service/src/test/java/io/mosip/esignet/services/OpenIdProfileServiceImplTest.java (3)
26-53: LGTM! Test structure and assertions are appropriate.The happy path test correctly mocks the repository response and validates that the service returns both features.
55-63: LGTM! Error handling test is comprehensive.The test correctly validates that an EsignetException is thrown with an appropriate message when the profile does not exist.
65-73: LGTM! Null handling test is appropriate.The test validates that the service handles null profile names gracefully by throwing an exception with a clear message.
openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfile.java (1)
14-29: LGTM! JPA entity correctly implements composite key pattern.The entity properly uses
@IdClasswith two@Idfields for the composite key. Column definitions are appropriate with explicit length and nullability constraints.openid-profile-service/src/main/java/io/mosip/esignet/entity/OpenIdProfileId.java (3)
15-22: LGTM! ID class properly implements JPA @IdClass requirements.The class correctly implements Serializable with explicit serialVersionUID, and Lombok annotations provide the necessary getters for JPA compliance (as resolved in the previous review).
23-28: LGTM! Constructors meet JPA requirements.Both no-arg and parameterized constructors are correctly implemented, satisfying JPA's requirements for ID classes.
30-42: LGTM! equals and hashCode correctly implemented.Both methods follow best practices:
equalsperforms identity check, type check, and null-safe field comparison, whilehashCodeusesObjects.hashfor consistency.oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (3)
133-148: Profile-based feature resolution and PAR gating in entrypoints look correct.Resolving
featuresviaopenIdProfileService.getFeaturesByProfileName(openidProfile)and:
- Calling
assertPARRequiredIsFalseonly for non-PAR flows (V1/V2),- Passing
featuresinto the common transaction builder for both non-PAR and PAR flows,gives consistent profile-driven behaviour while ensuring PAR-only profiles/clients cannot initiate non-PAR transactions. This matches the requirement that FAPI2.0 and NISDSP are enforced via server-side profile, with
"none"deferring to client additionalConfig. Based on learnings, this aligns with the expected feature sets per profile.Also applies to: 151-158, 167-185
433-488: Passingfeaturesinto the transaction builder and delegating additional config is reasonable.Extending
checkAndBuildOIDCTransactionwith afeaturesparameter and delegating all profile/client-specific flag setting tosetAdditionalConfigInOidcTransactionkeeps the builder focused on core transaction data while centralizing profile logic in one place. The rest of the transaction construction is preserved, so behaviour should remain backward compatible aside from the intended profile-driven flags.
501-505: V2 and PAR flows correctly reuse the common builder with features.Threading
featuresthroughbuildTransactionAndOAuthDetailResponseand reusing the samecheckAndBuildOIDCTransactioncall for both V2 and PAR flows ensures that profile-based flags are applied uniformly regardless of how the transaction is initiated.
...gement-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql (1)
56-58: Consider adding a reference to the DML script for clarity.The comments are clear, but future maintainers would benefit from knowing where the seed data lives. Consider adding a note after line 34 (after the table creation):
-- Note: Data seeding for this table is handled in db_scripts/mosip_esignet/dml.sql via esignet-openid_profile.csvThis improves discoverability of the data population mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java(6 hunks)db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_rollback.sql(1 hunks)db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
- db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_rollback.sql
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
📚 Learning: 2025-12-11T11:13:52.059Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Applied to files:
db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql
🔇 Additional comments (1)
db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql (1)
30-34: LGTM! Well-structured static lookup table.The table design is appropriate for storing profile-to-feature mappings. The composite primary key ensures uniqueness and provides efficient lookups without requiring additional indexes.
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
Outdated
Show resolved
Hide resolved
oidc-service-impl/src/main/java/io/mosip/esignet/entity/ServerProfile.java
Show resolved
Hide resolved
openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java
Outdated
Show resolved
Hide resolved
...gement-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
Show resolved
Hide resolved
openid-profile-service/src/main/java/io/mosip/esignet/services/OpenIdProfileServiceImpl.java
Outdated
Show resolved
Hide resolved
esignet-service/src/main/resources/application-default.properties
Outdated
Show resolved
Hide resolved
esignet-service/src/main/resources/application-default.properties
Outdated
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In
@esignet-core/src/main/java/io/mosip/esignet/core/spi/ServerProfileService.java:
- Around line 11-18: Update the JavaDoc for
ServerProfileService.getFeaturesByProfileName: correct the punctuation by
changing "fapi2.0." to "fapi2.0," and remove the incorrect profile value "gov",
replacing the list with the supported profiles "fapi2.0, nisdsp, none" so the
method documentation accurately and cleanly describes accepted profileName
values.
In @esignet-service/src/main/resources/application-default.properties:
- Around line 32-33: Update the comment above the mosip.esignet.server.profile
property to remove the unsupported "gov" profile; ensure the comment only lists
the actual profile constants used in the codebase (fapi2.0, nisdsp, none) so it
matches Constants.java and avoids referencing a non-existent GOV profile.
In
@oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java:
- Around line 646-680: In setAdditionalConfigInOidcTransaction, replace the
string literal "NONE" used in the useClientDefaults check with the shared
constant (Constants.NONE) to keep profile checks consistent; update the
expression (openidProfile == null) || "NONE".equalsIgnoreCase(openidProfile) to
use Constants.NONE.equalsIgnoreCase(openidProfile) so it matches other profile
checks and avoids hard-coded strings.
- Around line 418-427: In method assertPARRequiredIsFalse replace the string
literal "PAR" with the FEATURE_PAR constant to keep usage consistent (reference:
assertPARRequiredIsFalse, FEATURE_PAR, openidProfile, isParRequired) and update
the control flow to explicitly handle unknown openidProfile values (e.g., add an
else branch that logs a warning or throws an EsignetException when openidProfile
is not null and not NONE, FAPI2, or NISDSP) so misconfigured profiles are
surfaced instead of silently defaulting isParRequired to false.
🧹 Nitpick comments (2)
oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.java (1)
108-108: Redundant mock initialization.Since the test class is already annotated with
@ExtendWith(MockitoExtension.class)(line 60), callingMockitoAnnotations.openMocks(this)in thesetUpmethod is redundant. TheMockitoExtensionautomatically initializes all@Mockannotated fields.🔎 Proposed fix
@BeforeEach public void setUp() { Map<String, List<String>> claims = new HashMap<>(); claims.put("profile", Arrays.asList("given_name", "profile_picture", "name", "phone_number", "email")); claims.put("email", Arrays.asList("email", "email_verified")); claims.put("phone", Arrays.asList("phone_number", "phone_number_verified")); ClaimsHelperService claimsHelperService = new ClaimsHelperService(); ReflectionTestUtils.setField(claimsHelperService, "claims", claims); ReflectionTestUtils.setField(claimsHelperService, "objectMapper", objectMapper); ReflectionTestUtils.setField(authorizationHelperService, "claimsHelperService", claimsHelperService); - MockitoAnnotations.openMocks(this); }oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java (1)
457-471: Use ErrorConstants for exception messages.The exception message is hardcoded as a plain string. For consistency with the rest of the codebase (which uses
ErrorConstantsfor error codes), define a constant for this error scenario.🔎 Proposed refactor
In
ErrorConstants.java, add:public static final String INVALID_PROFILE_NO_FEATURES = "invalid_profile_no_features";Then update the exception:
public List<String> getFeaturesByProfileName(String profileName) { List<ServerProfile> profiles = serverProfileRepository.findByProfileName(profileName); if (profiles == null || profiles.isEmpty()) { - throw new EsignetException("No features found for openid profile: " + profileName); + log.error("No features found for openid profile: {}", profileName); + throw new EsignetException(INVALID_PROFILE_NO_FEATURES); } return profiles .stream() .map(ServerProfile::getFeature) .collect(Collectors.toList()); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.javaclient-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.javaesignet-core/src/main/java/io/mosip/esignet/core/spi/ServerProfileService.javaesignet-service/pom.xmlesignet-service/src/main/resources/application-default.propertiesoidc-service-impl/src/main/java/io/mosip/esignet/entity/ServerProfile.javaoidc-service-impl/src/main/java/io/mosip/esignet/entity/ServerProfileId.javaoidc-service-impl/src/main/java/io/mosip/esignet/repository/ServerProfileRepository.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.javaoidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java
- esignet-service/pom.xml
- client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
esignet-service/src/main/resources/application-default.propertiesoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.javaoidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-12-23T05:15:42.700Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.
Applied to files:
esignet-service/src/main/resources/application-default.propertiesoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-12-11T11:13:52.059Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Applied to files:
esignet-service/src/main/resources/application-default.propertiesesignet-core/src/main/java/io/mosip/esignet/core/spi/ServerProfileService.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.javaoidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-28T05:36:42.921Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:127-130
Timestamp: 2025-11-28T05:36:42.921Z
Learning: In the MOSIP esignet codebase, the `mosip.esignet.discovery.issuer-id` property is configured as `${mosip.esignet.domain.url}${server.servlet.path}`, where `server.servlet.path` typically equals `/v1/esignet`. Therefore, the `discoveryIssuerId` field already contains the `/v1/esignet` prefix, and appending `/oauth/token` for the non-V2 token endpoint correctly produces the full path `/v1/esignet/oauth/token`.
Applied to files:
esignet-service/src/main/resources/application-default.properties
📚 Learning: 2025-12-02T11:19:44.545Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1560
File: api-test/src/main/resources/esignet/LinkedConsent/LinkedAuthenticationConsentWla/LinkedAuthenticationConsentWla.yml:512-566
Timestamp: 2025-12-02T11:19:44.545Z
Learning: In the esignet test suite file LinkedAuthenticationConsentWla.yml, negative test cases can include successful sub-operation responses (sendOtpResp and validateOtpResp) alongside the expected error response. This composite structure validates that OTP operations can succeed while the main authentication fails, allowing comprehensive testing of the authentication flow.
Applied to files:
oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.java
📚 Learning: 2025-11-17T05:56:18.589Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1535
File: api-test/src/main/resources/esignet/PAR/OauthParNegativeScenarios/OauthParNegativeScenarios.yml:162-162
Timestamp: 2025-11-17T05:56:18.589Z
Learning: In the esignet PAR (Pushed Authorization Requests) implementation, when an invalid client_id is provided, the error response uses "error": "invalid_request" with "error_description": "invalid_client_id", rather than "error": "invalid_client". The test expectations in OauthParNegativeScenarios.yml reflect this implementation behavior.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧬 Code graph analysis (1)
esignet-core/src/main/java/io/mosip/esignet/core/spi/ServerProfileService.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/exception/EsignetException.java (1)
EsignetException(11-26)
🔇 Additional comments (12)
esignet-service/src/main/resources/application-default.properties (2)
38-39: LGTM!The server profile cache TTL configuration is appropriate. A 5-hour TTL aligns with the static nature of profile configuration and matches the TTL used for similar configuration caches (kbispec).
194-194: LGTM!The cache configuration for
serverprofileis properly integrated across all three required locations (cache names, size mapping, and TTL mapping). The cache size of 1 is appropriate for caching a single profile configuration.Also applies to: 224-225, 247-248
oidc-service-impl/src/main/java/io/mosip/esignet/repository/ServerProfileRepository.java (1)
15-17: LGTM!The repository interface correctly extends
JpaRepositorywith the appropriate entity and composite key types. The@Cacheableannotation properly caches results byprofileName, aligning with the cache configuration in application-default.properties.oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.java (3)
585-598: LGTM!The test properly validates the happy path for feature retrieval. It correctly mocks the repository to return multiple
ServerProfileentities and verifies that features are extracted and returned as a list.
600-608: LGTM!The test correctly validates the error path when no features are found for a given profile name. The exception message is descriptive and includes the profile name for debugging purposes.
610-618: LGTM!The test appropriately validates the null input case. Defensive testing like this helps prevent null pointer exceptions in production and ensures graceful error handling.
oidc-service-impl/src/main/java/io/mosip/esignet/entity/ServerProfileId.java (1)
15-43: LGTM!The composite key class correctly implements all JPA requirements: implements
Serializable, provides both no-arg and parameterized constructors, and properly overridesequals()andhashCode()using both key fields. The implementation follows standard JPA composite key patterns.oidc-service-impl/src/main/java/io/mosip/esignet/entity/ServerProfile.java (1)
14-29: LGTM! Well-structured JPA entity with composite key.The entity correctly uses
@IdClassfor the composite primary key pattern with bothprofileNameandfeatureas key fields. Column constraints (length and nullable) are appropriate for a configuration table.oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java (1)
134-135: LGTM! Field injection is consistent with the rest of the class.The
@Autowiredfield injection ofServerProfileRepositoryfollows the existing pattern in this service class.oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (3)
126-127: LGTM! Configuration field properly defined with sensible default.The
openidProfilefield with a default value of"fapi2.0"correctly drives profile-based feature configuration throughout the service.
132-133: LGTM! Feature retrieval correctly integrated into transaction flow.The calls to
authorizationHelperService.getFeaturesByProfileName(openidProfile)appropriately fetch the feature set and thread it through the OAuth detail methods to enable profile-driven behavior.Also applies to: 149-150, 178-180
646-680: Well-structured feature-driven configuration logic.The method correctly applies profile-based features to the OIDC transaction with proper defensive null handling and clear conditional logic. The implementation aligns with the requirements: FAPI2 enables PAR/DPOP/JWE, NISDSP adds PKCE, and "none" profile defers to client configuration.
Based on learnings, the feature mappings are correct.
esignet-core/src/main/java/io/mosip/esignet/core/spi/ServerProfileService.java
Show resolved
Hide resolved
esignet-service/src/main/resources/application-default.properties
Outdated
Show resolved
Hide resolved
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
Outdated
Show resolved
Hide resolved
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (2)
51-51: Remove redundant import covered by wildcard.Line 51 explicitly imports
REQUIRE_PAR, but line 50 already imports all constants from the same class with a wildcard (import static io.mosip.esignet.core.constants.Constants.*;). The explicit import is unnecessary.🔎 Proposed fix
import static io.mosip.esignet.core.constants.Constants.*; -import static io.mosip.esignet.core.constants.Constants.REQUIRE_PAR; import static io.mosip.esignet.core.spi.TokenService.ACR;
671-673: Consider validating userInfoResponseType against available features.The
userInfoResponseTypelogic defaults toFEATURE_JWSwhen the profile is active and JWE is not enabled. This means JWS will be used even if it's not explicitly present in thefeatureslist. If JWS is intended as a baseline requirement, this is fine; otherwise, consider adding validation to ensure the selected response type is actually supported by the profile.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.
📚 Learning: 2025-12-11T11:13:52.059Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-11-17T05:56:18.589Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1535
File: api-test/src/main/resources/esignet/PAR/OauthParNegativeScenarios/OauthParNegativeScenarios.yml:162-162
Timestamp: 2025-11-17T05:56:18.589Z
Learning: In the esignet PAR (Pushed Authorization Requests) implementation, when an invalid client_id is provided, the error response uses "error": "invalid_request" with "error_description": "invalid_client_id", rather than "error": "invalid_client". The test expectations in OauthParNegativeScenarios.yml reflect this implementation behavior.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
📚 Learning: 2025-12-23T05:15:42.700Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.
Applied to files:
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧬 Code graph analysis (1)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-79)
🔇 Additional comments (4)
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java (4)
126-127: LGTM! Profile configuration properly injected.The
openidProfilefield is correctly configured with a sensible default (fapi2.0) and will drive the feature-based OIDC transaction configuration throughout the service.
132-133: LGTM! Consistent feature retrieval across authorization flows.All three OAuth detail methods (
getOauthDetails,getOauthDetailsV2,getPAROAuthDetails) now consistently fetch features based on the configured profile and thread them through to downstream methods. This provides uniform profile-driven behavior.Also applies to: 149-150, 178-180
418-427: LGTM! PAR validation now uses profile-driven features.The method correctly uses the
FEATURE_PARconstant and implements profile-aware logic: when the profile isnullorNONE, it respects client configuration; otherwise, it validates against the profile's feature set.
429-430: LGTM! Method signatures properly extended for feature threading.The method signatures for
checkAndBuildOIDCTransactionandbuildTransactionAndOAuthDetailResponsehave been consistently updated to accept thefeaturesparameter, and the new helpersetAdditionalConfigInOidcTransactionis appropriately invoked to apply profile-driven configuration to the OIDC transaction.Also applies to: 482-482, 497-498, 500-500
client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java
Show resolved
Hide resolved
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java
Outdated
Show resolved
Hide resolved
esignet-service/src/main/resources/application-default.properties
Outdated
Show resolved
Hide resolved
esignet-service/src/main/resources/application-default.properties
Outdated
Show resolved
Hide resolved
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @db_scripts/mosip_esignet/dml.sql:
- Around line 9-12: Update the inline comment above the COPY command to reflect
all three CSV columns used by the \COPY statement: change the comment that
currently lists "profile_name, feature" to "profile_name, feature,
additional_config_key" so it matches the \COPY esignet.openid_profile
(profile_name, feature, additional_config_key) FROM
'./dml/esignet-openid_profile.csv' DELIMITER ',' HEADER CSV; and the CSV file
name esignet-openid_profile.csv.
In @esignet-service/src/main/resources/application-default.properties:
- Around line 32-33: The comment listing supported server profiles is
inconsistent with the actual supported values; inspect the code that reads
mosip.esignet.server.profile (lookup of profile enum or config parsing) to
confirm whether "gov" is a valid option, then update the comment line above
mosip.esignet.server.profile to exactly match the real supported values (e.g.,
"fapi2.0, nisdsp, none" if gov is not supported, or include "gov" if it is);
ensure the property key mosip.esignet.server.profile and any profile enum/class
handling profiles are the sources of truth when editing the comment.
🧹 Nitpick comments (6)
oidc-service-impl/src/main/java/io/mosip/esignet/services/OpenIdConnectServiceImpl.java (1)
56-58: Consider simplifying the null-safe boolean parsing.The current implementation relies on
Boolean.parseBoolean(null)returningfalse, which works but isn't immediately clear. Consider using a more explicit pattern for better readability:♻️ Suggested simplification
-if((Boolean.parseBoolean((transaction.getAdditionalConfigMap() != null - ? transaction.getAdditionalConfigMap().get(DPOP_BOUND_ACCESS_TOKENS) - : null))) && !tokenService.isValidDpopServerNonce(dpopHeader, transaction)) { +if(Boolean.parseBoolean( + transaction.getAdditionalConfigMap() != null + ? transaction.getAdditionalConfigMap().getOrDefault(DPOP_BOUND_ACCESS_TOKENS, "false") + : "false" + ) && !tokenService.isValidDpopServerNonce(dpopHeader, transaction)) {This makes it explicit that
falseis the default value when the config is absent.oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java (1)
159-161: Consider simplifying the null-safe boolean parsing.This implementation uses the same pattern as in OpenIdConnectServiceImpl (lines 56-58). The current code relies on
Boolean.parseBoolean(null)returningfalse, which works but could be more explicit.♻️ Suggested simplification
-if (Boolean.parseBoolean((transaction.getAdditionalConfigMap() != null - ? transaction.getAdditionalConfigMap().get(DPOP_BOUND_ACCESS_TOKENS) - : null))) { +if (Boolean.parseBoolean( + transaction.getAdditionalConfigMap() != null + ? transaction.getAdditionalConfigMap().getOrDefault(DPOP_BOUND_ACCESS_TOKENS, "false") + : "false" + )) {This makes the default
falsevalue explicit when the config is absent, improving code readability.oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java (1)
329-331: Use theDPOP_BOUND_ACCESS_TOKENSconstant instead of a hardcoded string.This line uses the literal string
"dpop_bound_access_tokens"while lines 116 and 276 use theDPOP_BOUND_ACCESS_TOKENSconstant. This inconsistency could lead to subtle bugs if the constant value changes.♻️ Proposed fix
- kycExchangeDto.setUserInfoResponseType(transaction.getAdditionalConfigMap() != null - ? transaction.getAdditionalConfigMap().get("dpop_bound_access_tokens") - : null); + kycExchangeDto.setUserInfoResponseType(transaction.getAdditionalConfigMap() != null + ? transaction.getAdditionalConfigMap().get(DPOP_BOUND_ACCESS_TOKENS) + : null);db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql (2)
145-150: Add schema prefix for consistency with INSERT statements.The
CREATE TABLEstatement doesn't include theesignet.schema prefix, while theINSERTstatements at lines 155-157 do useesignet.openid_profile. While the script connects tomosip_esignetdatabase (line 5), explicitly using the schema prefix ensures consistency and prevents issues if the search_path is modified.♻️ Proposed fix
-CREATE TABLE IF NOT EXISTS openid_profile ( +CREATE TABLE IF NOT EXISTS esignet.openid_profile ( profile_name VARCHAR(100) NOT NULL, feature VARCHAR(100) NOT NULL, additional_config_key VARCHAR(200) NOT NULL, CONSTRAINT pk_openid_profile PRIMARY KEY (profile_name, feature) );
151-154: Add missing COMMENT foradditional_config_keycolumn.Comments are added for
profile_nameandfeaturecolumns, but theadditional_config_keycolumn is missing its comment. The DDL script atdb_scripts/mosip_esignet/ddl/esignet-openid_profile.sqlincludes this comment.♻️ Proposed fix
COMMENT ON TABLE openid_profile IS 'Static table for global configuration: profile name and feature mapping.'; COMMENT ON COLUMN openid_profile.profile_name IS 'Profile name for configuration.'; COMMENT ON COLUMN openid_profile.feature IS 'Feature enabled for the profile.'; +COMMENT ON COLUMN openid_profile.additional_config_key IS 'Additional config key name for the feature.';oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java (1)
462-469: Use an error constant instead of the dynamic exception message and add duplicate key handling toCollectors.toMap.Two issues in this method:
The exception message should use an error constant from
ErrorConstantsfor consistency.INVALID_OPENID_PROFILEdoes not currently exist in the class and will need to be added, or an existing constant likeINVALID_REQUESTcan be used.
Collectors.toMapwithout a merge function will throwIllegalStateExceptionif duplicateadditionalConfigKeyvalues exist for the same profile. Add a merge function to handle this defensively:♻️ Proposed fix
public Map<String, String> getFeaturesByProfileName(String profileName) { List<ServerProfile> profiles = serverProfileRepository.findByProfileName(profileName); if (profiles == null || profiles.isEmpty()) { - throw new EsignetException("No features found for openid profile: " + profileName); + throw new EsignetException(ErrorConstants.INVALID_REQUEST); } return profiles.stream() - .collect(Collectors.toMap(ServerProfile::getAdditionalConfigKey, ServerProfile::getFeature)); + .collect(Collectors.toMap( + ServerProfile::getAdditionalConfigKey, + ServerProfile::getFeature, + (existing, replacement) -> existing)); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
db_scripts/mosip_esignet/dml/esignet-openid_profile.csvis excluded by!**/*.csv
📒 Files selected for processing (18)
client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.javaclient-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.javadb_scripts/mosip_esignet/ddl/esignet-openid_profile.sqldb_scripts/mosip_esignet/dml.sqldb_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_rollback.sqldb_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sqldocker-compose/init.sqlesignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.javaesignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.javaesignet-service/src/main/resources/application-default.propertiesesignet-service/src/main/resources/application-local.propertiesoidc-service-impl/src/main/java/io/mosip/esignet/entity/ServerProfile.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/OpenIdConnectServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.javaoidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_rollback.sql
- client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java
- oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationHelperServiceTest.java
- client-management-service-impl/src/main/java/io/mosip/esignet/services/ClientManagementServiceImpl.java
- oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
esignet-service/src/main/resources/application-default.propertiesesignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/OpenIdConnectServiceImpl.javaoidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.javaesignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java
📚 Learning: 2025-12-23T05:15:42.700Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.
Applied to files:
esignet-service/src/main/resources/application-default.properties
📚 Learning: 2025-12-11T11:13:52.059Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Applied to files:
esignet-service/src/main/resources/application-default.propertiesdb_scripts/mosip_esignet/ddl/esignet-openid_profile.sqldb_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sqldocker-compose/init.sqloidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java
📚 Learning: 2025-11-28T05:36:42.921Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:127-130
Timestamp: 2025-11-28T05:36:42.921Z
Learning: In the MOSIP esignet codebase, the `mosip.esignet.discovery.issuer-id` property is configured as `${mosip.esignet.domain.url}${server.servlet.path}`, where `server.servlet.path` typically equals `/v1/esignet`. Therefore, the `discoveryIssuerId` field already contains the `/v1/esignet` prefix, and appending `/oauth/token` for the non-V2 token endpoint correctly produces the full path `/v1/esignet/oauth/token`.
Applied to files:
esignet-service/src/main/resources/application-default.propertiesoidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java
🧬 Code graph analysis (2)
oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-69)
oidc-service-impl/src/main/java/io/mosip/esignet/services/OpenIdConnectServiceImpl.java (1)
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
Constants(8-69)
🔇 Additional comments (14)
esignet-service/src/main/resources/application-local.properties (2)
30-30: Verify the rationale for switching from Redis to in-memory caching.Changing
spring.cache.typefromredistosimplereplaces distributed caching with in-memory caching. While this may simplify local development, it could mask concurrency issues, race conditions, and caching bugs that would surface in production with Redis. This change appears unrelated to the PR objectives (OpenID profile validation features).Please clarify:
- Is this cache type change intentional and necessary for the OpenID profile feature testing?
- What is the rationale for switching to in-memory caching in the local profile?
- Should this change be in a separate PR focused on local development infrastructure?
37-37: Verify database port change compatibility.The port was changed from
5455to5432(standard PostgreSQL). This could be a breaking change for local development setups expecting the service on port5455. Additionally, this change appears orthogonal to the PR's OpenID profile validation objectives.Please confirm:
- Is the port change intentional, or was it accidentally included in this PR?
- Does this align with the team's local development setup, or should developers update their database instances?
- Should configuration changes be documented in the PR description or a migration guide?
esignet-service/src/main/resources/application-default.properties (2)
38-39: LGTM!The server profile cache TTL configuration follows existing patterns and uses a reasonable TTL value consistent with similar configuration caches (e.g., kbispec).
194-194: LGTM!The server profile cache is properly configured in all three required locations (names, size, and expiration), ensuring consistent cache behavior.
Also applies to: 224-225, 247-248
oidc-service-impl/src/main/java/io/mosip/esignet/services/OpenIdConnectServiceImpl.java (1)
26-26: LGTM!The static import for the new constant follows existing patterns in the codebase.
esignet-core/src/main/java/io/mosip/esignet/core/constants/Constants.java (1)
66-68: LGTM!The new constants follow existing naming conventions and are properly placed in the constants file for centralized management.
oidc-service-impl/src/main/java/io/mosip/esignet/services/TokenServiceImpl.java (1)
46-46: LGTM!The static import for the new constant is consistent with the changes in OpenIdConnectServiceImpl and follows existing patterns.
oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java (2)
115-124: LGTM - config-driven DPOP check.The change to read the DPOP binding flag from
additionalConfigMapaligns with the PR's goal of profile-driven feature configuration. The null-safe check and fallback logic are correctly implemented.
275-281: LGTM - consistent token type determination.The token type selection correctly uses the same config-driven approach as the validation logic above.
docker-compose/init.sql (1)
168-184: LGTM - openid_profile table and seed data are correctly defined.The table structure with composite primary key
(profile_name, feature)andadditional_config_keycolumn is correctly defined. The seed data forfapi2.0profile correctly includes only PAR, DPOP, and JWE features (without PKCE), which aligns with the expected profile configuration. Based on learnings, PKCE is only included in thenisdspprofile.esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java (1)
82-86: LGTM - new fields support profile-driven configuration.The new fields
requirePushedAuthorizationRequests,requirePKCE, andadditionalConfigMapappropriately extend the transaction model to support OpenID profile configurations. TheadditionalConfigMapprovides flexibility for feature flags, and callers already handle null checks when accessing it.oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationHelperService.java (1)
134-136: LGTM - dependency injection for ServerProfileRepository.The autowired repository follows the existing pattern in this class.
oidc-service-impl/src/main/java/io/mosip/esignet/entity/ServerProfile.java (1)
14-31: LGTM - well-structured JPA entity.The entity correctly maps to the
openid_profiletable with appropriate annotations. The@IdClassapproach for the composite primary key, column length constraints, and nullable settings all align with the DDL definition.db_scripts/mosip_esignet/ddl/esignet-openid_profile.sql (1)
17-27: LGTM - clean DDL definition with proper documentation.The table definition is well-structured with:
- Appropriate column types and constraints
- Composite primary key matching the JPA entity
- Comprehensive comments for the table and all columns
IF NOT EXISTSfor idempotent execution
esignet-service/src/main/resources/application-default.properties
Outdated
Show resolved
Hide resolved
Signed-off-by: Md-Humair-KK <mdhumair.kankudti@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @esignet-service/src/main/resources/application-default.properties:
- Line 32: The comment line "# Server profile can be either of fapi2.0, nisdsp,
gov, none etc" is misleading because "gov" isn't a documented supported profile;
update that comment (the "# Server profile..." line) to either list only the
supported profiles (e.g., "fapi2.0, nisdsp, none") or explicitly note that "gov"
is a planned/optional future value (e.g., "fapi2.0, nisdsp, none (or 'gov' as a
future/optional profile)") so the configuration comment matches the PR
objectives.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
esignet-service/src/main/resources/application-default.properties
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.
📚 Learning: 2025-12-23T05:15:42.700Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1594
File: esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java:253-280
Timestamp: 2025-12-23T05:15:42.700Z
Learning: In the MOSIP esignet codebase, the property `mosip.esignet.public-key-hash.fields` (which configures RSA and EC hash fields for public key hashing) is permanently defined in application default properties and is guaranteed to be non-null. Therefore, null checks for this configuration value in IdentityProviderUtil are not required.
Applied to files:
esignet-service/src/main/resources/application-default.properties
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
esignet-service/src/main/resources/application-default.properties
📚 Learning: 2025-12-11T11:13:52.059Z
Learnt from: Md-Humair-KK
Repo: mosip/esignet PR: 1573
File: client-management-service-impl/src/test/java/io/mosip/esignet/ClientManagementServiceTest.java:359-399
Timestamp: 2025-12-11T11:13:52.059Z
Learning: For the esignet OpenID profile feature configurations: the "fapi2.0" profile enables only PAR, DPOP, and JWE features; the "nisdsp" profile enables PAR, DPOP, JWE, and PKCE features. PKCE should not be included in feature lists or assertions for fapi2.0 profile tests.
Applied to files:
esignet-service/src/main/resources/application-default.properties
📚 Learning: 2025-11-28T05:36:42.921Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:127-130
Timestamp: 2025-11-28T05:36:42.921Z
Learning: In the MOSIP esignet codebase, the `mosip.esignet.discovery.issuer-id` property is configured as `${mosip.esignet.domain.url}${server.servlet.path}`, where `server.servlet.path` typically equals `/v1/esignet`. Therefore, the `discoveryIssuerId` field already contains the `/v1/esignet` prefix, and appending `/oauth/token` for the non-V2 token endpoint correctly produces the full path `/v1/esignet/oauth/token`.
Applied to files:
esignet-service/src/main/resources/application-default.properties
🔇 Additional comments (2)
esignet-service/src/main/resources/application-default.properties (2)
32-33: Verify property name aligns with PR description.The PR objectives state the property should be named
mosip.esignet.openid.profile, but the implementation usesmosip.esignet.server.profile. Confirm this naming is intentional, especially since downstream code may reference the PR description.
38-39: Cache configuration additions look good.The
serverprofilecache is consistently integrated: added to cache names, configured with appropriate size (5), and TTL mapped to the dedicatedmosip.esignet.server.profile.cache.ttl.secondsproperty. The 5-hour TTL aligns with the kbispec cache pattern.Also applies to: 194-194, 224-225, 247-248
| '${server.servlet.path}/oauth/v2/token', \ | ||
| '${server.servlet.path}/oidc/userinfo' } | ||
|
|
||
| # Server profile can be either of fapi2.0, nisdsp, gov, none etc |
There was a problem hiding this comment.
Clarify unsupported profile value in comment.
The comment lists "gov" as an example value alongside the documented supported profiles (fapi2.0, nisdsp, none) from the PR objectives. Either update the comment to list only supported values or confirm "gov" is an intended future/supported value.
🤖 Prompt for AI Agents
In @esignet-service/src/main/resources/application-default.properties at line
32, The comment line "# Server profile can be either of fapi2.0, nisdsp, gov,
none etc" is misleading because "gov" isn't a documented supported profile;
update that comment (the "# Server profile..." line) to either list only the
supported profiles (e.g., "fapi2.0, nisdsp, none") or explicitly note that "gov"
is a planned/optional future value (e.g., "fapi2.0, nisdsp, none (or 'gov' as a
future/optional profile)") so the configuration comment matches the PR
objectives.
5 participants
Addition of a new PKCE flag in the client management endpoints - create client and update client as part of additional config.
New property “mosip.esignet.openid.profile“ is added in the application_default.properties (fapi2.0 /nisdsp)
Client creation and client update - validated which profile is required by AS and update the additional config accordingly.
if “fapi2.0“ then PAR, DPOP, JWE
if “nisdsp“ then PAR, DPOP, JWE, PKCE
if “none“ - then use the flags as is which are set by the RP for this client without overriding them.
Validations during the initiation of the transaction
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.