[TT-12897] Merge path based permissions when combining policies

[titpetric]

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

---

PR uses custom policies to combine several policies with access rights set.

Since a map was in the path, user API for custom policies needed an extension to preserve policy ID order. The existing function returning a map didn't handle json decode errors properly and go semantics when looping over maps don't preserve this order, but it's random so tests would fail. Verified with task stress.

Issue: https://tyktech.atlassian.net/browse/TT-12897

---

PR Type

Bug fix, Enhancement, Tests

---

Description

• Enhanced policy application logic by introducing MergeAllowedURLs to merge allowed URLs efficiently.
• Refactored Store to use a slice for policies, and introduced StoreMap for unordered policy storage.
• Improved custom policy handling by adding GetCustomPolicies to preserve policy order.
• Updated tests to ensure proper application of policies and added new tests for MergeAllowedURLs.
• Updated Taskfile to include a new stress task for running stress tests.

---

Changes walkthrough 📝

	Relevant files
Enhancement	apply.go Enhance policy application logic and logging                          internal/policy/apply.go Introduced MergeAllowedURLs function to merge allowed URLs. Updated Logger function to return a logrus.Entry. Changed session.CustomPolicies() to session.GetCustomPolicies(). +9/-17    store.go Refactor Store to use slice for policies                                  internal/policy/store.go Changed Store to use a slice for policies. Updated methods to accommodate slice-based storage. +20/-7    store_map.go Add StoreMap for unordered policy storage                                internal/policy/store_map.go Introduced StoreMap for unordered policy storage. Implemented methods for StoreMap. +46/-0    util.go Introduce MergeAllowedURLs and remove unused functions      internal/policy/util.go Added MergeAllowedURLs function for merging URL access specs. Removed copyAllowedURLs and contains functions. +46/-70  custom_policies.go Enhance custom policies handling with order preservation  user/custom_policies.go Added GetCustomPolicies to preserve policy order. Updated CustomPolicies to use GetCustomPolicies. +21/-7
Tests	apply_test.go Update tests for policy application                                            internal/policy/apply_test.go Added initialization of policy.Service in tests. Ensured Apply method is tested with assert.NoError. +29/-28  util_test.go Add tests for MergeAllowedURLs function                                    internal/policy/util_test.go Added tests for MergeAllowedURLs function. +64/-0
Configuration changes	Taskfile.yml Update Taskfile with stress test task                                        internal/policy/Taskfile.yml Added stress task for running stress tests. Updated default task to include test. +16/-0

---

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

[buger]

I'm a bot and I 👍 this PR title. 🤖

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Function Redundancy The function appendIfMissing in util.go has been modified to use the slices package for checking containment, which simplifies the function but may not be necessary if the function is only used in one place. Consider if the added dependency is justified or if the function can be further optimized. Logic Change The change in the method of merging allowed URLs from copyAllowedURLs to MergeAllowedURLs in apply.go could potentially alter the behavior of URL merging logic. This needs thorough testing to ensure that the new merging logic correctly handles all edge cases as expected.

[github-actions]

API Changes

--- prev.txt	2024-10-01 15:03:36.900478815 +0000
+++ current.txt	2024-10-01 15:03:31.237459834 +0000
@@ -12736,6 +12736,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
@@ -12775,6 +12781,7 @@
     Reset marks the session as not modified, skipping related updates.
 
 func (s *SessionState) SetCustomPolicies(list []Policy)
+    SetCustomPolicies sets custom policies into session metadata.
 
 func (s *SessionState) SetKeyHash(hash string)
 

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Improve the merging logic in MergeAllowedURLs to handle duplicate URLs more effectively Ensure that MergeAllowedURLs correctly handles the merging of methods for URLs that appear in both s1 and s2 to prevent any potential data loss or errors in access control. internal/policy/apply.go [30] -merged[url] = appendIfMissing(v, r.Methods...) +if existingMethods, exists := merged[url]; exists { + merged[url] = appendIfMissing(existingMethods, r.Methods...) +} else { + merged[url] = r.Methods +} Suggestion importance[1-10]: 7 Why: This suggestion addresses a potential bug by ensuring that the merging logic correctly handles duplicate URLs, which is important for maintaining accurate access control.	7
Best practice	Ensure consistent usage of store initialization methods to prevent data handling inconsistencies Validate that NewStore and NewStoreMap are used consistently throughout the codebase to prevent inconsistencies in data handling and storage mechanisms. internal/policy/apply.go [117] -storage := NewStore(customPolicies) +storage := NewStoreMap(customPolicies) # Ensure consistent usage of either NewStore or NewStoreMap Suggestion importance[1-10]: 6 Why: Ensuring consistent usage of store initialization methods can prevent data handling inconsistencies, which is beneficial for maintainability and correctness.	6
	Add error handling or logging for duplicate methods in URL access specifications Consider handling the case where slices.Contains(dest, v) returns true by logging or handling the duplicate method to avoid silent failures or unexpected behaviors. internal/policy/util.go [53-58] for _, v := range in { if slices.Contains(dest, v) { + // Handle or log duplicate method continue } dest = append(dest, v) } Suggestion importance[1-10]: 5 Why: The suggestion to handle or log duplicate methods in URL access specifications can improve code robustness by preventing silent failures, although it is not critical for functionality.	5

[sonarcloud]

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

[jeffy-mathew]

why is this changed?

[titpetric]

Custom policies doesn't preserve order (map policyID => policy) and would fail tests due to the order changes. The PolicyIDs() doesn't return ids always in the same order, meaning Methods can be returned out of order failing the assertion(s), requiring a deeper assertion to account for the typical go range over map behaviour.

Store{} preserves order.
StoreMap{} doesn't.

-customPolicies, err := session.CustomPolicies()      <--- used to be a map
+customPolicies, err := session.GetCustomPolicies()   <--- provides a list of policies now
if err != nil {
        policyIDs = session.PolicyIDs()
} else {
        storage = NewStore(customPolicies)
        policyIDs = storage.PolicyIDs()               <--- maintains policy list order
}

This change ensures a deterministic order for applying policies, and in turn the merged access rights Methods are sorted with the order the policies are applied in, simplifying the assertion in tests.

No other uses in gw. If the implementing ticket wasn't so bad with detail, I could easily consider custom policies functions internal scope. The underlying storage is a []any inside a map[string]any MetaData.

Considered but rejected a deeper assertion in tests for methods (ElementsEqual...). Not preserving custom policy order was a code smell, the added GetCustomPolicies works around the restriction.

[titpetric]

Sidenote:

• This could have been a CustomPolicies []Policy on SessionState, not sure why it needs the API around a map[string]any;
• I'm not sure why custom policies were stuck into metadata other than to piggyback on an accessible key/value store field. No public API changes ensure whatever is coupled to it continues to work.