Merging to release-5.3: [TT-12897/TT-13284] Add additional partitioned test case, fix ordering issue (#6635)

[buger]

User description

[TT-12897/TT-13284] Add additional partitioned test case, fix ordering issue (#6635)

User description

TT-12897

Summary	[Security]Path-Based Permissions permissions in policies are not preserved when policies are combined
Type	Bug
Status	In Dev
Points	N/A
Labels	customer_bug, jira_escalated, QA_Fail

---

Subtask: https://tyktech.atlassian.net/browse/TT-13284
Parent: https://tyktech.atlassian.net/browse/TT-12897

---

PR Type

Bug fix, Tests

---

Description

• Fixed a bug in applyPartitions function to ensure rights map is
  filled with known APIs, ensuring policies with ACL rights are honored
  even if not first.
• Improved merging logic for RestrictedTypes, AllowedTypes, and
  FieldAccessRights to handle empty cases and intersections correctly.
• Added test cases to verify the correct application of ACL and rate
  limits from custom policies, ensuring the order of policies does not
  affect the outcome.

---

Changes walkthrough 📝

	Relevant files
Bug fix	apply.go Fix policy merging and ordering issues in partitioned policies internal/policy/apply.go Ensure rights map is filled with known APIs to honor policies. Modify merging logic for RestrictedTypes, AllowedTypes, and FieldAccessRights. Fix ordering issue in policy application by using previously seen rights. +41/-21
Tests	apply_test.go Add test cases for ACL and rate limit application                internal/policy/apply_test.go Add test cases for applying ACL from custom policies. Verify correct application of rate limits and access rights. +47/-0

---

💡 PR-Agent usage: Comment /help "your question" on any pull
request to receive relevant information

Co-authored-by: Tit Petric [email protected]

---

PR Type

Bug fix, Tests

---

Description

• Fixed a bug in ApplyPolicies function to ensure rights map is filled with known APIs, ensuring policies with ACL rights are honored even if not first.
• Improved merging logic for RestrictedTypes, AllowedTypes, and FieldAccessRights to handle empty cases and intersections correctly.
• Added test cases to verify the correct application of ACL and rate limits from custom policies, ensuring the order of policies does not affect the outcome.
• Enhanced custom policies handling by adding GetCustomPolicies to preserve policy order and improve error handling.

---

Changes walkthrough 📝

	Relevant files
Bug fix	middleware.go Fix policy merging and ordering issues in partitioned policies gateway/middleware.go Ensure rights map is filled with known APIs to honor policies. Modify merging logic for RestrictedTypes, AllowedTypes, and FieldAccessRights. Fix ordering issue in policy application by using previously seen rights. +40/-21
Tests	apply_acl_test.go Add test cases for ACL and rate limit application                tests/policy/apply_acl_test.go Add test cases for applying ACL from custom policies. Verify correct application of rate limits and access rights. +68/-0
Enhancement	shim.go Add type aliases for gateway components                                    tests/policy/shim.go Add type aliases for Gateway, APISpec, and BaseMiddleware. +5/-1      custom_policies.go Enhance custom policies handling with order preservation  user/custom_policies.go Add GetCustomPolicies to preserve policy order. Modify CustomPolicies to use GetCustomPolicies. Add error handling for JSON operations. +21/-7

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[github-actions]

API Changes

--- prev.txt	2024-10-15 11:08:51.066752071 +0000
+++ current.txt	2024-10-15 11:08:48.145748965 +0000
@@ -11710,6 +11710,10 @@
 
 type APISpec = gateway.APISpec
 
+type BaseMiddleware = gateway.BaseMiddleware
+
+type Gateway = gateway.Gateway
+
 # Package: ./tests/quota
 
 # Package: ./tests/regression
@@ -12118,6 +12122,12 @@
     Clone returns a fresh copy of s
 
 func (s *SessionState) CustomPolicies() (map[string]Policy, error)
+    CustomPolicies returns a map of custom policies on the session. To preserve
+    policy order, use GetCustomPolicies instead.
+
+func (s *SessionState) GetCustomPolicies() ([]Policy, error)
+    GetCustomPolicies is like CustomPolicies but returns the list, preserving
+    order.
 
 func (s *SessionState) GetQuotaLimitByAPIID(apiID string) (int64, int64, int64, int64)
     GetQuotaLimitByAPIID return quota max, quota remaining, quota renewal rate
@@ -12150,6 +12160,7 @@
     ApplyPolicies is empty.
 
 func (s *SessionState) SetCustomPolicies(list []Policy)
+    SetCustomPolicies sets custom policies into session metadata.
 
 func (s *SessionState) SetKeyHash(hash string)
 

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 6635 - Fully compliant Fully compliant requirements: Fix policy merging and ordering issues in partitioned policies. Add test cases for ACL and rate limit application.

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Bug Ensure the new logic for merging ACLs, rate limits, and other policy partitions does not introduce any regressions or unexpected behavior. Particularly, the changes in how rights are initialized and merged could affect existing functionality.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Initialize the map to prevent nil map assignment errors Ensure that the rights map is initialized before attempting to assign values to avoid potential nil map assignment panics. gateway/middleware.go [498] +if rights == nil { + rights = make(map[string]user.AccessDefinition) +} rights[k] = user.AccessDefinition{} Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential runtime panic by ensuring the rights map is initialized before assignment. This is crucial for preventing nil map assignment errors, which could lead to application crashes.	8
Performance	Optimize the merging of type fields by using a map for faster lookups Use a more efficient method to check and add unique types to r.RestrictedTypes and r.AllowedTypes to avoid nested loops and improve performance. gateway/middleware.go [518-526] +restrictedTypeMap := make(map[string]int) +for i, rt := range r.RestrictedTypes { + restrictedTypeMap[rt.Name] = i +} for _, t := range v.RestrictedTypes { - for ri, rt := range r.RestrictedTypes { - if t.Name == rt.Name { - r.RestrictedTypes[ri].Fields = intersection(rt.Fields, t.Fields) - } + if i, ok := restrictedTypeMap[t.Name]; ok { + r.RestrictedTypes[i].Fields = intersection(r.RestrictedTypes[i].Fields, t.Fields) + } else { + r.RestrictedTypes = append(r.RestrictedTypes, t) } } Suggestion importance[1-10]: 7 Why: The suggestion improves performance by replacing nested loops with a map for faster lookups, which is beneficial for optimizing the merging process of type fields. This change enhances efficiency, especially when dealing with large datasets.	7
Maintainability	Refactor repetitive conditional checks into a separate function for clarity and maintainability Refactor the conditional checks for len(r.FieldAccessRights) == 0 and similar checks to a separate function to reduce code duplication and improve readability. gateway/middleware.go [547-561] -if len(r.FieldAccessRights) == 0 { - r.FieldAccessRights = v.FieldAccessRights -} else { - for _, far := range v.FieldAccessRights { - exists := false - for i, rfar := range r.FieldAccessRights { - if far.TypeName == rfar.TypeName && far.FieldName == rfar.FieldName { - exists = true - mergeFieldLimits(&r.FieldAccessRights[i].Limits, far.Limits) - } - } - if !exists { - r.FieldAccessRights = append(r.FieldAccessRights, far) - } - } -} +mergeFieldAccessRights(r, v) Suggestion importance[1-10]: 6 Why: The suggestion enhances code readability and maintainability by refactoring repetitive conditional checks into a separate function. This reduces code duplication and improves clarity, making the codebase easier to manage.	6

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint