Check "irn" JWT header to decide if hybrid token must be introspected, PLTFRM-84867#63
Conversation
Semerokozlyat
force-pushed
the
feature/PLTFRM-84867-introspect-token-on-condition-hybrid-rns
branch
from
f350ce4 to
c681f21
Compare
idptoken/introspector.go
Outdated
| return false | ||
| } | ||
| for i := range scopeFilter { | ||
| sfRN := strings.ToLower(strings.TrimSpace(scopeFilter[i].ResourceNamespace)) |
There was a problem hiding this comment.
Let's use strings.EqualFold (https://pkg.go.dev/strings#EqualFold) instead of strings.ToLower() to avoid memory allocation for case when resource namespace or irn's item contains upper case letter.
idptoken/introspector.go
Outdated
| continue | ||
| } | ||
| for _, iRN := range introspectableRNsArr { | ||
| if sfRN == strings.ToLower(strings.TrimSpace(iRN.(string))) { |
There was a problem hiding this comment.
If irn contains non-string array, panic will be. Suggest asserting to []string above.
There was a problem hiding this comment.
Unfortunately it is not possible since json decoder returns []interface{} even if it was initially slice of strings.
I added "ok" notation to the type assertion of the individual slice element to avoid panic here
idptoken/introspector.go
Outdated
| } | ||
| for i := range scopeFilter { | ||
| sfRN := strings.ToLower(strings.TrimSpace(scopeFilter[i].ResourceNamespace)) | ||
| if sfRN == "" { |
There was a problem hiding this comment.
Why do we need to handle empty resource namespace separately? As I know, we may have empty rs in the scope filter.
There was a problem hiding this comment.
Discussed, added empty string as a valid case.
internal/idputil/idp_util.go
Outdated
|
|
||
| const JWTTypeAppAccessToken = "application/at+jwt" | ||
|
|
||
| const JWTHeaderFieldIRN = "irn" // array of Resource Namespaces for roles available after token introspection only |
There was a problem hiding this comment.
Let's add a comment what irn means hear - introspectable resource namespace.
| // 1. nri field indicates introspection is required (nri absent/false/0) | ||
| // 2. irn (Introspectable Resource Namespace) field contains list of Resource Namespaces matching the scopeFilter | ||
| func checkIntrospectionRequiredByJWTHeader(jwtHeader map[string]interface{}, scopeFilter jwt.ScopeFilter) bool { | ||
| return introspectionRequiredByNRIField(jwtHeader) && |
There was a problem hiding this comment.
I would suggest checking "nri" only if "irn" is missing since the first is deprecated (pls add a comment about it). Otherwise, will be not be able to remove "nri" in the future - this library will not be ready for it.
Semerokozlyat
force-pushed
the
feature/PLTFRM-84867-introspect-token-on-condition-hybrid-rns
branch
4 times, most recently
from
1cbdc3e to
43fe8ff
Compare
idptoken/introspector.go
Outdated
| continue | ||
| } | ||
| if strings.EqualFold( | ||
| strings.TrimSpace(scopeFilter[i].ResourceNamespace), |
There was a problem hiding this comment.
let's move space trimming outside of the nested cycle
There was a problem hiding this comment.
Ok, fixed, but actually I have checked it before and this strings.TrimSpace does not affect performance (cpu or mem):
before:
goos: darwin
goarch: arm64
pkg: github.com/acronis/go-authkit/idptoken
cpu: Apple M1 Pro
BenchmarkIntrospectionRequiredByIRNField-10 44315258 22.89 ns/op 0 B/op 0 allocs/op
after:
goos: darwin
goarch: arm64
pkg: github.com/acronis/go-authkit/idptoken
cpu: Apple M1 Pro
BenchmarkIntrospectionRequiredByIRNField-10 46749982 22.94 ns/op 0 B/op 0 allocs/op
idptoken/introspector.go
Outdated
| } | ||
|
|
||
| func introspectionRequiredByNRIField(jwtHeader map[string]interface{}) bool { | ||
| notRequiredIntrospection, ok := jwtHeader["nri"] |
There was a problem hiding this comment.
since you defined a const JWTHeaderFieldIRN, may I ask you to define another const for the "nri" header as well with small comment what is it and highlight that that header will be deprecated soon?
Semerokozlyat
force-pushed
the
feature/PLTFRM-84867-introspect-token-on-condition-hybrid-rns
branch
from
43fe8ff to
f63abdf
Compare
…ybrid token must be introspected, closes PLTFRM-84867
Semerokozlyat
force-pushed
the
feature/PLTFRM-84867-introspect-token-on-condition-hybrid-rns
branch
from
f63abdf to
5233555
Compare
2 participants
Check Introspectable Resource Namespaces in JWT header to decide if hybrid token must be introspected
Closes PLTFRM-84867