auth: fix panic using WithRoot and improve JWT coverage #9698
Conversation
|
@gyuho FYI https://jenkins-etcd-public.prod.coreos.systems/job/etcd-ci-ppc64/4075/console |
|
@mkumatag Can you clean up the disk :) |
|
@gyuho PTAL, thanks. |
|
@hexfusion Can we add a simple unit test? Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #9698 +/- ##
==========================================
+ Coverage 69.49% 69.76% +0.26%
==========================================
Files 375 375
Lines 35179 35179
==========================================
+ Hits 24449 24541 +92
+ Misses 8981 8887 -94
- Partials 1749 1751 +2
Continue to review full report at Codecov.
|
@gyuho yes sir will address |
|
@gyuho test added, thanks. |
|
I took a second look, and this seems wrong. Request with JWT token should also be passing contexts with auth information. See We just need to check as if ts, ok := as.tokenProvider.(*tokenSimple); ok && ts != nil { Also we need unit tests to cover this line, not on the util function. |
auth/store.go
Outdated
| @@ -1284,7 +1284,7 @@ func NewTokenProvider( | |||
| } | |||
|
|
|||
| func (as *authStore) WithRoot(ctx context.Context) context.Context { | |||
| if !as.IsAuthEnabled() { | |||
| if tt := tokenProviderType(as); tt != "simple" || !as.IsAuthEnabled() { | |||
There was a problem hiding this comment.
we probably should define "simple" as a const.
There was a problem hiding this comment.
@xiang90 good idea, done.
hexfusion
force-pushed
the
fx_auth
branch
2 times, most recently
from
01dde0c to
dd7d4e4
Compare
hexfusion
changed the title
|
Updated the check and added some testing. In general, it is very awkward how NewTokenProvider accepts opts as a string, that is fine for simple but not for jwt. I didn't address this here but this should be looked at further. I also removed the util function and tests as they were not necessary. /cc @gyuho @joelegasse |
hexfusion
changed the title
There was a problem hiding this comment.
@gyuho The only times those context values are used is by the simple token during assign (token creation). This means that we don't need a type-switch to handle other token types (for now).
|
@gyuho @joelegasse @xiang90 thank you! |
etcd-io#9698 wasn't really testing the panic code path when leases are expiry. Signed-off-by: Gyuho Lee <[email protected]>
etcd-io#9698 wasn't really testing the panic code path when leases are expiry. Signed-off-by: Gyuho Lee <[email protected]>
ref: #9695