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

[buger]

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

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

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

API Changes

no api changes detected

[github-actions]

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

[sonarcloud]

Quality Gate failed

Failed conditions
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