Skip to content

Commit

Permalink
Merging to release-5.3.9: [TT-12710]deleting All Partitioned Policies…
Browse files Browse the repository at this point in the history 
… a Key is linked to does not delete the Key (#6473) (#6777)

### **User description**
<details open>
<summary><a href="https://tyktech.atlassian.net/browse/TT-12710"
title="TT-12710" target="_blank">TT-12710</a></summary>
  <br />
  <table>
    <tr>
      <th>Summary</th>
<td>Deleting All Partitioned Policies a Key is linked to does not
prevent Key from being used </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 Test</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%20'24Bugsmash%20ORDER%20BY%20created%20DESC"
title="'24Bugsmash">'24Bugsmash</a></td>
    </tr>
  </table>
</details>
<!--
  do not remove this marker as it will break jira-lint's functionality.
  added_by_jira_lint
-->

---

[TT-12710]deleting All Partitioned Policies a Key is linked to does not
delete the Key (#6473)

### **User description**
TASK: https://tyktech.atlassian.net/browse/TT-12710
Fixed case in which trying to apply a non-existing policy error would be
swallowed when having partitioned keys.

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


___

### **PR Type**
Bug fix


___

### **Description**
- Fixed a bug where errors for non-existing policies were ignored if
multiple policies were processed, ensuring that an error is returned
immediately.
- Improved error handling in the `Apply` method of the `Service` to
prevent silent failures when policies are missing.


___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Bug
fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>apply.go</strong><dd><code>Fix error handling for
non-existing policies in Apply method</code></dd></summary>
<hr>

internal/policy/apply.go

<li>Removed logic that continued processing policies when a non-existing
<br>policy was encountered.<br> <li> Ensured that an error is returned
immediately if a policy is not <br>found.<br>


</details>


  </td>
<td><a

href="https://github.com/TykTechnologies/tyk/pull/6473/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1">+0/-4</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>                    
</table></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**:
>Comment `/help` on the PR to get a list of all available PR-Agent tools
and their descriptions

[TT-12710]:
https://tyktech.atlassian.net/browse/TT-12710?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ


___

### **PR Type**
Bug fix, Tests


___

### **Description**
- Fixed a bug where errors for non-existing policies were ignored,
ensuring proper error handling in the `ApplyPolicies` method.
- Enhanced test cases across multiple files to validate policy deletion
behavior and its impact on API access.
- Improved test coverage for JWT session handling, multi-auth scenarios,
and policy application.
- Simplified policy synchronization logic in the gateway server to
ensure consistency.
- Added a utility function for deleting policies in test scenarios.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Tests</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>api_test.go</strong><dd><code>Enhanced test coverage
for policy deletion and session updates.</code></dd></summary>
<hr>

gateway/api_test.go

<li>Added new test cases to validate policy deletion behavior and its
<br>impact on API access.<br> <li> Enhanced existing test cases to
include additional policies and <br>metadata for better coverage.<br>
<li> Adjusted assertions to ensure proper validation of access rights
and <br>applied policies.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6777/files#diff-10b4a3d7bdd8d98e48b288d27fd46d9ee436617806c46913fdf7942c0e4a992e">+102/-26</a></td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>multiauth_test.go</strong><dd><code>Updated multi-auth
tests with enhanced policy validation.</code></dd></summary>
<hr>

gateway/multiauth_test.go

<li>Updated test cases to include API ID and access rights in policy
<br>creation.<br> <li> Enhanced validation for multi-auth scenarios with
updated policies.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6777/files#diff-f7a567eba510b8bdffbc8e0878461e5dac9c066e8cc5feaa18eeccde54555f22">+7/-2</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>mw_jwt_test.go</strong><dd><code>Enhanced JWT session
tests with policy validation.</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; </dd></summary>
<hr>

gateway/mw_jwt_test.go

<li>Added API ID and access rights to JWT-related test cases.<br> <li>
Improved test coverage for JWT session handling and policy
<br>application.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6777/files#diff-406bf8fdb6c0cc77f04c6245c70abfc592ddb1525aa843200d850e14d135ebfc">+137/-12</a></td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>testutil.go</strong><dd><code>Added policy deletion
utility for tests.</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; </dd></summary>
<hr>

gateway/testutil.go

- Added a utility function to delete policies during tests.



</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6777/files#diff-7aaf6ae49fb8f58a8c99d337fedd15b3e430dd928ed547e425ef429b10d28ce8">+6/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></td></tr><tr><td><strong>Bug fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>middleware.go</strong><dd><code>Improved error handling
in ApplyPolicies method.</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

gateway/middleware.go

<li>Added error handling to return an error when no valid policies are
<br>applied to a session.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6777/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1b">+4/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>server.go</strong><dd><code>Simplified policy
synchronization logic.</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; </dd></summary>
<hr>

gateway/server.go

<li>Simplified policy synchronization logic by always updating
<br><code>policiesByID</code>.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6777/files#diff-4652d1bf175a0be8f5e61ef7177c9666f23e077d8626b73ac9d13358fa8b525b">+1/-3</a>&nbsp;
&nbsp; &nbsp; </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: andrei-tyk <[email protected]>
Co-authored-by: Andrei <[email protected]>
  • Loading branch information
@buger @andrei-tyk
3 people authored Dec 17, 2024
1 parent 014ce04 commit c70d30f
Show file tree
Hide file tree
Showing 7 changed files with 300 additions and 75 deletions.
128 changes: 102 additions & 26 deletions gateway/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -455,16 +455,24 @@ func TestKeyHandler_UpdateKey(t *testing.T) {
}
})

pIdAccess := ts.CreatePolicy(func(p *user.Policy) {
p.Partitions.Acl = true
p.AccessRights = map[string]user.AccessDefinition{testAPIID: {
APIID: testAPIID, Versions: []string{"v1"},
}}
p.Tags = []string{"p3-tag"}
p.MetaData = map[string]interface{}{
"p3-meta": "p3-value",
}
})

session, key := ts.CreateSession(func(s *user.SessionState) {
s.ApplyPolicies = []string{pID}
s.ApplyPolicies = []string{pIdAccess, pID}
s.Tags = []string{"key-tag1", "key-tag2"}
s.MetaData = map[string]interface{}{
"key-meta1": "key-value1",
"key-meta2": "key-value2",
}
s.AccessRights = map[string]user.AccessDefinition{testAPIID: {
APIID: testAPIID, Versions: []string{"v1"},
}}
})

t.Run("Add policy not enforcing acl", func(t *testing.T) {
Expand All @@ -477,8 +485,8 @@ func TestKeyHandler_UpdateKey(t *testing.T) {
}...)

sessionState, found := ts.Gw.GlobalSessionManager.SessionDetail("default", key, false)
if !found || sessionState.AccessRights[testAPIID].APIID != testAPIID || len(sessionState.ApplyPolicies) != 2 {

_, exists := sessionState.AccessRights[testAPIID]
if !found || !exists || len(sessionState.ApplyPolicies) != 3 {
t.Fatal("Adding policy to the list failed")
}
})
Expand All @@ -493,7 +501,8 @@ func TestKeyHandler_UpdateKey(t *testing.T) {
}...)

sessionState, found := ts.Gw.GlobalSessionManager.SessionDetail("default", key, false)
if !found || sessionState.AccessRights[testAPIID].APIID != testAPIID || len(sessionState.ApplyPolicies) != 0 {
_, exists := sessionState.AccessRights[testAPIID]
if !found || !exists || len(sessionState.ApplyPolicies) != 0 {
t.Fatal("Removing policy from the list failed")
}
})
Expand All @@ -518,21 +527,21 @@ func TestKeyHandler_UpdateKey(t *testing.T) {
}

t.Run("Add", func(t *testing.T) {
expected := []string{"p1-tag", "p2-tag", "key-tag1", "key-tag2"}
session.ApplyPolicies = []string{pID, pID2}
expected := []string{"p1-tag", "p2-tag", "p3-tag", "key-tag1", "key-tag2"}
session.ApplyPolicies = []string{pID, pID2, pIdAccess}
assertTags(session, expected)
})

t.Run("Make unique", func(t *testing.T) {
expected := []string{"p1-tag", "p2-tag", "key-tag1", "key-tag2"}
session.ApplyPolicies = []string{pID, pID2}
expected := []string{"p1-tag", "p2-tag", "p3-tag", "key-tag1", "key-tag2"}
session.ApplyPolicies = []string{pID, pID2, pIdAccess}
session.Tags = append(session.Tags, "p1-tag", "key-tag1")
assertTags(session, expected)
})

t.Run("Remove", func(t *testing.T) {
expected := []string{"p1-tag", "p2-tag", "key-tag2"}
session.ApplyPolicies = []string{pID, pID2}
expected := []string{"p1-tag", "p2-tag", "p3-tag", "key-tag2"}
session.ApplyPolicies = []string{pID, pID2, pIdAccess}
session.Tags = []string{"key-tag2"}
assertTags(session, expected)
})
Expand All @@ -559,31 +568,34 @@ func TestKeyHandler_UpdateKey(t *testing.T) {
expected := map[string]interface{}{
"p1-meta": "p1-value",
"p2-meta": "p2-value",
"p3-meta": "p3-value",
"key-meta1": "key-value1",
"key-meta2": "key-value2",
}
session.ApplyPolicies = []string{pID, pID2}
session.ApplyPolicies = []string{pID, pID2, pIdAccess}
assertMetaData(session, expected)
})

t.Run("Make unique", func(t *testing.T) {
expected := map[string]interface{}{
"p1-meta": "p1-value",
"p2-meta": "p2-value",
"p3-meta": "p3-value",
"key-meta1": "key-value1",
"key-meta2": "key-value2",
}
session.ApplyPolicies = []string{pID, pID2}
session.ApplyPolicies = []string{pID, pID2, pIdAccess}
assertMetaData(session, expected)
})

t.Run("Remove", func(t *testing.T) {
expected := map[string]interface{}{
"p1-meta": "p1-value",
"p2-meta": "p2-value",
"p3-meta": "p3-value",
"key-meta2": "key-value2",
}
session.ApplyPolicies = []string{pID, pID2}
session.ApplyPolicies = []string{pID, pID2, pIdAccess}
session.MetaData = map[string]interface{}{
"key-meta2": "key-value2",
}
Expand Down Expand Up @@ -701,13 +713,14 @@ func TestKeyHandler_DeleteKeyWithQuota(t *testing.T) {

pID := ts.CreatePolicy(func(p *user.Policy) {
p.QuotaMax = 1
p.AccessRights = map[string]user.AccessDefinition{testAPIID: {
APIID: testAPIID,
}}
})

_, key := ts.CreateSession(func(s *user.SessionState) {
s.ApplyPolicies = []string{pID}
s.AccessRights = map[string]user.AccessDefinition{testAPIID: {
APIID: testAPIID,
}}

})

authHeaders := map[string]string{
Expand Down Expand Up @@ -741,7 +754,11 @@ func TestUpdateKeyWithCert(t *testing.T) {
defer ts.Close()

apiId := "MTLSApi"
pID := ts.CreatePolicy(func(p *user.Policy) {})
pID := ts.CreatePolicy(func(p *user.Policy) {
p.AccessRights = map[string]user.AccessDefinition{apiId: {
APIID: apiId, Versions: []string{"v1"},
}}
})

ts.Gw.BuildAndLoadAPI(func(spec *APISpec) {
spec.APIID = apiId
Expand All @@ -768,9 +785,6 @@ func TestUpdateKeyWithCert(t *testing.T) {
// create session base and set cert
session, key := ts.CreateSession(func(s *user.SessionState) {
s.ApplyPolicies = []string{pID}
s.AccessRights = map[string]user.AccessDefinition{apiId: {
APIID: apiId, Versions: []string{"v1"},
}}
s.Certificate = certID
})

Expand Down Expand Up @@ -813,9 +827,6 @@ func TestUpdateKeyWithCert(t *testing.T) {
// create session base and set cert
session, key := ts.CreateSession(func(s *user.SessionState) {
s.ApplyPolicies = []string{pID}
s.AccessRights = map[string]user.AccessDefinition{apiId: {
APIID: apiId, Versions: []string{"v1"},
}}
s.Certificate = certID
})

Expand Down Expand Up @@ -3902,6 +3913,71 @@ func TestOrgKeyHandler_LastUpdated(t *testing.T) {
}...)
}

func TestDeletionOfPoliciesThatFromAKeyDoesNotMakeTheAPIKeyless(t *testing.T) {
const testAPIID = "testAPIID"

ts := StartTest(nil)
defer ts.Close()

apiID1 := testAPIID + "1"
apiID2 := testAPIID + "2"

ts.Gw.BuildAndLoadAPI(func(spec *APISpec) {
spec.APIID = apiID1
spec.UseKeylessAccess = false
spec.OrgID = "default"
spec.Proxy.ListenPath = "/api1"
}, func(spec *APISpec) {
spec.APIID = apiID2
spec.UseKeylessAccess = false
spec.OrgID = "default"
spec.Proxy.ListenPath = "/api2"
})

policyForApi1 := ts.CreatePolicy(func(p *user.Policy) {
p.AccessRights = map[string]user.AccessDefinition{apiID1: {
APIID: apiID1,
}}
})

policyForApi2 := ts.CreatePolicy(func(p *user.Policy) {
p.AccessRights = map[string]user.AccessDefinition{apiID2: {
APIID: apiID2,
}}
})

_, key := ts.CreateSession(func(s *user.SessionState) {
s.ApplyPolicies = []string{policyForApi1, policyForApi2}
})

authHeaders := map[string]string{
"authorization": key,
}

res, err := ts.Run(t, []test.TestCase{
{Method: "GET", Path: "/api1", Headers: authHeaders, Code: 200},
{Method: "GET", Path: "/api2", Headers: authHeaders, Code: 200},
}...)
assert.NotNil(t, res)
assert.Nil(t, err)

ts.DeletePolicy(policyForApi2)
res, err = ts.Run(t, []test.TestCase{
{Method: "GET", Path: "/api1", Headers: authHeaders, Code: 200},
{Method: "GET", Path: "/api2", Headers: authHeaders, Code: 403},
}...)
assert.NotNil(t, res)
assert.Nil(t, err)

ts.DeletePolicy(policyForApi1)
res, err = ts.Run(t, []test.TestCase{
{Method: "GET", Path: "/api1", Headers: authHeaders, Code: 403},
{Method: "GET", Path: "/api2", Headers: authHeaders, Code: 403},
}...)
assert.NotNil(t, res)
assert.Nil(t, err)
}

func TestPurgeOAuthClientTokensEndpoint(t *testing.T) {
conf := func(globalConf *config.Config) {
// set tokens to be expired after 1 second
Expand Down
4 changes: 4 additions & 0 deletions gateway/middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,10 @@ func (t *BaseMiddleware) ApplyPolicies(session *user.SessionState) error {
session.AccessRights = rights
}

if len(rights) == 0 && policyIDs != nil {
return errors.New("key has no valid policies to be applied")
}

return nil
}

Expand Down
9 changes: 7 additions & 2 deletions gateway/multiauth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,11 +304,16 @@ func TestJWTAuthKeyMultiAuth(t *testing.T) {
ts := StartTest(nil)
defer ts.Close()

pID := ts.CreatePolicy()
const testAPIID = "test-api-id"
pID := ts.CreatePolicy(func(p *user.Policy) {
p.AccessRights = map[string]user.AccessDefinition{
testAPIID: {APIID: testAPIID, APIName: "test-api"},
}
})

spec := ts.Gw.BuildAndLoadAPI(func(spec *APISpec) {
spec.UseKeylessAccess = false

spec.APIID = testAPIID
spec.AuthConfigs = make(map[string]apidef.AuthConfig)

spec.UseStandardAuth = true
Expand Down
Loading

0 comments on commit c70d30f

Please sign in to comment.