Merging to release-5.3.9: [TT-12710]deleting All Partitioned Policies a Key is linked to does not delete the Key (#6473)

[buger]

User description

TT-12710

Summary	Deleting All Partitioned Policies a Key is linked to does not prevent Key from being used
Type	Bug
Status	In Test
Points	N/A
Labels	'24Bugsmash

---

TT-12710deleting All Partitioned Policies a Key is linked to does not delete the Key (#6473)

User description

TASK: https://tyktech.atlassian.net/browse/TT-12710
Fixed case in which trying to apply a non-existing policy error would be
swallowed when having partitioned keys.

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing
  functionality to change)
• Refactoring or add test (improvements in base code or adds test
  coverage to functionality)

Checklist

• I ensured that the documentation is up to date
• I explained why this PR updates go.mod in detail with reasoning
  why it's required
• I would like a code coverage CI quality gate exception and have
  explained why

---

PR Type

Bug fix

---

Description

• Fixed a bug where errors for non-existing policies were ignored if
  multiple policies were processed, ensuring that an error is returned
  immediately.
• Improved error handling in the Apply method of the Service to
  prevent silent failures when policies are missing.

---

Changes walkthrough 📝

	Relevant files
Bug fix	apply.go Fix error handling for non-existing policies in Apply method internal/policy/apply.go Removed logic that continued processing policies when a non-existing policy was encountered. Ensured that an error is returned immediately if a policy is not found. +0/-4

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools
and their descriptions

---

PR Type

Bug fix, Tests

---

Description

• Fixed a bug where errors for non-existing policies were ignored, ensuring proper error handling in the ApplyPolicies method.
• Enhanced test cases across multiple files to validate policy deletion behavior and its impact on API access.
• Improved test coverage for JWT session handling, multi-auth scenarios, and policy application.
• Simplified policy synchronization logic in the gateway server to ensure consistency.
• Added a utility function for deleting policies in test scenarios.

---

Changes walkthrough 📝

	Relevant files
Tests	api_test.go Enhanced test coverage for policy deletion and session updates. gateway/api_test.go Added new test cases to validate policy deletion behavior and its impact on API access. Enhanced existing test cases to include additional policies and metadata for better coverage. Adjusted assertions to ensure proper validation of access rights and applied policies. +102/-26 multiauth_test.go Updated multi-auth tests with enhanced policy validation. gateway/multiauth_test.go Updated test cases to include API ID and access rights in policy creation. Enhanced validation for multi-auth scenarios with updated policies. +7/-2      mw_jwt_test.go Enhanced JWT session tests with policy validation.              gateway/mw_jwt_test.go Added API ID and access rights to JWT-related test cases. Improved test coverage for JWT session handling and policy application. +137/-12 testutil.go Added policy deletion utility for tests.                                  gateway/testutil.go Added a utility function to delete policies during tests. +6/-0
Bug fix	middleware.go Improved error handling in ApplyPolicies method.                  gateway/middleware.go Added error handling to return an error when no valid policies are applied to a session. +4/-0      server.go Simplified policy synchronization logic.                                  gateway/server.go Simplified policy synchronization logic by always updating policiesByID. +1/-3

---

💡 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]

API Changes

--- prev.txt	2024-12-17 10:54:59.205331536 +0000
+++ current.txt	2024-12-17 10:54:56.051332550 +0000
@@ -9945,6 +9945,8 @@
 
 func (s *Test) CreateSession(sGen ...func(s *user.SessionState)) (*user.SessionState, string)
 
+func (s *Test) DeletePolicy(policyID string)
+
 func (s *Test) Do(tc test.TestCase) (*http.Response, error)
 
 func (s *Test) EnablePort(port int, protocol string)

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 6473 - Fully compliant Fully compliant requirements: Fix the issue where deleting all partitioned policies linked to a key does not delete the key. Ensure that errors for non-existing policies are not ignored when processing multiple policies. Improve error handling in the Apply method to prevent silent failures when policies are missing. Not compliant requirements:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Bug The new error handling logic in the ApplyPolicies method returns an error if no valid policies are applied. This change might have unintended side effects on existing workflows and should be carefully reviewed. Code Smell The change in syncPolicies directly assigns pols to gw.policiesByID without checking if pols is empty. This could potentially lead to overwriting existing policies with an empty map.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add a nil check for policyIDs to prevent runtime errors Ensure that the policyIDs variable is checked for nil before accessing its length to avoid potential nil pointer dereference. gateway/middleware.go [772-774] -if len(rights) == 0 && policyIDs != nil { +if len(rights) == 0 { + if policyIDs == nil { + return errors.New("policyIDs is nil") + } return errors.New("key has no valid policies to be applied") } Suggestion importance[1-10]: 8 Why: The suggestion adds a safeguard against a potential nil pointer dereference, which is a critical runtime error. This improves the robustness of the code by ensuring policyIDs is not nil before accessing its length.	8
General	Ensure safe merging of policies to prevent overwriting existing data Avoid overwriting gw.policiesByID without checking for potential conflicts or ensuring thread safety during concurrent operations. gateway/server.go [587] -gw.policiesByID = pols +gw.policiesByID = mergePolicies(gw.policiesByID, pols) Suggestion importance[1-10]: 7 Why: The suggestion addresses a potential issue with overwriting gw.policiesByID without considering existing data. By introducing a merging function, it ensures data integrity and thread safety during concurrent operations.	7
	Add a check to ensure the policy exists before deletion Verify if the policyID exists before attempting to delete it in DeletePolicy to avoid unnecessary operations or potential errors. gateway/testutil.go [853-854] -delete(s.Gw.policiesByID, policyID) +if _, exists := s.Gw.policiesByID[policyID]; exists { + delete(s.Gw.policiesByID, policyID) +} Suggestion importance[1-10]: 6 Why: The suggestion improves the DeletePolicy function by verifying the existence of the policy before attempting deletion. This avoids unnecessary operations and potential errors, enhancing the function's reliability.	6

[sonarqubecloud]

Quality Gate failed

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

See analysis details on SonarQube Cloud

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