[TT-12897] [backport 5.3] Merge path based permissions

[titpetric]

PR Type

Enhancement, Tests

---

Description

• Introduced a new Repository interface in the gateway package for managing policies, with methods for retrieving and updating policies.
• Optimized URL merging logic in middleware.go by replacing copyAllowedURLs with mergeAllowedURLs.
• Added MergeAllowedURLs function in internal/policy/util.go for efficient URL access spec merging.
• Removed tests for the deprecated copyAllowedURLs function and added new integration tests for MergeAllowedURLs.
• Added a Taskfile for automating tests, benchmarks, and code formatting.

---

Changes walkthrough 📝

	Relevant files
Enhancement	gateway.go Add policy management interface and methods to Gateway      gateway/gateway.go Added a new Repository interface for policy management. Implemented methods for policy retrieval and management in Gateway. Introduced concurrency-safe access to policies using mutex locks. +62/-0    middleware.go Optimize URL merging logic in middleware                                  gateway/middleware.go Replaced copyAllowedURLs with mergeAllowedURLs for merging URL access specs. Removed redundant code for merging allowed URLs. +4/-33    repository.go Introduce Repository interface for policy retrieval            internal/policy/repository.go - Introduced a `Repository` interface for policy retrieval. +11/-0    util.go Add utility functions for URL merging and slice operations internal/policy/util.go Added MergeAllowedURLs function for merging URL access specs. Introduced utility functions for slice operations. +105/-0
Tests	middleware_test.go Remove tests for deprecated URL copy function                        gateway/middleware_test.go - Removed tests for `copyAllowedURLs` function. +0/-62    allowed_urls_test.go Add integration tests for URL merging functionality            tests/policy/allowed_urls_test.go Added integration tests for MergeAllowedURLs function. Validated merged access rights in session state. +156/-0  shim.go Add test shims for policy package                                                tests/policy/shim.go - Added test shims for policy package. +9/-0
Configuration changes	Taskfile.yml Add Taskfile for test automation and coverage                        tests/policy/Taskfile.yml Added Taskfile for running tests and benchmarks. Configured tasks for test coverage and formatting. +53/-0

---

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

[github-actions[bot]]

API Changes

--- prev.txt    2024-10-10 16:20:19.329276586 +0000
+++ current.txt 2024-10-10 16:20:16.488265826 +0000
@@ -8199,6 +8199,15 @@

 func (gw *Gateway) NotifyCurrentServerStatus()

+func (gw *Gateway) PolicyByID(id string) (user.Policy, bool)
+    PolicyByID will return a Policy matching the passed Policy ID.
+
+func (gw *Gateway) PolicyCount() int
+    PolicyCount will return the number of policies loaded in the gateway.
+
+func (gw *Gateway) PolicyIDs() []string
+    PolicyIDs returns a list of IDs for each policy loaded in the gateway.
+
 func (gw *Gateway) ProcessOauthClientsOps(clients map[string]string)
     ProcessOauthClientsOps performs the appropriate action for the received
     clients it can be any of the Create,Update and Delete operations
@@ -8216,6 +8225,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

@@ -9486,6 +9502,11 @@
     TickOk triggers a reload and ensures a queue happened and a reload cycle
     happens. This will block until all the cases are met.

+type Repository interface {
+   policy.Repository
+}
+    Repository is a description of our Gateway API promises.
+
 type RequestDefinition struct {
    Method      string            `json:"method"`
    Headers     map[string]string `json:"headers"`
@@ -11672,6 +11693,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/quota

 # Package: ./tests/regression

[github-actions[bot]]

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 Concurrency Concerns The implementation of policy management methods in the Gateway class uses mutexes for thread safety. However, the use of defer for unlocking might lead to performance issues under high load. Consider evaluating the impact and exploring more efficient locking strategies or lock-free approaches. Refactoring Needed The replacement of 'copyAllowedURLs' with 'mergeAllowedURLs' changes the behavior of URL merging in ACLs. Ensure that this new approach aligns with the intended business logic and does not introduce regressions or unexpected behaviors. Performance Optimization The 'MergeAllowedURLs' function uses maps and slices to merge URLs and methods, which might be inefficient with large data sets. Consider optimizing this function to handle large inputs more efficiently, possibly by minimizing memory allocations and improving iteration strategies.

[github-actions[bot]]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Prevent potential nil map panics by ensuring map initialization ___ **Ensure that the policiesByID map is properly initialized before any operations are performed on it in methods like PolicyIDs, PolicyByID, PolicyCount, and SetPoliciesByID to prevent potential nil map panics.** [gateway/gateway.go [21-25]](https://github.com/TykTechnologies/tyk/pull/6627/files#diff-17cb8b37eda9018fe1c6cdb5f96b3fc948fc8ba49bc516987b8269576db9fcd4R21-R25) ```diff +if gw.policiesByID == nil { + gw.policiesByID = make(map[string]user.Policy) +} result := make([]string, 0, len(gw.policiesByID)) for id := range gw.policiesByID { result = append(result, id) } return result ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a critical issue by ensuring that the `policiesByID` map is initialized before use, preventing potential nil map panics which could lead to runtime errors.	9
Performance	Improve performance of method merging by using a map for deduplication ___ **Optimize the appendIfMissing function by using a map for lookups instead of a slice to improve performance for large input sizes.** [internal/policy/util.go [53-58]](https://github.com/TykTechnologies/tyk/pull/6627/files#diff-0323c3da13f08a9ccd340ac04208d680856354fd566dffcad925fa6645639955R53-R58) ```diff +seen := make(map[string]struct{}) +for _, v := range dest { + seen[v] = struct{}{} +} for _, v := range in { - if slices.Contains(dest, v) { - continue + if _, exists := seen[v]; !exists { + dest = append(dest, v) + seen[v] = struct{}{} } - dest = append(dest, v) } ``` Suggestion importance[1-10]: 8 Why: This suggestion optimizes the `appendIfMissing` function by using a map for lookups, which can significantly improve performance for large input sizes by reducing the time complexity of deduplication.	8
Enhancement	Enhance robustness by adding error handling in policy updates ___ **Add error handling for the SetPoliciesByID method to handle potential issues when updating policies, such as when the policiesByID map is nil or when an invalid policy is provided.** [gateway/gateway.go [59-61]](https://github.com/TykTechnologies/tyk/pull/6627/files#diff-17cb8b37eda9018fe1c6cdb5f96b3fc948fc8ba49bc516987b8269576db9fcd4R59-R61) ```diff +if gw.policiesByID == nil { + return errors.New("policiesByID map is not initialized") +} for _, pol := range pols { + if pol.ID == "" { + return errors.New("policy ID is empty") + } gw.policiesByID[pol.ID] = pol } ``` Suggestion importance[1-10]: 7 Why: Adding error handling in the `SetPoliciesByID` method improves robustness by checking for nil maps and invalid policies, which can prevent potential issues during policy updates.	7

[sonarcloud[bot]]

Quality Gate failed

Failed conditions
51.5% Coverage on New Code (required ≥ 80%)
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