Skip to content

Commit

Permalink
Fix token update generate new hash entry (TykTechnologies#3229)
Browse files Browse the repository at this point in the history
<!-- Provide a general summary of your changes in the Title above -->

## Description
On update keys, we should not call twice `doAddOrUpdate` as this create duplicates for the same key in redis, instead, we should check if we are dealing with a custom key or not, and therefore perform the proper action.

## Related Issue
TykTechnologies#3109

## Motivation and Context
Give solution to TykTechnologies#3109

## How This Has Been Tested
- Run GW and Dashboard with `"hash_keys": true`
- Create a custom key via gw api
- Update custom key...and check that the value was updated and an additional registry was not created in redis
- Create key via dashboard
- Update key via dashboard, check that the value was updated but an additional registry in redis was not created for the same key
- Do the same but now with `"hash_keys": false`

## Screenshots (if appropriate)

## Types of changes
<!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] 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 you're unsure about any of these, don't hesitate to ask; we're here to help! -->
- [x] Make sure you are requesting to **pull a topic/feature/bugfix branch** (right side). If pulling from your own
      fork, don't request your `master`!
- [x] Make sure you are making a pull request against the **`master` branch** (left side). Also, you should start
      *your branch* off *our latest `master`*.
- [ ] My change requires a change to the documentation.
  - [ ] If you've changed APIs, describe what needs to be updated in the documentation.
  - [ ] If new config option added, ensure that it can be set via ENV variable
- [ ] I have updated the documentation accordingly.
- [ ] Modules and vendor dependencies have been updated; run `go mod tidy && go mod vendor`
- [ ] When updating library version must provide reason/explanation for this update.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
- [x] Check your code additions will not fail linting checks:
  - [x] `go fmt -s`
  - [x] `go vet`
  • Loading branch information
sredxny committed Jul 25, 2020
1 parent a10b1d2 commit 110cdf7
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 5 deletions.
15 changes: 10 additions & 5 deletions gateway/api.go
Expand Up @@ -411,12 +411,17 @@ func handleAddOrUpdate(keyName string, r *http.Request, isHashed bool) (interfac
return apiError("Failed to create key, ensure security settings are correct."), http.StatusInternalServerError
}
} else {
// update legacy key format one
if err := doAddOrUpdate(keyName, &newSession, suppressReset, isHashed); err != nil {
return apiError("Failed to create key, ensure security settings are correct."), http.StatusInternalServerError

newFormatKey := generateToken(newSession.OrgID, keyName)
// search as a custom key
_, err := GlobalSessionManager.Store().GetKey(newFormatKey)

if err == nil {
// update new format key for custom keys, as it was found then its a customKey
keyName = newFormatKey
}
// update new format key
if err := doAddOrUpdate(generateToken(newSession.OrgID, keyName), &newSession, suppressReset, isHashed); err != nil {

if err := doAddOrUpdate(keyName, &newSession, suppressReset, isHashed); err != nil {
return apiError("Failed to create key, ensure security settings are correct."), http.StatusInternalServerError
}
}
Expand Down
55 changes: 55 additions & 0 deletions gateway/api_test.go
Expand Up @@ -486,6 +486,61 @@ func TestKeyHandler_UpdateKey(t *testing.T) {
})
}

func TestKeyHandler_CheckKeysNotDuplicateOnUpdate(t *testing.T) {
ts := StartTest()
defer ts.Close()

BuildAndLoadAPI(func(spec *APISpec) {
spec.UseKeylessAccess = false
spec.Auth.UseParam = true
})

cases := []struct {
Name string
IsCustomKey bool
KeyName string
}{
{
Name: "duplicity on update custom key",
IsCustomKey: true,
KeyName: "my_custom_key",
},
{
Name: "duplicity on update regular key",
IsCustomKey: false,
},
}

for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {
GlobalSessionManager.Store().DeleteAllKeys()
session := CreateStandardSession()
session.AccessRights = map[string]user.AccessDefinition{"test": {
APIID: "test", Versions: []string{"v1"},
}}

keyName := tc.KeyName
if !tc.IsCustomKey {
keyName = generateToken(session.OrgID, "")
}

if err := doAddOrUpdate(keyName, session, false, true); err != nil {
t.Error("Failed to create key, ensure security settings are correct:" + err.Error())
}

requestByte, _ := json.Marshal(session)
r := httptest.NewRequest(http.MethodPut, "/tyk/keys/"+keyName, bytes.NewReader(requestByte))
handleAddOrUpdate(keyName, r, true)

sessions := GlobalSessionManager.Sessions("")
if len(sessions) != 1 {
t.Errorf("Sessions stored in global manager should be 1. But got: %v", len(sessions))
}

})
}
}

func TestHashKeyHandler(t *testing.T) {
globalConf := config.Global()
// make it to use hashes for Redis keys
Expand Down

0 comments on commit 110cdf7

Please sign in to comment.