Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[TT-10109] Fix policy lookup map distortion #5730

Merged
merged 2 commits into from
Nov 6, 2023
Merged

[TT-10109] Fix policy lookup map distortion #5730

merged 2 commits into from
Nov 6, 2023

Conversation

furkansenharputlu
Copy link
Contributor

@furkansenharputlu furkansenharputlu commented Nov 5, 2023 •
edited
Loading

https://tyktech.atlassian.net/browse/TT-10109

The policy lookup map is distorted by ApplyPolicies function so it ends up wrong path base permission values in the session object.

See how it gets the original policy object and changes values inside it:

tyk/gateway/middleware.go

Line 493 in 6c6b153

ar := v

@furkansenharputlu furkansenharputlu self-assigned this Nov 5, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2023

API Changes

no api changes detected

@sweep-ai Sweep AI
Copy link

sweep-ai bot commented Nov 5, 2023

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2023

PR Analysis

  • 🎯 Main theme: Fixing a distortion in the policy lookup map in the ApplyPolicies function
  • 📝 PR summary: This PR addresses an issue where the policy lookup map was being distorted by the ApplyPolicies function, resulting in incorrect path base permission values in the session object. The fix involves making a deep copy of the policiesByID map to prevent the original map from being distorted. The PR also includes changes to the test cases to reflect the changes made in the code.
  • 📌 Type of PR: Bug fix
  • 🧪 Relevant tests added: Yes
  • ⏱️ Estimated effort to review [1-5]: 2, The PR is relatively small and the changes are straightforward. The main change involves making a deep copy of a map to prevent it from being distorted, which is a common and well-understood operation.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and the changes are clear. The use of deep copying to prevent the distortion of the original map is a good solution. However, error handling for the json Marshal and Unmarshal operations could be improved.

  • 🤖 Code feedback:

    • relevant file: gateway/middleware.go
      suggestion: Add error handling for the json.Marshal and json.Unmarshal operations. Ignoring errors from these operations could lead to unexpected behavior if the marshaling or unmarshaling fails. [important]
      relevant line: byt, _ := json.Marshal(t.Gw.policiesByID)

    • relevant file: gateway/policy_test.go
      suggestion: Consider adding more test cases to cover different scenarios and edge cases. This will help ensure that the function behaves correctly under all circumstances. [medium]
      relevant line: sessMatch: func(t *testing.T, sess *user.SessionState) {

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

@buger
Copy link
Member

buger commented Nov 5, 2023

API tests result: success ✅
Branch used: refs/pull/5730/merge
Commit:
Triggered by: pull_request (@furkansenharputlu)
Execution page

@furkansenharputlu furkansenharputlu force-pushed the fix/jwt-path-based-permission branch from 6c6b153 to 9635eb6 Compare November 5, 2023 21:22
@TykTechnologies TykTechnologies deleted a comment from sonarqubecloud bot Nov 5, 2023
@furkansenharputlu furkansenharputlu force-pushed the fix/jwt-path-based-permission branch from 9635eb6 to 9e32ecc Compare November 5, 2023 21:27
@furkansenharputlu furkansenharputlu force-pushed the fix/jwt-path-based-permission branch from 9e32ecc to 577f2b8 Compare November 5, 2023 21:29
@buger
Copy link
Member

buger commented Nov 5, 2023

API tests result: success ✅
Branch used: refs/pull/5730/merge
Commit: 9635eb6
Triggered by: pull_request (@furkansenharputlu)
Execution page

@buger
Copy link
Member

buger commented Nov 5, 2023

API tests result: success ✅
Branch used: refs/pull/5730/merge
Commit: 9e32ecc
Triggered by: pull_request (@furkansenharputlu)
Execution page

@buger
Copy link
Member

buger commented Nov 5, 2023

API tests result: success ✅
Branch used: refs/pull/5730/merge
Commit: 577f2b8
Triggered by: pull_request (@furkansenharputlu)
Execution page

@TykTechnologies TykTechnologies deleted a comment from sonarqubecloud bot Nov 5, 2023
buger
buger reviewed Nov 6, 2023
View reviewed changes
gateway/middleware.go Outdated
@@ -420,6 +421,11 @@ func (t BaseMiddleware) ApplyPolicies(session *user.SessionState) error {

return err
}

policyInBytes, _ := json.Marshal(originalPolicy)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be very slow, we will need to find another solution

@buger
Copy link
Member

buger commented Nov 6, 2023

API tests result: success ✅
Branch used: refs/pull/5730/merge
Commit: d71f887
Triggered by: pull_request (@furkansenharputlu)
Execution page

@furkansenharputlu furkansenharputlu force-pushed the fix/jwt-path-based-permission branch from d71f887 to 50b6d3a Compare November 6, 2023 15:06
@furkansenharputlu furkansenharputlu force-pushed the fix/jwt-path-based-permission branch from 50b6d3a to 0d9857d Compare November 6, 2023 15:08
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot E 5 Security Hotspots
Code Smell A 22 Code Smells

78.0% 78.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@buger
Copy link
Member

buger commented Nov 6, 2023

API tests result: success ✅
Branch used: refs/pull/5730/merge
Commit: 0d9857d
Triggered by: pull_request (@furkansenharputlu)
Execution page

@buger
Copy link
Member

buger commented Nov 6, 2023

API tests result: success ✅
Branch used: refs/pull/5730/merge
Commit: 50b6d3a
Triggered by: pull_request (@furkansenharputlu)
Execution page

@furkansenharputlu furkansenharputlu merged commit d18ea51 into master Nov 6, 2023
@furkansenharputlu furkansenharputlu deleted the fix/jwt-path-based-permission branch November 6, 2023 17:45
@furkansenharputlu
Copy link
Contributor Author

/release to release-5.2

@tykbot tykbot
Copy link

tykbot bot commented Nov 6, 2023

Working on it! Note that it can take a few minutes.

@furkansenharputlu
Copy link
Contributor Author

/release to release-5-lts

tykbot bot pushed a commit that referenced this pull request Nov 6, 2023
@furkansenharputlu
[TT-10109] Fix policy lookup map distortion (#5730)
2fcbc5f 
https://tyktech.atlassian.net/browse/TT-10109

The policy lookup map is distorted by `ApplyPolicies` function so it
ends up wrong path base permission values in the session object.

See how it gets the original policy object and changes values inside it:
https://github.com/TykTechnologies/tyk/blob/6c6b1535921543d2e2f34d65bbba7d67baffb547/gateway/middleware.go#L493

(cherry picked from commit d18ea51)
@buger buger mentioned this pull request Nov 6, 2023
Merged
@tykbot tykbot
Copy link

tykbot bot commented Nov 6, 2023

@furkansenharputlu Succesfully merged PR

@tykbot tykbot
Copy link

tykbot bot commented Nov 6, 2023

Working on it! Note that it can take a few minutes.

tykbot bot pushed a commit that referenced this pull request Nov 6, 2023
@furkansenharputlu
[TT-10109] Fix policy lookup map distortion (#5730)
d70057c 
https://tyktech.atlassian.net/browse/TT-10109

The policy lookup map is distorted by `ApplyPolicies` function so it
ends up wrong path base permission values in the session object.

See how it gets the original policy object and changes values inside it:
https://github.com/TykTechnologies/tyk/blob/6c6b1535921543d2e2f34d65bbba7d67baffb547/gateway/middleware.go#L493

(cherry picked from commit d18ea51)
@buger buger mentioned this pull request Nov 6, 2023
Merged
@tykbot tykbot
Copy link

tykbot bot commented Nov 6, 2023

@furkansenharputlu Succesfully merged PR

buger added a commit that referenced this pull request Nov 6, 2023
@buger
Merging to release-5.2: [TT-10109] Fix policy lookup map distortion (#…
c6dc825 
…5730)

[TT-10109] Fix policy lookup map distortion (#5730)

https://tyktech.atlassian.net/browse/TT-10109

The policy lookup map is distorted by `ApplyPolicies` function so it
ends up wrong path base permission values in the session object.

See how it gets the original policy object and changes values inside it:
https://github.com/TykTechnologies/tyk/blob/6c6b1535921543d2e2f34d65bbba7d67baffb547/gateway/middleware.go#L493
buger added a commit that referenced this pull request Nov 6, 2023
@buger
Merging to release-5-lts: [TT-10109] Fix policy lookup map distortion (…
1cf05fb 
…#5730)

[TT-10109] Fix policy lookup map distortion (#5730)

https://tyktech.atlassian.net/browse/TT-10109

The policy lookup map is distorted by `ApplyPolicies` function so it
ends up wrong path base permission values in the session object.

See how it gets the original policy object and changes values inside it:
https://github.com/TykTechnologies/tyk/blob/6c6b1535921543d2e2f34d65bbba7d67baffb547/gateway/middleware.go#L493
furkansenharputlu added a commit that referenced this pull request Nov 8, 2023
@furkansenharputlu
Revert "Merging to release-5-lts: [TT-10109] Fix policy lookup map di…
6a55365 
…stortion (#5730)"

This reverts commit 1cf05fb.
@furkansenharputlu furkansenharputlu mentioned this pull request Nov 8, 2023
Merged
7 tasks
furkansenharputlu added a commit that referenced this pull request Nov 8, 2023
@furkansenharputlu
Revert "Merging to release-5.2: [TT-10109] Fix policy lookup map dist…
69055b3 
…ortion (#5730)"

This reverts commit c6dc825.
@furkansenharputlu furkansenharputlu mentioned this pull request Nov 8, 2023
Merged
7 tasks
furkansenharputlu added a commit that referenced this pull request Nov 8, 2023
@furkansenharputlu
Revert "Merging to release-5-lts: [TT-10109] Fix policy lookup map di… (
a50afbc 
#5754)

…stortion (#5730)"

This reverts commit 1cf05fb.

<!-- Provide a general summary of your changes in the Title above -->

## Description

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

## Motivation and Context

<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

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

## Checklist

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the checklist. -->

- [ ] I ensured that the documentation is up to date
- [ ] I explained why this PR updates go.mod in detail with reasoning
why it's required
- [ ] I would like a code coverage CI quality gate exception and have
explained why
furkansenharputlu added a commit that referenced this pull request Nov 8, 2023
@furkansenharputlu
Revert "Merging to release-5.2: [TT-10109] Fix policy lookup map dist… (
9416376 
#5755)

…ortion (#5730)"

This reverts commit c6dc825.

<!-- Provide a general summary of your changes in the Title above -->

## Description

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

## Motivation and Context

<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

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

## Checklist

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the checklist. -->

- [ ] I ensured that the documentation is up to date
- [ ] I explained why this PR updates go.mod in detail with reasoning
why it's required
- [ ] I would like a code coverage CI quality gate exception and have
explained why
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buger buger buger approved these changes

Assignees

@furkansenharputlu furkansenharputlu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@furkansenharputlu @buger