diff --git a/ideas/should_fix_role_docs.md b/ideas/should_fix_role_docs.md new file mode 100644 index 000000000..e531db931 --- /dev/null +++ b/ideas/should_fix_role_docs.md @@ -0,0 +1,306 @@ +# Endpoints That Need Role Documentation Fixes + +This document identifies OBP API endpoints that have issues with role documentation or redundant role checks. + +## Issue Types + +1. **Missing Role in ResourceDoc**: Endpoint has `hasEntitlement` check in code but NO role specified in ResourceDoc `Some(List(...))` +2. **Duplicate Role Check**: Endpoint has role in ResourceDoc AND redundant `hasEntitlement` check in for comprehension + +## Why This Matters + +- **ResourceDoc roles** are the canonical source of truth for API documentation and are automatically enforced by the framework +- **Redundant hasEntitlement checks** in for comprehensions are unnecessary and violate DRY principle +- **Missing ResourceDoc roles** mean the API documentation doesn't reflect actual authorization requirements + +## Methodology to Find Issues + +```bash +# Find all hasEntitlement calls +grep -n "hasEntitlement.*callContext)" obp-api/src/main/scala/code/api/v*/APIMethods*.scala + +# For each endpoint with hasEntitlement, check if ResourceDoc has role defined +# Look for the ResourceDoc definition above the endpoint and check for Some(List(...)) +``` + +## Known Issues to Fix + +### v3.1.0 + +#### getCustomerByCustomerId +- **File**: `obp-api/src/main/scala/code/api/v3_1_0/APIMethods310.scala` +- **Line**: ~1285 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetCustomersAtOneBank))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, canGetCustomersAtOneBank, callContext)` +- **Action**: Remove hasEntitlement from for comprehension + +#### getCustomerByCustomerNumber +- **File**: `obp-api/src/main/scala/code/api/v3_1_0/APIMethods310.scala` +- **Line**: ~1328 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetCustomersAtOneBank))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, canGetCustomersAtOneBank, callContext)` +- **Action**: Remove hasEntitlement from for comprehension + +### v5.1.0 + +#### getCustomersByLegalName +- **File**: `obp-api/src/main/scala/code/api/v5_1_0/APIMethods510.scala` +- **Line**: ~2915 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetCustomersAtOneBank))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, canGetCustomersAtOneBank, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ⚠️ May have been fixed - verify + +### v6.0.0 + +#### getCustomersByLegalName +- **Status**: ✅ FIXED - Redundant check removed + +#### getCustomerByCustomerId +- **Status**: ✅ FIXED - Redundant check removed + +#### getCustomerByCustomerNumber +- **Status**: ✅ FIXED - Redundant check removed + +#### deleteEntitlement +- **File**: `obp-api/src/main/scala/code/api/v6_0_0/APIMethods600.scala` +- **Line**: ~2531 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canDeleteEntitlementAtAnyBank))` (line 2524) +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, canDeleteEntitlementAtAnyBank, callContext)` (line 2531) +- **Action**: TODO - Verify if this is intentional pattern or should remove hasEntitlement from for comprehension +- **Status**: ⚠️ TO REVIEW - Just added in v6.0.0, copied from v2.0.0 which also has duplicate check +- **Note**: This endpoint was newly added to v6.0.0 to support modified return values + +#### getMetrics +- **File**: `obp-api/src/main/scala/code/api/v6_0_0/APIMethods600.scala` +- **Line**: ~1447 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canReadMetrics))` (line 1438) +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, canReadMetrics, callContext)` (line 1447) +- **Action**: TODO - Verify if this is intentional pattern or should remove hasEntitlement from for comprehension +- **Status**: ⚠️ TO REVIEW - Just added in v6.0.0, copied from v5.1.0 which also has duplicate check +- **Note**: This endpoint has automatic from_date default and empty metrics warning log + +### v2.0.0 + +#### getKycDocuments +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~487 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetAnyKycDocuments))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, ApiRole.canGetAnyKycDocuments, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 477) + +#### getKycMedia +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~521 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetAnyKycMedia))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, ApiRole.canGetAnyKycMedia, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 514) + +#### getKycChecks +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~555 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetAnyKycChecks))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, ApiRole.canGetAnyKycChecks, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 548) + +#### getKycStatuses +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~588 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetAnyKycStatuses))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, ApiRole.canGetAnyKycStatuses, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 581) + +#### getSocialMediaHandles +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~621 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetSocialMediaHandles))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bank.bankId.value, u.userId, canGetSocialMediaHandles, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 614) + +#### addKycDocument +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~660 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canAddKycDocument))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, ApiRole.canAddKycDocument, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 649) + +#### addKycMedia +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~710 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canAddKycMedia))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, ApiRole.canAddKycMedia, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 701) + +#### addKycCheck +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~760 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canAddKycCheck))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, ApiRole.canAddKycCheck, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 751) + +#### addKycStatus +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~811 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canAddKycStatus))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, ApiRole.canAddKycStatus, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 802) + +#### addSocialMediaHandle +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~861 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canAddSocialMediaHandle))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bank.bankId.value, u.userId, canAddSocialMediaHandle, cc.callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 852) + +#### deleteEntitlement +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~1916 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canDeleteEntitlementAtAnyBank))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, canDeleteEntitlementAtAnyBank, cc.callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role added to ResourceDoc (line 1910) + +#### getAllEntitlements +- **File**: `obp-api/src/main/scala/code/api/v2_0_0/APIMethods200.scala` +- **Line**: ~1954 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetEntitlementsForAnyUserAtAnyBank))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, canGetEntitlementsForAnyUserAtAnyBank, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role added to ResourceDoc (line 1949) + +### v2.1.0 + +#### sandboxDataImport +- **File**: `obp-api/src/main/scala/code/api/v2_1_0/APIMethods210.scala` +- **Line**: ~140 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canCreateSandbox))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement("", u.userId, canCreateSandbox, cc.callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 131) + +#### addCardForBank +- **File**: `obp-api/src/main/scala/code/api/v2_1_0/APIMethods210.scala` +- **Line**: ~995 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canCreateCardsForBank))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, ApiRole.canCreateCardsForBank, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role already in ResourceDoc (line 988) + +### v1.3.0 + +#### getCardsForBank +- **File**: `obp-api/src/main/scala/code/api/v1_3_0/APIMethods130.scala` +- **Line**: ~105 +- **Issue**: Duplicate - has role in ResourceDoc AND hasEntitlement in code +- **ResourceDoc Role**: `Some(List(canGetCardsForBank))` +- **Code Check**: `_ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, ApiRole.canGetCardsForBank, callContext)` +- **Action**: Remove hasEntitlement from for comprehension +- **Status**: ✅ FIXED - Role added to ResourceDoc (line 99) + +## How to Fix + +### For Missing Roles in ResourceDoc + +1. Locate the ResourceDoc definition for the endpoint +2. Add the role to the ResourceDoc: `Some(List(roleNameHere))` +3. Example: + ```scala + resourceDocs += ResourceDoc( + getKycDocuments, + apiVersion, + "getKycDocuments", + "GET", + "/banks/BANK_ID/customers/CUSTOMER_ID/kyc_documents", + "Get KYC Documents", + "...", + EmptyBody, + kycDocumentsJSON, + List(UserNotLoggedIn, UnknownError), + List(apiTagKyc), + Some(List(canGetAnyKycDocuments)) // <-- ADD THIS + ) + ``` + +### For Duplicate Role Checks + +1. Verify the role is properly defined in ResourceDoc +2. Write tests that verify role checking works (401/403 errors without role, 200 with role) +3. Remove the redundant `hasEntitlement` line from the for comprehension +4. Remove the `authenticatedAccess` line if it's only used for the hasEntitlement check +5. Example: + ```scala + // BEFORE (redundant) + for { + (Full(u), callContext) <- authenticatedAccess(cc) + _ <- NewStyle.function.hasEntitlement(bankId.value, u.userId, canGetCustomersAtOneBank, callContext) + (customer, callContext) <- NewStyle.function.getCustomerByCustomerId(customerId, callContext) + } yield { ... } + + // AFTER (clean) + for { + (customer, callContext) <- NewStyle.function.getCustomerByCustomerId(customerId, cc.callContext) + } yield { ... } + ``` + +## Testing Requirements + +Before removing any hasEntitlement check: +1. Ensure the role is in ResourceDoc +2. Write comprehensive tests covering: + - Test without credentials → expects 401 + - Test without role → expects 403 with UserHasMissingRoles + - Test with role → expects success (200/201/etc) + +## Progress Tracking + +- ✅ Roles in ResourceDoc: 17 endpoints + - 3 in v6.0.0 (with tests) + - 10 in v2.0.0 (4 GET KYC + 4 Add KYC + 2 Entitlement - 2 roles just added) + - 2 in v2.1.0 + - 1 in v1.3.0 (role just added) + - All 17 still have redundant hasEntitlement checks that should be removed +- 🔍 To Review: v3.1.0 (3 endpoints), v5.1.0 (1 endpoint) +- ⚠️ Dynamic endpoints may need special handling + +## Notes + +- Dynamic endpoints and dynamic entities have their own role management system +- Some v2.0.0 endpoints are marked as "OldStyle" and may have different patterns +- Priority should be given to newer API versions (v5.x, v6.x) that are actively used + +## Related Documentation + +- See `developer_notes_roles.md` for role naming conventions +- See `release_notes.md` (18/11/2025) for role name changes +- See test files in `obp-api/src/test/scala/code/api/v6_0_0/CustomerTest.scala` for testing patterns + +Last Updated: 2025-01-XX \ No newline at end of file