Skip to content

Commit

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

### **User description**
[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 keys with no valid policies were not handled
correctly, ensuring an error is returned when no applicable policies
exist.
- Enhanced test cases across multiple files to validate policy
application, deletion, and behavior in various scenarios.
- Simplified policy synchronization logic in the gateway server.
- Added utility functions for policy deletion to improve test
management.
- Improved JWT session and multi-authentication tests to include API ID
and access rights in policies.



___



### **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 application and deletion.</code></dd></summary>
<hr>

gateway/api_test.go

<li>Added new test cases to validate policy application and deletion
<br>behavior.<br> <li> Enhanced existing test cases to include
additional policy scenarios.<br> <li> Introduced a new policy with ACL
and access rights for testing.<br>


</details>


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

</tr>

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

gateway/multiauth_test.go

<li>Updated test cases to include API ID and access rights in
policies.<br> <li> Improved test scenarios for multi-authentication with
JWT.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6781/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 policies in JWT-related tests.<br>
<li> Enhanced JWT session tests to validate policy behavior.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6781/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

<li>Added a utility function to delete policies during tests.<br> <li>
Improved test utilities for policy management.<br>


</details>


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

</tr>

<tr>
  <td>
    <details>
<summary><strong>apply_test.go</strong><dd><code>Improved tests for
custom policy application.</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

internal/policy/apply_test.go

<li>Updated tests to include ACL validation in policy application.<br>
<li> Enhanced test cases for custom policy application logic.<br>


</details>


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

</tr>
</table></td></tr><tr><td><strong>Enhancement</strong></td><td><table>
<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 removing redundant
checks.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6781/files#diff-4652d1bf175a0be8f5e61ef7177c9666f23e077d8626b73ac9d13358fa8b525b">+1/-3</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></td></tr><tr><td><strong>Bug fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>apply.go</strong><dd><code>Fixed error handling for
invalid policy application.</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
</dd></summary>
<hr>

internal/policy/apply.go

<li>Fixed bug where keys with no valid policies were not handled
<br>correctly.<br> <li> Added error handling for sessions with no
applicable policies.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6781/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1">+4/-0</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]>
  • Loading branch information
@buger @andrei-tyk
buger and andrei-tyk authored Dec 18, 2024
1 parent 7d56155 commit e8a3fbd
Show file tree
Hide file tree
Showing 7 changed files with 262 additions and 45 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 @@ -3900,6 +3911,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
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 e8a3fbd

Please sign in to comment.