ES-2712 #1613
ES-2712 #1613
Conversation
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
WalkthroughThis PR refactors server profile and feature flag handling by introducing a ServerProfile bean that centralizes feature configuration, replacing distributed feature map parameters with consolidated boolean flags in OIDCTransaction. It adds PKCE-related error codes and updates authorization flows to enforce PKCE requirements through per-transaction feature flags rather than global configuration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Fix all issues with AI agents
In `@esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java`:
- Around line 75-90: Update the database seed in the esignet-server profile SQL
so that the fapi2.0 profile no longer sets require_pkce=true: remove or set
require_pkce to false for the fapi2.0 INSERT/UPDATE and keep only PAR
(require_pushed_authorization_requests), DPOP (dpop_bound_access_token), and JWE
settings; then add a new nisdsp profile row in the same seed script with
features require_pushed_authorization_requests=true,
dpop_bound_access_token=true, require_pkce=true and set the proper
userinfo_response_type value (and any related DPoP nonce TTL/JKT columns if
present) so that nisdsp contains PAR, DPoP, PKCE and the userinfo_response_type
as specified.
In `@esignet-service/src/main/java/io/mosip/esignet/config/AppConfig.java`:
- Around line 85-105: Handle a null serverProfile explicitly and replace the
hard-coded error string with a constant: in the serverProfile() method check if
(serverProfile == null || "none".equalsIgnoreCase(serverProfile)) then return
the empty profile to avoid passing null into
serverProfileRepository.findByProfileName; when throwing the exception use
ErrorConstants.INVALID_SERVER_PROFILE instead of the literal
"INVALID_SERVER_PROFILE"; and add a public static final String
INVALID_SERVER_PROFILE = "invalid_server_profile" to the ErrorConstants class so
the constant is defined.
In
`@oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java`:
- Around line 630-635: The Javadoc for the method in AuthorizationServiceImpl
describing defaults is inaccurate: the code actually defaults feature booleans
to false and response type to "JWS" rather than "all available features". Update
the Javadoc to state the actual default behavior (booleans default to false and
response type defaults to "JWS"), or if the intended behavior is to enable all
features by default, change the implementation in the method that sets
additional config on the OIDCTransaction to initialize those booleans to true
and response type accordingly; reference the AuthorizationServiceImpl method
that takes OIDCTransaction and adjusts server/client additional config and the
OIDCTransaction fields that store the booleans and response type to make the
change.
In
`@oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationServiceTest.java`:
- Around line 115-117: Tests are failing because the `@Mock` ServerProfile returns
null for getFeatureMap(), causing an NPE when setFeatureFlags is exercised; in
the AuthorizationServiceTest setUp() method stub ServerProfile#getFeatureMap()
to return an empty Map (e.g., Collections.emptyMap()) so tests have a safe
default; update the setup in AuthorizationServiceTest (the setUp() method) to
add a when(serverProfile.getFeatureMap()).thenReturn(emptyMap) stub before
invoking code that calls setFeatureFlags.
🧹 Nitpick comments (3)
esignet-core/src/main/java/io/mosip/esignet/core/dto/ServerProfile.java (1)
12-17: Consider initializingfeatureMapto prevent potential NPE.While the current usage in
AppConfig.serverProfile()always sets the map, initializing the field defensively would prevent accidental NPE if the DTO is used elsewhere without explicit initialization.♻️ Optional defensive initialization
`@Data` public class ServerProfile { private String name; - private Map<String, String> featureMap; + private Map<String, String> featureMap = new HashMap<>(); }esignet-service/src/test/java/io/mosip/esignet/AppConfigTest.java (2)
34-35: Remove unused@Valueannotation.The
@Valueannotation has no effect in a@ExtendWith(MockitoExtension.class)test since Spring's property injection is not active. The field is never used as all tests set the value viaReflectionTestUtils.♻️ Remove dead code
- `@Value`("${mosip.esignet.server.profile:none}") - private String serverProfile; - `@Test` void serverProfile_ServerProfileIsNone_returnsWithNoFeatures() throws EsignetException {
57-63: Test relies on implicit repository behavior for null input.The test
serverProfile_NullProfile_ThrowsExceptionpasses becausefindByProfileName(null)returnsnullor empty list. However, this test doesn't mock the repository behavior, making the assertion dependent on default mock behavior (returningnull). Consider explicitly mocking for clarity.♻️ Add explicit mock for clarity
`@Test` void serverProfile_NullProfile_ThrowsException() { ReflectionTestUtils.setField(appConfig, "serverProfile", null); + when(serverProfileRepository.findByProfileName(null)).thenReturn(null); EsignetException exception = assertThrows(EsignetException.class, () -> appConfig.serverProfile()); assertEquals("INVALID_SERVER_PROFILE", exception.getMessage()); }
| //Shared flags between signup and eSignet services | ||
| String verificationStatus; | ||
| String verificationErrorCode; | ||
| String userInfoResponseType; | ||
|
|
||
| String[] prompt; | ||
| int consentExpireMinutes; | ||
|
|
||
| boolean requirePushedAuthorizationRequests; | ||
| boolean dpopBoundAccessToken; | ||
| boolean requirePKCE; | ||
| Map<String, String> additionalConfigMap; | ||
| String dpopJkt; | ||
| String dpopServerNonce; | ||
| Long dpopServerNonceTTL; | ||
|
|
||
| //Feature flags | ||
| boolean requirePushedAuthorizationRequests; | ||
| boolean dpopBoundAccessToken; | ||
| boolean requirePKCE; | ||
| String userInfoResponseType; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=java "OIDCTransaction" -C3 | head -100Repository: mosip/esignet
Length of output: 12740
🏁 Script executed:
rg -n --type=java "requirePKCE|requirePushedAuthorizationRequests|dpopBoundAccessToken" -C3 | head -150Repository: mosip/esignet
Length of output: 864
🏁 Script executed:
rg -n --type=java "additionalConfigMap" -C3Repository: mosip/esignet
Length of output: 39
🏁 Script executed:
rg -n --type=java "fapi2\.0|nisdsp" -C3 | head -100Repository: mosip/esignet
Length of output: 2365
🏁 Script executed:
fd -n "ServerProfile" --type fRepository: mosip/esignet
Length of output: 287
🏁 Script executed:
rg -n --type=java "setRequirePKCE|setRequirePushedAuthorizationRequests|setDpopBoundAccessToken" -C5Repository: mosip/esignet
Length of output: 18881
🏁 Script executed:
rg -n --type=java "getFeaturesByProfileName" -C5 | head -80Repository: mosip/esignet
Length of output: 926
🏁 Script executed:
rg -n --type=java "class ServerProfile" -A20Repository: mosip/esignet
Length of output: 4269
🏁 Script executed:
rg -n --type=java "REQUIRE_PKCE|REQUIRE_PAR|DPOP_BOUND_ACCESS_TOKENS" -C2 | head -100Repository: mosip/esignet
Length of output: 10946
🏁 Script executed:
rg -n --type=java "serverProfile\|ServerProfile" oidc-service-impl/src/main/java -A5 | head -150Repository: mosip/esignet
Length of output: 39
🏁 Script executed:
rg -n --type=java "cache.*ttl\|TTL\|Cache.*config" -C3 | head -100Repository: mosip/esignet
Length of output: 39
🏁 Script executed:
rg -n --type=java "@Cacheable|CacheManager|cache.put" -C3 | head -80Repository: mosip/esignet
Length of output: 9962
🏁 Script executed:
rg -n --type=java "setFeatureFlags" -B3 -A3Repository: mosip/esignet
Length of output: 8085
🏁 Script executed:
rg -n --type=java "cache.*expire\|cache.*ttl\|redis\.timeout\|spring.cache" -i | head -80Repository: mosip/esignet
Length of output: 39
🏁 Script executed:
rg -n --type=properties "cache\|redis\|ttl" esignet-service/src/main/resources | head -100Repository: mosip/esignet
Length of output: 91
🏁 Script executed:
cat esignet-service/src/main/resources/application-default.properties | head -100Repository: mosip/esignet
Length of output: 4738
🏁 Script executed:
rg -n --type=java "fapi2|nisdsp" -C5 | head -150Repository: mosip/esignet
Length of output: 3492
🏁 Script executed:
rg -n --type=java "ServerProfile" oidc-service-impl/src/main/java -A10 | head -100Repository: mosip/esignet
Length of output: 7221
🏁 Script executed:
rg -n "INSERT INTO" | grep -i "profile\|feature" | head -20Repository: mosip/esignet
Length of output: 1302
🏁 Script executed:
rg -n "INSERT INTO.*server_profile" -A1Repository: mosip/esignet
Length of output: 1524
🏁 Script executed:
cat db_scripts/mosip_esignet/dml/esignet-server_profile.sqlRepository: mosip/esignet
Length of output: 284
🏁 Script executed:
cat db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql | grep -A20 "INSERT INTO.*server_profile"Repository: mosip/esignet
Length of output: 477
🏁 Script executed:
cat db_scripts/mosip_esignet/dml/esignet-server_profile.sql | head -20Repository: mosip/esignet
Length of output: 284
🏁 Script executed:
rg -n "nisdsp" -C3Repository: mosip/esignet
Length of output: 2365
🏁 Script executed:
rg -n "nisdsp.*PKCE\|nisdsp.*PAR\|nisdsp.*DPOP"Repository: mosip/esignet
Length of output: 39
🏁 Script executed:
fd esignet-server_profileRepository: mosip/esignet
Length of output: 168
🏁 Script executed:
rg -n "profile_name.*nisdsp"Repository: mosip/esignet
Length of output: 39
🏁 Script executed:
cat db_scripts/mosip_esignet/dml/esignet-server_profile.sqlRepository: mosip/esignet
Length of output: 284
🏁 Script executed:
cat db_upgrade_script/mosip_esignet/sql/1.7.1_to_1.8.0_upgrade.sql | grep -A30 "server_profile"Repository: mosip/esignet
Length of output: 718
Fix server profile database configuration: fapi2.0 should not include PKCE
The database profile configuration violates the documented profile specifications. The fapi2.0 profile currently includes PKCE (as seen in db_scripts/mosip_esignet/dml/esignet-server_profile.sql), but per profile constraints, fapi2.0 should only enable PAR, DPOP, and JWE. The require_pkce feature should be exclusive to the nisdsp profile. Additionally, the nisdsp profile configuration is missing entirely from the database initialization scripts and should be added with features: PAR, DPOP, PKCE, and userinfo_response_type.
Update the database seed data to remove PKCE from fapi2.0 and add the complete nisdsp profile with the correct feature set.
🤖 Prompt for AI Agents
In `@esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java`
around lines 75 - 90, Update the database seed in the esignet-server profile SQL
so that the fapi2.0 profile no longer sets require_pkce=true: remove or set
require_pkce to false for the fapi2.0 INSERT/UPDATE and keep only PAR
(require_pushed_authorization_requests), DPOP (dpop_bound_access_token), and JWE
settings; then add a new nisdsp profile row in the same seed script with
features require_pushed_authorization_requests=true,
dpop_bound_access_token=true, require_pkce=true and set the proper
userinfo_response_type value (and any related DPoP nonce TTL/JKT columns if
present) so that nisdsp contains PAR, DPoP, PKCE and the userinfo_response_type
as specified.
| public ServerProfile serverProfile() throws EsignetException { | ||
| ServerProfile profile = new ServerProfile(); | ||
| profile.setName(serverProfile); | ||
| final Map<String, String> profileDataMap = new HashMap<>(); | ||
| profile.setFeatureMap(profileDataMap); | ||
|
|
||
| if("none".equalsIgnoreCase(serverProfile)) { | ||
| return profile; | ||
| } | ||
|
|
||
| List<io.mosip.esignet.entity.ServerProfile> profiles = serverProfileRepository.findByProfileName(serverProfile); | ||
| if (profiles == null || profiles.isEmpty()) { | ||
| log.error("**** No features found for the configured server profile: {} ****", serverProfile); | ||
| throw new EsignetException("INVALID_SERVER_PROFILE"); | ||
| } | ||
|
|
||
| for (io.mosip.esignet.entity.ServerProfile serverProfileEntity : profiles) { | ||
| profileDataMap.put(serverProfileEntity.getAdditionalConfigKey(), serverProfileEntity.getFeature()); | ||
| } | ||
| return profile; | ||
| } |
There was a problem hiding this comment.
Handle null serverProfile explicitly and define the error constant.
Two issues identified:
-
When
serverProfileisnull, theequalsIgnoreCasecheck on line 91 returnsfalse, causing a repository query withnullparameter. This relies on implicit behavior. Explicit null handling would be clearer. -
The error code
"INVALID_SERVER_PROFILE"is not defined inErrorConstants. For consistency with the codebase, define it there.
🔧 Suggested fix
Add to ErrorConstants.java:
public static final String INVALID_SERVER_PROFILE = "invalid_server_profile";Update the method:
`@Bean`
public ServerProfile serverProfile() throws EsignetException {
ServerProfile profile = new ServerProfile();
profile.setName(serverProfile);
final Map<String, String> profileDataMap = new HashMap<>();
profile.setFeatureMap(profileDataMap);
+ if(serverProfile == null) {
+ log.error("**** Server profile is not configured ****");
+ throw new EsignetException(ErrorConstants.INVALID_SERVER_PROFILE);
+ }
+
if("none".equalsIgnoreCase(serverProfile)) {
return profile;
}
List<io.mosip.esignet.entity.ServerProfile> profiles = serverProfileRepository.findByProfileName(serverProfile);
if (profiles == null || profiles.isEmpty()) {
log.error("**** No features found for the configured server profile: {} ****", serverProfile);
- throw new EsignetException("INVALID_SERVER_PROFILE");
+ throw new EsignetException(ErrorConstants.INVALID_SERVER_PROFILE);
}🤖 Prompt for AI Agents
In `@esignet-service/src/main/java/io/mosip/esignet/config/AppConfig.java` around
lines 85 - 105, Handle a null serverProfile explicitly and replace the
hard-coded error string with a constant: in the serverProfile() method check if
(serverProfile == null || "none".equalsIgnoreCase(serverProfile)) then return
the empty profile to avoid passing null into
serverProfileRepository.findByProfileName; when throwing the exception use
ErrorConstants.INVALID_SERVER_PROFILE instead of the literal
"INVALID_SERVER_PROFILE"; and add a public static final String
INVALID_SERVER_PROFILE = "invalid_server_profile" to the ErrorConstants class so
the constant is defined.
| /** | ||
| * Set additional config in OIDC transaction based on openid profile and client additional config | ||
| * Set additional config in OIDC transaction based on configured server profile and client additional config. | ||
| * Priority is given to server profile config | ||
| * Second priority is given to client additional config | ||
| * Otherwise the default value is set to all the available features | ||
| * @param oidcTransaction {@link OIDCTransaction} |
There was a problem hiding this comment.
Javadoc default-feature statement doesn’t match behavior.
The comment says defaults are “all available features,” but the implementation defaults booleans to false and response type to "JWS". Please align the comment with the actual behavior (or adjust defaults if that was intended).
🤖 Prompt for AI Agents
In
`@oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java`
around lines 630 - 635, The Javadoc for the method in AuthorizationServiceImpl
describing defaults is inaccurate: the code actually defaults feature booleans
to false and response type to "JWS" rather than "all available features". Update
the Javadoc to state the actual default behavior (booleans default to false and
response type defaults to "JWS"), or if the intended behavior is to enable all
features by default, change the implementation in the method that sets
additional config on the OIDCTransaction to initialize those booleans to true
and response type accordingly; reference the AuthorizationServiceImpl method
that takes OIDCTransaction and adjusts server/client additional config and the
OIDCTransaction fields that store the booleans and response type to make the
change.
| @Mock | ||
| private ServerProfile serverProfile; | ||
|
|
There was a problem hiding this comment.
Stub the ServerProfile feature map in test setup.
The new @Mock ServerProfile returns null for getFeatureMap() unless stubbed, which can NPE in tests that reach setFeatureFlags (unless prod code now guards). Add a default empty map in setUp() to keep tests stable.
🧪 Default empty feature map stub
when(environment.getActiveProfiles()).thenReturn(new String[]{"test"});
+ when(serverProfile.getFeatureMap()).thenReturn(Collections.emptyMap());🤖 Prompt for AI Agents
In
`@oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationServiceTest.java`
around lines 115 - 117, Tests are failing because the `@Mock` ServerProfile
returns null for getFeatureMap(), causing an NPE when setFeatureFlags is
exercised; in the AuthorizationServiceTest setUp() method stub
ServerProfile#getFeatureMap() to return an empty Map (e.g.,
Collections.emptyMap()) so tests have a safe default; update the setup in
AuthorizationServiceTest (the setUp() method) to add a
when(serverProfile.getFeatureMap()).thenReturn(emptyMap) stub before invoking
code that calls setFeatureFlags.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.