Merging to release-5.3: [TT-12897] Merge path based permissions when combining policies (#6597) #6625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<details open> <summary><a href="https://tyktech.atlassian.net/browse/TT-12897" title="TT-12897" target="_blank">TT-12897</a></summary> <br /> <table> <tr> <th>Summary</th> <td>[Security]Path-Based Permissions permissions in policies are not preserved when policies are combined</td> </tr> <tr> <th>Type</th> <td> <img alt="Bug" src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10303?size=medium" /> Bug </td> </tr> <tr> <th>Status</th> <td>In Dev</td> </tr> <tr> <th>Points</th> <td>N/A</td> </tr> <tr> <th>Labels</th> <td><a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20customer_bug%20ORDER%20BY%20created%20DESC" title="customer_bug">customer_bug</a>, <a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20jira_escalated%20ORDER%20BY%20created%20DESC" title="jira_escalated">jira_escalated</a></td> </tr> </table> </details> <!-- do not remove this marker as it will break jira-lint's functionality. added_by_jira_lint --> --- PR uses custom policies to combine several policies with access rights set. Since a `map` was in the path, user API for custom policies needed an extension to preserve policy ID order. The existing function returning a map didn't handle json decode errors properly and go semantics when looping over maps don't preserve this order, but it's random so tests would fail. Verified with `task stress`. Issue: https://tyktech.atlassian.net/browse/TT-12897 ___ Bug fix, Enhancement, Tests ___ - Enhanced policy application logic by introducing `MergeAllowedURLs` to merge allowed URLs efficiently. - Refactored `Store` to use a slice for policies, and introduced `StoreMap` for unordered policy storage. - Improved custom policy handling by adding `GetCustomPolicies` to preserve policy order. - Updated tests to ensure proper application of policies and added new tests for `MergeAllowedURLs`. - Updated Taskfile to include a new `stress` task for running stress tests. ___ <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>apply.go</strong><dd><code>Enhance policy application logic and logging</code> </dd></summary> <hr> internal/policy/apply.go <li>Introduced <code>MergeAllowedURLs</code> function to merge allowed URLs.<br> <li> Updated <code>Logger</code> function to return a <code>logrus.Entry</code>.<br> <li> Changed <code>session.CustomPolicies()</code> to <code>session.GetCustomPolicies()</code>. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1">+9/-17</a> </td> </tr> <tr> <td> <details> <summary><strong>store.go</strong><dd><code>Refactor Store to use slice for policies</code> </dd></summary> <hr> internal/policy/store.go <li>Changed <code>Store</code> to use a slice for policies.<br> <li> Updated methods to accommodate slice-based storage. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-13dec7bc453c9ff99550c83d2f86a017bbf7fb863584dc30603af15d29ef9d3d">+20/-7</a> </td> </tr> <tr> <td> <details> <summary><strong>store_map.go</strong><dd><code>Add StoreMap for unordered policy storage</code> </dd></summary> <hr> internal/policy/store_map.go <li>Introduced <code>StoreMap</code> for unordered policy storage.<br> <li> Implemented methods for <code>StoreMap</code>. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-24a7a95a1cf4f14b59a3475127dc45541357638d6949323255faeeb2ed657d27">+46/-0</a> </td> </tr> <tr> <td> <details> <summary><strong>util.go</strong><dd><code>Introduce MergeAllowedURLs and remove unused functions</code> </dd></summary> <hr> internal/policy/util.go <li>Added <code>MergeAllowedURLs</code> function for merging URL access specs.<br> <li> Removed <code>copyAllowedURLs</code> and <code>contains</code> functions. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-0323c3da13f08a9ccd340ac04208d680856354fd566dffcad925fa6645639955">+46/-70</a> </td> </tr> <tr> <td> <details> <summary><strong>custom_policies.go</strong><dd><code>Enhance custom policies handling with order preservation</code> </dd></summary> <hr> user/custom_policies.go <li>Added <code>GetCustomPolicies</code> to preserve policy order.<br> <li> Updated <code>CustomPolicies</code> to use <code>GetCustomPolicies</code>. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-911674993eef6c43a04edc0e90ea1f2e6d595792eef840d23b2e3deb1c8265c5">+21/-7</a> </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>apply_test.go</strong><dd><code>Update tests for policy application</code> </dd></summary> <hr> internal/policy/apply_test.go <li>Added initialization of <code>policy.Service</code> in tests.<br> <li> Ensured <code>Apply</code> method is tested with <code>assert.NoError</code>. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8">+29/-28</a> </td> </tr> <tr> <td> <details> <summary><strong>util_test.go</strong><dd><code>Add tests for MergeAllowedURLs function</code> </dd></summary> <hr> internal/policy/util_test.go - Added tests for `MergeAllowedURLs` function. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-c750a1b8a01d19dacf02ba7512b8e2b987bf8147cf3345a4374504d9d5b3840e">+64/-0</a> </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>Taskfile.yml</strong><dd><code>Update Taskfile with stress test task</code> </dd></summary> <hr> internal/policy/Taskfile.yml <li>Added <code>stress</code> task for running stress tests.<br> <li> Updated <code>default</code> task to include <code>test</code>. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6597/files#diff-e0f19d4dd27acb397e19ccb080f3142a09f5978699da5843bfc71e7ffa4bb775">+16/-0</a> </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull request to receive relevant information --------- Co-authored-by: Tit Petric <tit@tyk.io> (cherry picked from commit e31a08f)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TT-12897 Merge path based permissions when combining policies (#6597)
User description
TT-12897
PR uses custom policies to combine several policies with access rights
set.
Since a
mapwas in the path, user API for custom policies needed anextension to preserve policy ID order. The existing function returning a
map didn't handle json decode errors properly and go semantics when
looping over maps don't preserve this order, but it's random so tests
would fail. Verified with
task stress.Issue: https://tyktech.atlassian.net/browse/TT-12897
PR Type
Bug fix, Enhancement, Tests
Description
MergeAllowedURLstomerge allowed URLs efficiently.
Storeto use a slice for policies, and introducedStoreMapfor unordered policy storage.GetCustomPoliciestopreserve policy order.
tests for
MergeAllowedURLs.stresstask for running stresstests.
Changes walkthrough 📝
apply.go
Enhance policy application logic and logginginternal/policy/apply.go
MergeAllowedURLsfunction to merge allowed URLs.Loggerfunction to return alogrus.Entry.session.CustomPolicies()tosession.GetCustomPolicies().store.go
Refactor Store to use slice for policiesinternal/policy/store.go
Storeto use a slice for policies.store_map.go
Add StoreMap for unordered policy storageinternal/policy/store_map.go
StoreMapfor unordered policy storage.StoreMap.util.go
Introduce MergeAllowedURLs and remove unused functionsinternal/policy/util.go
MergeAllowedURLsfunction for merging URL access specs.copyAllowedURLsandcontainsfunctions.custom_policies.go
Enhance custom policies handling with order preservationuser/custom_policies.go
GetCustomPoliciesto preserve policy order.CustomPoliciesto useGetCustomPolicies.apply_test.go
Update tests for policy applicationinternal/policy/apply_test.go
policy.Servicein tests.Applymethod is tested withassert.NoError.util_test.go
Add tests for MergeAllowedURLs functioninternal/policy/util_test.go
MergeAllowedURLsfunction.Taskfile.yml
Update Taskfile with stress test taskinternal/policy/Taskfile.yml
stresstask for running stress tests.defaulttask to includetest.Co-authored-by: Tit Petric tit@tyk.io