[ES-2734] Added support for object in verified claim detail" #1568
Conversation
* Updated API and design documentation Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Updated API and design documentation Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Updated API and design documentation Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> --------- Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
* Added example Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Added 405 error code Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> --------- Signed-off-by: ase-101 <sunkadaeanusha@gmail.com>
Signed-off-by: pvsaidurga <saidurgacsea@gmail.com>
WalkthroughAdds documentation and OpenAPI schema entries for Pushed Authorization Requests (PAR) and DPoP-bound access tokens: new client configuration fields, a DPoP security scheme, renamed DPoP binding key, and multiple expanded/centralized OpenAPI schemas and required fields. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/esignet-openapi.yaml (2)
5683-5701: PAR error code semantics: align with implementation.For invalid client_id, implementation returns error="invalid_request" with error_description="invalid_client_id" (not error="invalid_client_id"). Adjust the error enum accordingly. Based on learnings, …
Apply this diff to remove the misleading enum value:
properties: error: type: string description: 'Error code, available in error response.' enum: - invalid_request - - invalid_client_id - invalid_redirect_uri - invalid_scope - invalid_acr - invalid_response_type - invalid_display - invalid_prompt
5748-5753: Name the PAR enforcement flag consistently.Text mentions “mandate_par_flow”; API config uses additionalConfig.require_pushed_authorization_requests. Use the same name everywhere.
🧹 Nitpick comments (4)
docs/design/fapi2-compliance.md (1)
40-40: Use consistent property casing for require_pushed_authorization_requests.Here it’s “Require_pushed_authorization_requests”, while OpenAPI and client-mgmt docs use “require_pushed_authorization_requests”. Align to the lower_snake_case form used in the API.
docs/esignet-openapi.yaml (2)
1448-1452: Schema vs example mismatch for scope on /authorize.Examples show multi-scope strings (e.g., “openid profile”), but the schema earlier constrains scope to single values via enum. Relax the schema there as done for PAR.
6291-6303: Enforce at least one check when check_details is present.Spec text says “MUST have at least one member”; add minItems: 1 to reflect that.
Apply this diff:
check_details: type: array description: | JSON array representing the checks done in relation to the evidence. When present this array MUST have at least one member. This is applicable only for below evidence types: 1. document 2. electronic_record 3. vouch + minItems: 1 items: $ref: '#/components/schemas/EvidenceCheckDetail'docs/design/eSignet-client-mgmt.md (1)
26-31: Good additions; add them to the example block.Documented flags are clear. Update the JSON example below to include require_pushed_authorization_requests and dpop_bound_access_tokens for completeness.
Apply this diff:
"forgot_pwd_link_required": true, - "consent_expire_in_mins": 20 + "consent_expire_in_mins": 20, + "require_pushed_authorization_requests": false, + "dpop_bound_access_tokens": false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/design/eSignet-client-mgmt.md(1 hunks)docs/design/fapi2-compliance.md(2 hunks)docs/esignet-openapi.yaml(55 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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:
docs/design/eSignet-client-mgmt.mddocs/esignet-openapi.yamldocs/design/fapi2-compliance.md
📚 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:
docs/esignet-openapi.yaml
🔇 Additional comments (4)
docs/design/fapi2-compliance.md (1)
99-101: LGTM: updated mock RP README link.Reference looks correct and current.
docs/esignet-openapi.yaml (2)
5993-6002: verified_claims supports object or array — good.Allowing both a single VerifiedClaimDetail and an array matches OIDC Identity Assurance usage.
5935-5938: DPoP security scheme tooling compatibility.OpenAPI “http” schemes are variably supported beyond Bearer/Basic/Digest. Please verify generators (e.g., Swagger UI/Codegen) accept scheme: DPoP; if not, consider modeling via a header parameter only plus Bearer/DPoP token_type semantics.
docs/design/eSignet-client-mgmt.md (1)
26-31: Keep naming consistent across docs.Ensure this file and fapi2-compliance.md both use the same lower_snake_case property names used in the OpenAPI (require_pushed_authorization_requests, dpop_bound_access_tokens).
Signed-off-by: pvsaidurga <saidurgacsea@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/esignet-openapi.yaml (1)
5681-5703: Fix PAR error response schema: required field name doesn’t match property (errorDescriptionvserror_description).In both
400and405responses for/oauth/par, therequiredarray listserrorDescription, but the actual property iserror_description. This makes the OpenAPI invalid/inconsistent and will confuse generated clients. Also, runtime behavior for PAR errors elsewhere useserror_description, noterrorDescription(see PAR negative tests). Based on learnings, …Please align
requiredwith the actual property name, and keep OAuth-style snake_case:'400': ... content: application/json: schema: type: object - required: - - error - - errorDescription + required: + - error + - error_description properties: error: type: string description: 'Error code, available in error response.' error_description: type: string description: 'Error description, available in error response.' ... '405': description: Method Not Allowed ... content: application/json: schema: type: object - required: - - error - - errorDescription + required: + - error + - error_description properties: error: type: string description: 'Error code, available in error response.' error_description: type: string description: 'Error description, available in error response.'Also applies to: 5702-5723
♻️ Duplicate comments (1)
docs/esignet-openapi.yaml (1)
1435-1451: Modelscopeas a free-form space-delimited string, not an enum.
scopeis declared as a string but constrained via an enum of single values (openid, profile, etc.) while your new examples show multi-scope values ("openid profile"). This both contradicts OAuth/OIDC semantics (space‑delimited scopes) and will cause generated clients/validators to reject valid requests for multiple scopes.Same issue appears:
- In
/authorizequery paramscope(Lines 1435–1451).- In
/oauth/parform fieldscope(Lines 5539–5550) — this was already flagged in a prior review.Recommendation:
- Keep
type: stringand the existing description + examples.- Remove the
enumblocks so multi-scope strings like"openid profile email"are valid.- - name: scope + - name: scope in: query ... schema: type: string - enum: - - openid - - profile - - email - - address - - phone - - offline_access examples: - openid profile - openid email - openid ... - scope: - type: string - description: ... - enum: - - openid - - profile - - email - - address - - phone - - offline_access + scope: + type: string + description: Specifies requested scopes as a space-delimited string; must include "openid" for OIDC. Unknown scopes are ignored.Also applies to: 5539-5550
🧹 Nitpick comments (1)
docs/design/fapi2-compliance.md (1)
32-41: Good alignment on PAR/DPoP flows; fix minor config-name casing for consistency.
- The PAR flow description (including explicit non-supported JAR request parameter) and the DPoP sequence (steps 2–7) now line up with the OpenAPI:
/oauth/par,/authorize, and/oauth/v2/tokenare correctly referenced.- The
dpop_bound_access_tokensclient flag is documented with behavior matching the OpenAPI schema.Minor suggestion: in the “Client Configuration” section for PAR you use
**Require_pushed_authorization_requests**, whereas the OpenAPI and JSON field arerequire_pushed_authorization_requests. Consider making the name lowercase here to avoid confusion for operators copying from the doc into config.-**Require_pushed_authorization_requests** property in the client configuration should be set to true to enforce PAR. By default, it is set to false. +**require_pushed_authorization_requests** property in the client configuration should be set to true to enforce PAR. By default, it is set to false.Also applies to: 58-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/design/fapi2-compliance.md(2 hunks)docs/esignet-openapi.yaml(55 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
docs/esignet-openapi.yamldocs/design/fapi2-compliance.md
📚 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:
docs/esignet-openapi.yamldocs/design/fapi2-compliance.md
📚 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:
docs/esignet-openapi.yaml
🔇 Additional comments (2)
docs/esignet-openapi.yaml (2)
5959-6416: Verified-claims schema and object/array support look correct and match the stated goal.The updated
Claim.userinfo.verified_claimsdefinition (oneOfarray ofVerifiedClaimDetailor singleVerifiedClaimDetail), along with the newVerifiedClaimDetail,ClaimDetail,FilterCriteria,EvidenceCheckDetail,EvidenceIssuer,ElectronicRecord, andClaimStatusschemas is coherent and aligned with OIDC Identity Assurance:
- Supports both object and array forms for
verified_claimsas per the spec.- Captures filtering semantics (
value/values,max_age, trust framework, evidence types, etc.).- Exposes the resolved claim status via
ClaimStatus(claim/available/verified/purpose).This looks good and satisfies the “support for object in verified claim detail” objective.
4590-4733: DPoP wiring across token, userinfo, and security schemes is consistent.The additions for DPoP look coherent:
/oauth/v2/token:
token_typeextended toBearer | DPoP.- Error codes include
invalid_dpop_proofanduse_dpop_nonce.- A global
DPoPheader parameter is defined at the path level.
/oidc/userinfo:
WWW-AUTHENTICATEaddsinvalid_dpop_proofanduse_dpop_nonce.- Security now allows
Authorization-DPoPalongsideAuthorization-access_token.- A
DPoPheader parameter is defined.
components.securitySchemes.Authorization-DPoPcorrectly modelsAuthorization: DPoP <token>.This is internally consistent and matches the described DPoP flow in the design doc.
Also applies to: 4769-4807, 5931-5937
3 participants
Summary by CodeRabbit
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.