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

feat: Allow extra fields in introspect response #520

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions handler/oauth2/strategy_jwt_session.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,14 @@ func (s *JWTSession) Clone() fosite.Session {

return deepcopy.Copy(s).(fosite.Session)
}

// GetExtraClaims implements ExtraClaimsSession for JWTSession.
// The returned value is a copy of JWTSession claims.
func (s *JWTSession) GetExtraClaims() map[string]interface{} {
if s == nil {
return nil
}

// We make a clone so that WithScopeField does not change the original value.
return s.Clone().(*JWTSession).GetJWTClaims().WithScopeField(jwt.JWTScopeFieldString).ToMapClaims()
mitar marked this conversation as resolved.
Show resolved Hide resolved
}
24 changes: 19 additions & 5 deletions handler/oauth2/strategy_jwt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ var jwtValidCase = func(tokenType fosite.TokenType) *fosite.Request {
JWTClaims: &jwt.JWTClaims{
Issuer: "fosite",
Subject: "peter",
Audience: []string{"group0"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this still work though? If so can we add a test to cover this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it cannot work because we are filtering out aud from GetExtraClaims. Before the test was in fact buggy, I would claim, and didn't really check if audience is set in the generated (and tested) token. So setting this value does nothing.

IssuedAt: time.Now().UTC(),
NotBefore: time.Now().UTC(),
Extra: map[string]interface{}{"foo": "bar"},
Expand All @@ -71,6 +70,8 @@ var jwtValidCase = func(tokenType fosite.TokenType) *fosite.Request {
r.SetRequestedScopes([]string{"email", "offline"})
r.GrantScope("email")
r.GrantScope("offline")
r.SetRequestedAudience([]string{"group0"})
r.GrantAudience("group0")
return r
}

Expand All @@ -83,7 +84,6 @@ var jwtValidCaseWithZeroRefreshExpiry = func(tokenType fosite.TokenType) *fosite
JWTClaims: &jwt.JWTClaims{
Issuer: "fosite",
Subject: "peter",
Audience: []string{"group0"},
IssuedAt: time.Now().UTC(),
NotBefore: time.Now().UTC(),
Extra: map[string]interface{}{"foo": "bar"},
Expand All @@ -100,6 +100,8 @@ var jwtValidCaseWithZeroRefreshExpiry = func(tokenType fosite.TokenType) *fosite
r.SetRequestedScopes([]string{"email", "offline"})
r.GrantScope("email")
r.GrantScope("offline")
r.SetRequestedAudience([]string{"group0"})
r.GrantAudience("group0")
return r
}

Expand All @@ -112,7 +114,6 @@ var jwtValidCaseWithRefreshExpiry = func(tokenType fosite.TokenType) *fosite.Req
JWTClaims: &jwt.JWTClaims{
Issuer: "fosite",
Subject: "peter",
Audience: []string{"group0"},
IssuedAt: time.Now().UTC(),
NotBefore: time.Now().UTC(),
Extra: map[string]interface{}{"foo": "bar"},
Expand All @@ -129,6 +130,8 @@ var jwtValidCaseWithRefreshExpiry = func(tokenType fosite.TokenType) *fosite.Req
r.SetRequestedScopes([]string{"email", "offline"})
r.GrantScope("email")
r.GrantScope("offline")
r.SetRequestedAudience([]string{"group0"})
r.GrantAudience("group0")
return r
}

Expand All @@ -144,11 +147,10 @@ var jwtExpiredCase = func(tokenType fosite.TokenType) *fosite.Request {
JWTClaims: &jwt.JWTClaims{
Issuer: "fosite",
Subject: "peter",
Audience: []string{"group0"},
IssuedAt: time.Now().UTC(),
NotBefore: time.Now().UTC(),
ExpiresAt: time.Now().UTC().Add(-time.Minute),
Extra: make(map[string]interface{}),
Extra: map[string]interface{}{"foo": "bar"},
},
JWTHeader: &jwt.Headers{
Extra: make(map[string]interface{}),
Expand All @@ -161,6 +163,8 @@ var jwtExpiredCase = func(tokenType fosite.TokenType) *fosite.Request {
r.SetRequestedScopes([]string{"email", "offline"})
r.GrantScope("email")
r.GrantScope("offline")
r.SetRequestedAudience([]string{"group0"})
r.GrantAudience("group0")
return r
}

Expand Down Expand Up @@ -216,6 +220,16 @@ func TestAccessToken(t *testing.T) {
assert.Equal(t, "email offline", scope)
}

extraClaimsSession, ok := c.r.GetSession().(fosite.ExtraClaimsSession)
require.True(t, ok)
claims := extraClaimsSession.GetExtraClaims()
assert.Equal(t, "bar", claims["foo"])
// Returned, but will be ignored by the introspect handler.
assert.Equal(t, "peter", claims["sub"])
assert.Equal(t, []string{"group0"}, claims["aud"])
// Scope field is always a string.
assert.Equal(t, "email offline", claims["scope"])

validate := jWithField.signature(token)
err = jWithField.ValidateAccessToken(nil, c.r, token)
if c.pass {
Expand Down
65 changes: 40 additions & 25 deletions introspection_response_writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,35 +204,50 @@ func (f *Fosite) WriteIntrospectionResponse(rw http.ResponseWriter, r Introspect
return
}

expiresAt := int64(0)
response := map[string]interface{}{
"active": true,
}

extraClaimsSession, ok := r.GetAccessRequester().GetSession().(ExtraClaimsSession)
if ok {
extraClaims := extraClaimsSession.GetExtraClaims()
if extraClaims != nil {
for name, value := range extraClaims {
switch name {
// We do not allow these to be set through extra claims.
case "exp", "client_id", "scope", "iat", "sub", "aud", "username":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must include iss, jti, nbf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really possible to limit. Because currently regular claims from JWT tokens are passed through to introspection endpoint using GetExtraClaims. I tried to not do it like that, but I get import cycle. See: 04cd625

break
mitar marked this conversation as resolved.
Show resolved Hide resolved
default:
response[name] = value
}
}
}
}

if !r.GetAccessRequester().GetSession().GetExpiresAt(AccessToken).IsZero() {
expiresAt = r.GetAccessRequester().GetSession().GetExpiresAt(AccessToken).Unix()
response["exp"] = r.GetAccessRequester().GetSession().GetExpiresAt(AccessToken).Unix()
}
if r.GetAccessRequester().GetClient().GetID() != "" {
response["client_id"] = r.GetAccessRequester().GetClient().GetID()
}
if len(r.GetAccessRequester().GetGrantedScopes()) > 0 {
response["scope"] = strings.Join(r.GetAccessRequester().GetGrantedScopes(), " ")
}
if !r.GetAccessRequester().GetRequestedAt().IsZero() {
response["iat"] = r.GetAccessRequester().GetRequestedAt().Unix()
}
if r.GetAccessRequester().GetSession().GetSubject() != "" {
response["sub"] = r.GetAccessRequester().GetSession().GetSubject()
}
if len(r.GetAccessRequester().GetGrantedAudience()) > 0 {
response["aud"] = r.GetAccessRequester().GetGrantedAudience()
mitar marked this conversation as resolved.
Show resolved Hide resolved
}
if r.GetAccessRequester().GetSession().GetUsername() != "" {
response["username"] = r.GetAccessRequester().GetSession().GetUsername()
}

rw.Header().Set("Content-Type", "application/json;charset=UTF-8")
rw.Header().Set("Cache-Control", "no-store")
rw.Header().Set("Pragma", "no-cache")
_ = json.NewEncoder(rw).Encode(struct {
Active bool `json:"active"`
ClientID string `json:"client_id,omitempty"`
Scope string `json:"scope,omitempty"`
Audience []string `json:"aud,omitempty"`
ExpiresAt int64 `json:"exp,omitempty"`
IssuedAt int64 `json:"iat,omitempty"`
Subject string `json:"sub,omitempty"`
Username string `json:"username,omitempty"`
// Session is not included per default because it might expose sensitive information.
// Session Session `json:"sess,omitempty"`
}{
Active: true,
ClientID: r.GetAccessRequester().GetClient().GetID(),
Scope: strings.Join(r.GetAccessRequester().GetGrantedScopes(), " "),
ExpiresAt: expiresAt,
IssuedAt: r.GetAccessRequester().GetRequestedAt().Unix(),
Subject: r.GetAccessRequester().GetSession().GetSubject(),
Audience: r.GetAccessRequester().GetGrantedAudience(),
Username: r.GetAccessRequester().GetSession().GetUsername(),
// Session is not included because it might expose sensitive information.
// Session: r.GetAccessRequester().GetSession(),
})
_ = json.NewEncoder(rw).Encode(response)
}
58 changes: 49 additions & 9 deletions introspection_response_writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func TestWriteIntrospectionResponseBody(t *testing.T) {
setup func()
active bool
hasExp bool
hasExtra bool
}{
{
description: "should success for not expired access token",
Expand All @@ -93,8 +94,9 @@ func TestWriteIntrospectionResponseBody(t *testing.T) {
sess.SetExpiresAt(ires.TokenUse, time.Now().Add(time.Hour*2))
ires.AccessRequester = NewAccessRequest(sess)
},
active: true,
hasExp: true,
active: true,
hasExp: true,
hasExtra: false,
},
{
description: "should success for expired access token",
Expand All @@ -105,8 +107,9 @@ func TestWriteIntrospectionResponseBody(t *testing.T) {
sess.SetExpiresAt(ires.TokenUse, time.Now().Add(-time.Hour*2))
ires.AccessRequester = NewAccessRequest(sess)
},
active: false,
hasExp: false,
active: false,
hasExp: false,
hasExtra: false,
},
{
description: "should success for ExpiresAt not set access token",
Expand All @@ -117,17 +120,42 @@ func TestWriteIntrospectionResponseBody(t *testing.T) {
sess.SetExpiresAt(ires.TokenUse, time.Time{})
ires.AccessRequester = NewAccessRequest(sess)
},
active: true,
hasExp: false,
active: true,
hasExp: false,
hasExtra: false,
},
{
mitar marked this conversation as resolved.
Show resolved Hide resolved
description: "should output extra claims",
setup: func() {
ires.Active = true
ires.TokenUse = AccessToken
sess := &DefaultSession{}
sess.GetExtraClaims()["extra"] = "foobar"
// We try to set these, but they should be ignored.
for _, field := range []string{"exp", "client_id", "scope", "iat", "sub", "aud", "username"} {
sess.GetExtraClaims()[field] = "invalid"
}
sess.SetExpiresAt(ires.TokenUse, time.Time{})
ires.AccessRequester = NewAccessRequest(sess)
},
active: true,
hasExp: false,
hasExtra: true,
},
} {
t.Run(c.description, func(t *testing.T) {
c.setup()
f.WriteIntrospectionResponse(rw, ires)
var params struct {
Active bool `json:"active"`
Exp *int64 `json:"exp"`
Iat *int64 `json:"iat"`
Active bool `json:"active"`
Exp *int64 `json:"exp"`
Iat *int64 `json:"iat"`
Extra string `json:"extra"`
ClientId string `json:"client_id"`
Scope string `json:"scope"`
Subject string `json:"sub"`
Audience string `json:"aud"`
Username string `json:"username"`
}
assert.Equal(t, 200, rw.Code)
err := json.NewDecoder(rw.Body).Decode(&params)
Expand All @@ -140,6 +168,18 @@ func TestWriteIntrospectionResponseBody(t *testing.T) {
} else {
assert.Nil(t, params.Exp)
}
if c.hasExtra {
assert.Equal(t, params.Extra, "foobar")
} else {
assert.Empty(t, params.Extra)
}
assert.NotEqual(t, "invalid", params.Exp)
assert.NotEqual(t, "invalid", params.ClientId)
assert.NotEqual(t, "invalid", params.Scope)
assert.NotEqual(t, "invalid", params.Iat)
assert.NotEqual(t, "invalid", params.Subject)
assert.NotEqual(t, "invalid", params.Audience)
assert.NotEqual(t, "invalid", params.Username)
}
})
}
Expand Down
22 changes: 22 additions & 0 deletions session.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type DefaultSession struct {
ExpiresAt map[TokenType]time.Time
Username string
Subject string
Extra map[string]interface{}
}

func (s *DefaultSession) SetExpiresAt(key TokenType, exp time.Time) {
Expand Down Expand Up @@ -97,3 +98,24 @@ func (s *DefaultSession) Clone() Session {

return deepcopy.Copy(s).(Session)
}

// ExtraClaimsSession provides an interface for session to store any extra claims.
type ExtraClaimsSession interface {
// GetExtraClaims returns a map to store extra claims.
// The returned value can be modified in-place.
GetExtraClaims() map[string]interface{}
}

// GetExtraClaims implements ExtraClaimsSession for DefaultSession.
// The returned value can be modified in-place.
func (s *DefaultSession) GetExtraClaims() map[string]interface{} {
if s == nil {
return nil
}

if s.Extra == nil {
s.Extra = make(map[string]interface{})
}

return s.Extra
}