Merging to release-5.3: fix unnecessary call to DeleteRawKey if no allowance scope is defined (TT-11721) (#6373)

buger commented 1 month ago

User description

fix unnecessary call to DeleteRawKey if no allowance scope is defined (#6373)

User description

This PR fixes an issue where DeleteRawKey is called multiple times for AllowanceScopes that are non-existent.

Types of changes

[x] 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)

PR Type

Bug fix, Tests

Description

Added a new method deleteRawKeysWithAllowanceScope in DefaultSessionManager to handle deletion of raw keys with allowance scopes.
Modified ResetQuota to use the new method for better handling of raw key deletions.
Implemented checks to skip deletion if the allowance scope is empty, preventing unnecessary calls.
Added comprehensive unit tests for the new method, covering various edge cases and ensuring correct behavior.

Changes walkthrough 📝

Relevant files

Bug fix

auth_manager.go

Add method to handle
raw key deletion with allowance scopes

gateway/auth_manager.go

Added a new method deleteRawKeysWithAllowanceScope to handle deletion
of raw keys with allowance scopes.

Modified ResetQuota to use the new method.

Added checks to skip deletion if the allowance scope is empty.

+13/-2

Tests

auth_manager_test.go

Add unit tests
for deleteRawKeysWithAllowanceScope method

gateway/auth_manager_test.go

Added unit tests for deleteRawKeysWithAllowanceScope method.

Created a mock countingStorageHandler to count delete operations.

Tested various scenarios including nil storage handler, nil session,
and sessions with/without allowance scopes.

+190/-0

💡 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

Added a new method deleteRawKeysWithAllowanceScope in DefaultSessionManager to handle deletion of raw keys with allowance scopes.
Modified ResetQuota to use the new method for better handling of raw key deletions.
Implemented checks to skip deletion if the allowance scope is empty, preventing unnecessary calls.
Added comprehensive unit tests for the new method, covering various edge cases and ensuring correct behavior.

Changes walkthrough 📝

Relevant files

Bug fix

auth_manager.go

Add method for raw key deletion with allowance scopes

gateway/auth_manager.go

Added deleteRawKeysWithAllowanceScope method to handle raw key
deletions with allowance scopes.

Modified ResetQuota to use the new method.

Implemented checks to skip deletion if the allowance scope is empty.

+13/-2

Tests

auth_manager_test.go

Add unit tests for raw key deletion method

gateway/auth_manager_test.go

Added unit tests for deleteRawKeysWithAllowanceScope.

Created countingStorageHandler for test purposes.

Covered edge cases and ensured correct behavior.

+190/-0

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

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	2
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The method `deleteRawKeysWithAllowanceScope` does not handle the case where `keyName` is empty or null. This could potentially lead to the deletion of incorrect keys or no operation being performed when it is actually needed.
⚡ Key issues to review	Error Handling: The method `deleteRawKeysWithAllowanceScope` returns silently if `store` or `session` is nil. It might be beneficial to log these events or handle them more explicitly to aid in debugging and maintainability.

github-actions[bot] commented 1 month ago

API Changes

no api changes detected

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Handle potential errors from `store.DeleteRawKey` to improve robustness ___ To enhance the robustness of the `deleteRawKeysWithAllowanceScope` function, consider handling potential errors from `store.DeleteRawKey(rawKey)`. This could involve logging the error or incorporating error handling logic. [gateway/auth_manager.go [95]](https://github.com/TykTechnologies/tyk/pull/6375/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR95-R95) ```diff -store.DeleteRawKey(rawKey) +if !store.DeleteRawKey(rawKey) { + log.Errorf("Failed to delete raw key: %s", rawKey) +} ``` Suggestion importance[1-10]: 9 Why: Handling potential errors from `store.DeleteRawKey` is crucial for robustness, ensuring that failures are logged and can be addressed.	9
Enhancement	Add unit tests for `deleteRawKeysWithAllowanceScope` with edge case values for `keyName` ___ Add a unit test to verify that `deleteRawKeysWithAllowanceScope` correctly handles cases where `keyName` is empty or contains special characters, to ensure robustness against different input scenarios. [gateway/auth_manager.go [420]](https://github.com/TykTechnologies/tyk/pull/6375/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR420-R420) ```diff -sessionManager.deleteRawKeysWithAllowanceScope(handler, session, "keyName") +sessionManager.deleteRawKeysWithAllowanceScope(handler, session, "") +sessionManager.deleteRawKeysWithAllowanceScope(handler, session, "key@Name#123") ``` Suggestion importance[1-10]: 8 Why: Adding unit tests for edge cases ensures the function handles various input scenarios robustly, which is important for comprehensive testing.	8
Possible issue	Add validation for `acl.AllowanceScope` to ensure it meets expected criteria before using it ___ Consider checking if `acl.AllowanceScope` is not only non-empty but also valid according to expected formats or values before constructing the `rawKey`. This can prevent potential issues with invalid keys being used in operations. [gateway/auth_manager.go [91-95]](https://github.com/TykTechnologies/tyk/pull/6375/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR91-R95) ```diff -if acl.AllowanceScope == "" { +if acl.AllowanceScope == "" \|\| !isValidAllowanceScope(acl.AllowanceScope) { continue } rawKey := QuotaKeyPrefix + acl.AllowanceScope + "-" + keyName store.DeleteRawKey(rawKey) ``` Suggestion importance[1-10]: 8 Why: Adding validation for `acl.AllowanceScope` can prevent potential issues with invalid keys, improving the robustness and reliability of the code.	8
Maintainability	Refactor raw key deletion logic into a separate method to improve code organization ___ Refactor the loop in `deleteRawKeysWithAllowanceScope` to a separate method to improve readability and maintainability. This method could handle the construction and deletion of the raw key. [gateway/auth_manager.go [90-95]](https://github.com/TykTechnologies/tyk/pull/6375/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR90-R95) ```diff for _, acl := range session.AccessRights { + handleAllowanceScopeDeletion(acl, store, keyName) +} + +func handleAllowanceScopeDeletion(acl user.AccessDefinition, store storage.Handler, keyName string) { if acl.AllowanceScope == "" { - continue + return } rawKey := QuotaKeyPrefix + acl.AllowanceScope + "-" + keyName - store.DeleteRawKey(rawKey) + if !store.DeleteRawKey(rawKey) { + log.Errorf("Failed to delete raw key: %s", rawKey) + } } ``` Suggestion importance[1-10]: 7 Why: Refactoring the loop into a separate method improves readability and maintainability, though it is more of a code organization enhancement rather than a critical fix.	7