Merging to release-5.4: 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 to handle raw key deletion with allowance scopes

gateway/auth_manager.go

Added deleteRawKeysWithAllowanceScope method to handle raw key
deletion 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 deleteRawKeysWithAllowanceScope method

gateway/auth_manager_test.go

Added unit tests for deleteRawKeysWithAllowanceScope method.

Created countingStorageHandler for test purposes.

Covered various edge cases to ensure 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

API Changes

no api changes detected

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The new method `deleteRawKeysWithAllowanceScope` checks if `store` or `session` is `nil` and returns immediately without any error handling or logging. This could potentially hide issues where these parameters are unexpectedly `nil`.
⚡ Key issues to review	Code Clarity: The method `deleteRawKeysWithAllowanceScope` could benefit from some comments explaining the logic, especially why it's necessary to check for an empty `AllowanceScope` and skip deletion.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a nil check for `session.AccessRights` to prevent nil pointer dereference ___ To avoid potential nil pointer dereference, add a nil check for `session.AccessRights` before iterating over it in the `deleteRawKeysWithAllowanceScope` function. [gateway/auth_manager.go [90-93]](https://github.com/TykTechnologies/tyk/pull/6374/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR90-R93) ```diff +if session.AccessRights == nil { + return +} for _, acl := range session.AccessRights { if acl.AllowanceScope == "" { continue } ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a critical potential bug by preventing a nil pointer dereference, which could lead to runtime panics. It is essential for ensuring the stability of the function.	10
Possible bug	Add a check for an empty `keyName` to prevent issues with key operations ___ Consider checking for an empty `keyName` in the `deleteRawKeysWithAllowanceScope` function to prevent potential issues with key generation and deletion operations. This can be done by adding a simple check at the beginning of the function. [gateway/auth_manager.go [85-88]](https://github.com/TykTechnologies/tyk/pull/6374/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR85-R88) ```diff func (b DefaultSessionManager) deleteRawKeysWithAllowanceScope(store storage.Handler, session user.SessionState, keyName string) { - if store == nil \|\| session == nil { + if store == nil \|\| session == nil \|\| keyName == "" { return } ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by ensuring that an empty `keyName` does not cause issues with key generation and deletion operations. It is a simple yet effective check to improve code robustness.	9
Best practice	Add error handling for `DeleteRawKey` to manage deletion failures ___ It's recommended to handle potential errors from `store.DeleteRawKey(rawKey)` to ensure that failures in key deletion are properly managed and logged. [gateway/auth_manager.go [94-95]](https://github.com/TykTechnologies/tyk/pull/6374/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR94-R95) ```diff rawKey := QuotaKeyPrefix + acl.AllowanceScope + "-" + keyName -store.DeleteRawKey(rawKey) +if err := store.DeleteRawKey(rawKey); err != nil { + log.Errorf("Failed to delete raw key: %s, error: %v", rawKey, err) +} ``` Suggestion importance[1-10]: 8 Why: Handling potential errors from `DeleteRawKey` is a best practice that ensures failures are properly managed and logged, improving the reliability and maintainability of the code.	8
Enhancement	Modify the loop to break after processing the first non-empty `AllowanceScope` ___ To enhance the efficiency of the loop that checks `AllowanceScope`, consider breaking the loop once a non-empty `AllowanceScope` is found, if the intent is only to check for its existence rather than processing all items. [gateway/auth_manager.go [90-95]](https://github.com/TykTechnologies/tyk/pull/6374/files#diff-311e18b071d244ed1615c0019215b633278679df373288b3451bcc5cf7c52c4eR90-R95) ```diff for _, acl := range session.AccessRights { if acl.AllowanceScope == "" { continue } rawKey := QuotaKeyPrefix + acl.AllowanceScope + "-" + keyName store.DeleteRawKey(rawKey) + break # Assuming the intent is to find the first non-empty scope and process it } ``` Suggestion importance[1-10]: 4 Why: While this suggestion could enhance efficiency, it changes the logic by breaking the loop after the first non-empty `AllowanceScope` is found. This might not align with the intended functionality of processing all valid scopes.	4