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.

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

fix set remove (part 2) #414

Closed
jozadev24 opened this issue Jul 15, 2021 · 5 comments · Fixed by #415
Closed

fix set remove (part 2) #414

jozadev24 opened this issue Jul 15, 2021 · 5 comments · Fixed by #415

Comments

@jozadev24
Copy link

Me again ... :-)

So when the key to be removed is the last one in the set, the last 2 keys are removed.

This line

jwx/jwk/set.go

Line 72 in 5e2de6e

s.keys = s.keys[:i-1]
should be like this:
s.keys = s.keys[:i]

I propose to replace the new test func

func TestGH412(t *testing.T) {

with something like this, which tests removal of first, last and center keys (which will fail with unfixed version):

func TestSetRemove(t *testing.T) {
	checkKeyRemoved := func(set jwk.Set, k jwk.Key) bool {
		ctx := context.Background()
		for iter := set.Iterate(ctx); iter.Next(ctx); {
			pair := iter.Pair()
			key := pair.Value.(jwk.Key)
			if !assert.NotEqual(t, k.KeyID(), key.KeyID(), `key id should not match`) {
				return false
			}
		}
		return true
	}

	set := jwk.NewSet()
	const max = 5
	for i := 0; i < max; i++ {
		k, err := jwxtest.GenerateRsaJwk()
		if !assert.NoError(t, err, `jwxttest.GenerateRsaJwk() should succeed`) {
			return
		}

		k.Set(jwk.KeyIDKey, strconv.Itoa(i))
		set.Add(k)
	}

	if !assert.Equal(t, max, set.Len(), `set.Len should be %d`, max) {
		return
	}

	first, ok := set.Get(0)
	if !assert.True(t, ok, `set.Get should succeed`) {
		return
	}

	last, ok := set.Get(max - 1)
	if !assert.True(t, ok, `set.Get should succeed`) {
		return
	}

	center, ok := set.Get(max / 2)
	if !assert.True(t, ok, `set.Get should succeed`) {
		return
	}

	if !assert.True(t, set.Remove(first), `set.Remove should succeed`) {
		return
	}

	if !assert.Equal(t, max-1, set.Len(), `set.Len should be %d`, max-1) {
		return
	}

	if !checkKeyRemoved(set, first) {
		return
	}

	if !assert.True(t, set.Remove(last), `set.Remove should succeed`) {
		return
	}

	if !assert.Equal(t, max-2, set.Len(), `set.Len should be %d`, max-2) {
		return
	}

	if !checkKeyRemoved(set, last) {
		return
	}

	if !assert.True(t, set.Remove(center), `set.Remove should succeed`) {
		return
	}

	if !assert.Equal(t, max-3, set.Len(), `set.Len should be %d`, max-3) {
		return
	}

	if !checkKeyRemoved(set, center) {
		return
	}
}
@lestrrat
Copy link
Collaborator

Sorry, I just saw your previous message about 10 minutes ago in bed (it's 10pm here), and was already on it. fix coming up soon...

@jozadev24
Copy link
Author

no problem. Sorry, should have seen the other line in the first place. ... bloody indices, always tend to brake my brain. ;-)

@lestrrat
Copy link
Collaborator

No problem. Thanks. At least I was making the same mistake in a uniform manner (or... something)

@lestrrat
Copy link
Collaborator

done and done

@jozadev24
Copy link
Author

thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants