[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-10 13:49:20.936564915 +0000
+++ current.txt	2024-10-10 13:49:15.005508863 +0000
@@ -7651,6 +7651,13 @@
 	MsgCertificateExpired                      = "Certificate has expired"
 )
 const (
+	Pass      = model.Pass
+	Fail      = model.Fail
+	Warn      = model.Warn
+	Datastore = model.Datastore
+	System    = model.System
+)
+const (
 	// Zero value - the service is open and ready to use
 	OPEN = 0
 
@@ -8790,6 +8797,13 @@
 func (gw *Gateway) SetNodeID(nodeID string)
     SetNodeID writes NodeID safely.
 
+func (gw *Gateway) SetPolicies(pols map[string]user.Policy)
+    SetPolicies updates the internal policy map with a new policy map.
+
+func (gw *Gateway) SetPoliciesByID(pols ...user.Policy)
+    SetPoliciesByID will update the internal policiesByID map with new policies.
+    The key used will be the policy ID.
+
 func (gw *Gateway) SetupNewRelic() (app newrelic.Application)
     SetupNewRelic creates new newrelic.Application instance
 
@@ -9031,6 +9045,12 @@
 	RevProxyTransform RevProxyTransform `mapstructure:"rev_proxy_header_cleanup" bson:"rev_proxy_header_cleanup" json:"rev_proxy_header_cleanup"`
 }
 
+type HealthCheckItem = model.HealthCheckItem
+
+type HealthCheckResponse = model.HealthCheckResponse
+
+type HealthCheckStatus = model.HealthCheckStatus
+
 type HealthCheckValues struct {
 	ThrottledRequestsPS float64 `bson:"throttle_reqests_per_second,omitempty" json:"throttle_reqests_per_second"`
 	QuotaViolationsPS   float64 `bson:"quota_violations_per_second,omitempty" json:"quota_violations_per_second"`
@@ -12389,6 +12409,23 @@
 
 package coprocess // import "github.com/TykTechnologies/tyk/tests/coprocess"
 
+# Package: ./tests/policy
+
+package policy // import "github.com/TykTechnologies/tyk/tests/policy"
+
+
+CONSTANTS
+
+const DefaultOrg = "default-org-id"
+
+VARIABLES
+
+var StartTest = gateway.StartTest
+
+TYPES
+
+type APISpec = gateway.APISpec
+
 # Package: ./tests/proxy
 
 package proxy // import "github.com/TykTechnologies/tyk/tests/proxy"
@@ -12898,6 +12935,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
@@ -12937,6 +12980,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 passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[titpetric]

/release to release-5.3

[tykbot]

Working on it! Note that it can take a few minutes.

[tykbot]

@titpetric Succesfully merged PR