fix unnecessary call to DeleteRawKey if no allowance scope is defined (TT-11721)

pvormste commented 1 month ago

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

github-actions[bot] commented 1 month ago

API Changes

no api changes detected

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 `AccessRights` is nil. This could lead to a nil pointer dereference if `session.AccessRights` is not initialized.
⚡ Key issues to review	Code Quality: The method `deleteRawKeysWithAllowanceScope` checks for `store == nil` and `session == nil` at the beginning, which is good for preventing nil dereferences. However, it might be beneficial to also log these occurrences or handle them more gracefully than just returning, to aid in debugging and operational monitoring.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Handle errors from `DeleteRawKey` to ensure deletion success is logged and managed ___ The function `deleteRawKeysWithAllowanceScope` should handle potential errors returned by `store.DeleteRawKey`. Ignoring these errors can lead to unnoticed failures in key deletion. [gateway/auth_manager.go [95]](https://github.com/TykTechnologies/tyk/pull/6373/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR95-R95) ```diff -store.DeleteRawKey(rawKey) +if err := store.DeleteRawKey(rawKey); err != nil { + log.Errorf("Failed to delete raw key %s: %v", rawKey, err) +} ``` Suggestion importance[1-10]: 9 Why: Handling potential errors from `DeleteRawKey` is crucial for ensuring that failures in key deletion are noticed and managed, which improves the reliability of the code.	9
Enhancement	Add validation for `AllowanceScope` values before deleting keys ___ Consider checking if `acl.AllowanceScope` is not only non-empty but also valid before proceeding to delete the raw key. This can prevent unintended deletions due to malformed `AllowanceScope` values. [gateway/auth_manager.go [91-93]](https://github.com/TykTechnologies/tyk/pull/6373/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR91-R93) ```diff -if acl.AllowanceScope == "" { +if !isValidAllowanceScope(acl.AllowanceScope) { continue } ``` Suggestion importance[1-10]: 8 Why: Adding validation for `AllowanceScope` values can prevent unintended deletions due to malformed values, enhancing the robustness of the code.	8
Performance	Implement batch deletion of keys to enhance performance ___ To improve the efficiency of the deletion process, consider using a batch delete approach if your storage backend supports it. This can significantly reduce the overhead of individual delete operations, especially when dealing with large datasets. [gateway/auth_manager.go [95]](https://github.com/TykTechnologies/tyk/pull/6373/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR95-R95) ```diff -store.DeleteRawKey(rawKey) +keysToDelete := gatherKeysToDelete(session.AccessRights, keyName) +store.BatchDeleteRawKeys(keysToDelete) ``` Suggestion importance[1-10]: 7 Why: Batch deletion can improve performance by reducing the overhead of individual delete operations, especially with large datasets. However, it depends on the storage backend's support for batch operations.	7
Maintainability	Refactor code to separate validation and deletion of keys ___ Refactor the loop that checks for `AllowanceScope` to filter and collect all valid scopes first, and then perform deletion. This separates the concerns of validation and deletion, making the code cleaner and potentially more efficient. [gateway/auth_manager.go [90-95]](https://github.com/TykTechnologies/tyk/pull/6373/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR90-R95) ```diff +validKeys := []string{} for _, acl := range session.AccessRights { - if acl.AllowanceScope == "" { - continue + if acl.AllowanceScope != "" { + validKeys = append(validKeys, QuotaKeyPrefix + acl.AllowanceScope + "-" + keyName) } - rawKey := QuotaKeyPrefix + acl.AllowanceScope + "-" + keyName - store.DeleteRawKey(rawKey) +} +for _, key := range validKeys { + store.DeleteRawKey(key) } ``` Suggestion importance[1-10]: 6 Why: Refactoring to separate validation and deletion can make the code cleaner and potentially more efficient. However, the improvement is more about code maintainability than a critical issue.	6