[TT-12897/TT-13284] Add additional partitioned test case, fix ordering issue

titpetric commented 1 month ago

User description

Summary	[Security]Path-Based Permissions permissions in policies are not preserved when policies are combined
Type	Bug
Status	In Dev
Points	N/A
Labels	customer_bug, jira_escalated, QA_Fail

Subtask: https://tyktech.atlassian.net/browse/TT-13284 Parent: https://tyktech.atlassian.net/browse/TT-12897

PR Type

Bug fix, Tests

Description

Fixed a bug in applyPartitions function to ensure rights map is filled with known APIs, ensuring policies with ACL rights are honored even if not first.
Improved merging logic for RestrictedTypes, AllowedTypes, and FieldAccessRights to handle empty cases and intersections correctly.
Added test cases to verify the correct application of ACL and rate limits from custom policies, ensuring the order of policies does not affect the outcome.

Changes walkthrough 📝

Relevant files

Bug fix

apply.go

Fix policy merging and ordering issues in partitioned policies

internal/policy/apply.go

Ensure rights map is filled with known APIs to honor policies.

Modify merging logic for RestrictedTypes, AllowedTypes, and
FieldAccessRights.

Fix ordering issue in policy application by using previously seen
rights.

+41/-21

Tests

apply_test.go

Add test cases for ACL and rate limit application

internal/policy/apply_test.go

Add test cases for applying ACL from custom policies.

Verify correct application of rate limits and access rights.

+47/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

buger commented 1 month ago

Let's make that PR title a 💯 shall we? 💪

<p>
Your <em>PR title</em> and <em>story title</em> look <strong>slightly different</strong>. Just checking in to know if it was intentional!
</p>
<table>
  <tr>
    <th>Story Title</th>
    <td>[Security]Path-Based Permissions permissions in policies are not preserved when policies are combined</td>
  </tr>
  <tr>
      <th>PR Title</th>
      <td>[TT-12897/TT-13284] Add additional partitioned test case, fix ordering issue</td>
    </tr>
</table>
<p>
  Check out this <a href="https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests">guide</a> to learn more about PR best-practices.
</p>

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Code Redundancy
The logic for merging field access rights, restricted types, and allowed types is repetitive and could be refactored into a more generic function to improve maintainability and reduce code duplication. Error Handling
The new code does not handle potential errors from the `intersection` function used in merging types fields. This could lead to runtime panics if the function fails.

github-actions[bot] commented 1 month ago

API Changes

no api changes detected

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Best practice	Add validation for `policy.QuotaMax` to ensure it adheres to constraints ___ Validate the `policy.QuotaMax` value before using it to ensure it meets expected constraints, such as being non-negative and not exceeding certain predefined limits. [internal/policy/apply.go [439-443]](https://github.com/TykTechnologies/tyk/pull/6635/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R439-R443) ```diff -if greaterThanInt64(policy.QuotaMax, ar.Limit.QuotaMax) { - ar.Limit.QuotaMax = policy.QuotaMax - if greaterThanInt64(policy.QuotaMax, session.QuotaMax) { - session.QuotaMax = policy.QuotaMax +if policy.QuotaMax >= 0 && policy.QuotaMax <= maxQuotaLimit { + if greaterThanInt64(policy.QuotaMax, ar.Limit.QuotaMax) { + ar.Limit.QuotaMax = policy.QuotaMax + if greaterThanInt64(policy.QuotaMax, session.QuotaMax) { + session.QuotaMax = policy.QuotaMax + } } +} else { + return fmt.Errorf("invalid QuotaMax value: %d", policy.QuotaMax) } ``` Suggestion importance[1-10]: 8 Why: This suggestion adds validation for `policy.QuotaMax`, ensuring it meets expected constraints. This is a critical enhancement for preventing potential runtime errors and ensuring data integrity.	8
Possible issue	Avoid potential concurrent map write issues by using a temporary map for updates ___ Ensure that the `rights` map is not modified directly within the loop to avoid potential concurrent map write issues. Consider using a temporary map to store updates and merge them back to `rights` after the loop. [internal/policy/apply.go [358-362]](https://github.com/TykTechnologies/tyk/pull/6635/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R358-R362) ```diff +tempRights := make(map[string]user.AccessDefinition) for k := range policy.AccessRights { if _, ok := rights[k]; ok { continue } - rights[k] = user.AccessDefinition{} + tempRights[k] = user.AccessDefinition{} +} +for k, v := range tempRights { + rights[k] = v } ``` Suggestion importance[1-10]: 7 Why: This suggestion addresses potential concurrent map write issues by using a temporary map for updates, which is a valid concern in concurrent programming. It enhances the robustness of the code, making it safer for concurrent execution.	7
Enhancement	Improve the efficiency and readability of merging type fields ___ Refactor the nested loops that merge `RestrictedTypes` and `AllowedTypes` to improve efficiency and readability. Consider using a map to track types and reduce the complexity of the operation. [internal/policy/apply.go [385-389]](https://github.com/TykTechnologies/tyk/pull/6635/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R385-R389) ```diff +restrictedTypeMap := make(map[string]int) +for i, rt := range r.RestrictedTypes { + restrictedTypeMap[rt.Name] = i +} for _, t := range v.RestrictedTypes { - for ri, rt := range r.RestrictedTypes { - if t.Name == rt.Name { - r.RestrictedTypes[ri].Fields = intersection(rt.Fields, t.Fields) - } + if i, exists := restrictedTypeMap[t.Name]; exists { + r.RestrictedTypes[i].Fields = intersection(r.RestrictedTypes[i].Fields, t.Fields) } } ``` Suggestion importance[1-10]: 6 Why: The suggestion improves the efficiency and readability of the code by using a map to track types, reducing the complexity of nested loops. This is a beneficial enhancement for maintainability and performance.	6