Repository: zitadel/oidc
Pull request #244 (2022-11-14 22:28:49)

feat: add CanRefreshTokenInfo to support non-JWT refresh tokens 
Add an additional, optional, op.Storage interface so that refresh tokens
that are not JWTs do not cause failures when they randomly, sometimes, decrypt without error

```go
// CanRefreshTokenInfo is an optional additional interface that Storage can support.
// Supporting CanRefreshTokenInfo is required to be able to revoke a refresh token that
// does not happen to also be a JWTs work properly.
type CanRefreshTokenInfo interface {
        // GetRefreshTokenInfo must return oidc.ErrInvalidRefreshToken when presented
	// with a token that is not a refresh token.
	GetRefreshTokenInfo(ctx context.Context, clientID string, token string) (userID string, tokenID string, err error)
}
```

This will have a minor conflict with #231: both add NEXT_RELEASE.md.  This branch has the version that should be kept.

codecov[bot]: # [Codecov](https://codecov.io/gh/zitadel/oidc/pull/244?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) Report
> Merging [#244](https://codecov.io/gh/zitadel/oidc/pull/244?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (8ff14d4) into [main](https://codecov.io/gh/zitadel/oidc/commit/0847a5985a5ffa034bf58a32405a61587f9e2de4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (0847a59) will **decrease** coverage by `0.08%`.
> The diff coverage is `0.00%`.

```diff
@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
- Coverage   12.33%   12.24%   -0.09%     
==========================================
  Files          41       41              
  Lines        3325     3347      +22     
==========================================
  Hits          410      410              
- Misses       2903     2925      +22     
  Partials       12       12              
```


| [Impacted Files](https://codecov.io/gh/zitadel/oidc/pull/244?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) | Coverage Δ | |
|---|---|---|
| [pkg/oidc/error.go](https://codecov.io/gh/zitadel/oidc/pull/244/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29pZGMvZXJyb3IuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/op/token\_revocation.go](https://codecov.io/gh/zitadel/oidc/pull/244/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL3Rva2VuX3Jldm9jYXRpb24uZ28=) | `0.00% <0.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel)

muir: @livio-a : I've moved the error definition to `op/storage.go`.  Is there anything else holding you back from approving this?

Patch:
diff --git a/NEXT_RELEASE.md b/NEXT_RELEASE.md
index d4f9e71..91f7f5d 100644
--- a/NEXT_RELEASE.md
+++ b/NEXT_RELEASE.md
@@ -3,4 +3,5 @@
 
 - Add `rp/RelyingParty.GetRevokeEndpoint`
 - Rename `op/OpStorage.GetKeyByIDAndUserID` to `op/OpStorage.GetKeyByIDAndClientID` 
+- Add `CanRefreshTokenInfo` (`GetRefreshTokenInfo()`) to `op.Storage`
 
diff --git a/pkg/oidc/userinfo.go b/pkg/oidc/userinfo.go
index 4d524e3..c8e34d6 100644
--- a/pkg/oidc/userinfo.go
+++ b/pkg/oidc/userinfo.go
@@ -167,6 +167,9 @@ func (u *userinfo) IsPhoneNumberVerified() bool {
 }
 
 func (u *userinfo) GetAddress() UserInfoAddress {
+	if u.Address == nil {
+		return &userInfoAddress{}
+	}
 	return u.Address
 }
 
@@ -389,7 +392,11 @@ func (u *userinfo) UnmarshalJSON(data []byte) error {
 	if err := json.Unmarshal(data, &a); err != nil {
 		return err
 	}
-	u.Address = a.Address
+
+	if a.Address != nil {
+		u.Address = a.Address
+	}
+
 	u.UpdatedAt = Time(time.Unix(a.UpdatedAt, 0).UTC())
 
 	if err := json.Unmarshal(data, &u.claims); err != nil {
diff --git a/pkg/op/storage.go b/pkg/op/storage.go
index 2b3c93f..153cd21 100644
--- a/pkg/op/storage.go
+++ b/pkg/op/storage.go
@@ -2,6 +2,7 @@ package op
 
 import (
 	"context"
+	"errors"
 	"time"
 
 	"gopkg.in/square/go-jose.v2"
@@ -50,6 +51,17 @@ type AuthStorage interface {
 	GetKeySet(context.Context) (*jose.JSONWebKeySet, error)
 }
 
+// CanRefreshTokenInfo is an optional additional interface that Storage can support.
+// Supporting CanRefreshTokenInfo is required to be able to (revoke) a refresh token that
+// is neither an encrypted string of <tokenID>:<userID> nor a JWT.
+type CanRefreshTokenInfo interface {
+	// GetRefreshTokenInfo must return ErrInvalidRefreshToken when presented
+	// with a token that is not a refresh token.
+	GetRefreshTokenInfo(ctx context.Context, clientID string, token string) (userID string, tokenID string, err error)
+}
+
+var ErrInvalidRefreshToken = errors.New("invalid_refresh_token")
+
 type ClientCredentialsStorage interface {
 	ClientCredentialsTokenRequest(ctx context.Context, clientID string, scopes []string) (TokenRequest, error)
 }
diff --git a/pkg/op/token_revocation.go b/pkg/op/token_revocation.go
index db5eea8..9dd0295 100644
--- a/pkg/op/token_revocation.go
+++ b/pkg/op/token_revocation.go
@@ -2,6 +2,7 @@ package op
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/url"
 	"strings"
@@ -31,14 +32,33 @@ func revocationHandler(revoker Revoker) func(http.ResponseWriter, *http.Request)
 }
 
 func Revoke(w http.ResponseWriter, r *http.Request, revoker Revoker) {
-	token, _, clientID, err := ParseTokenRevocationRequest(r, revoker)
+	token, tokenTypeHint, clientID, err := ParseTokenRevocationRequest(r, revoker)
 	if err != nil {
 		RevocationRequestError(w, r, err)
 		return
 	}
-	tokenID, subject, ok := getTokenIDAndSubjectForRevocation(r.Context(), revoker, token)
-	if ok {
-		token = tokenID
+	var subject string
+	doDecrypt := true
+	if canRefreshInfo, ok := revoker.Storage().(CanRefreshTokenInfo); ok && tokenTypeHint != "access_token" {
+		userID, tokenID, err := canRefreshInfo.GetRefreshTokenInfo(r.Context(), clientID, token)
+		if err != nil {
+			// An invalid refresh token means that we'll try other things (leaving doDecrypt==true)
+			if !errors.Is(err, ErrInvalidRefreshToken) {
+				RevocationRequestError(w, r, oidc.ErrServerError().WithParent(err))
+				return
+			}
+		} else {
+			token = tokenID
+			subject = userID
+			doDecrypt = false
+		}
+	}
+	if doDecrypt {
+		tokenID, userID, ok := getTokenIDAndSubjectForRevocation(r.Context(), revoker, token)
+		if ok {
+			token = tokenID
+			subject = userID
+		}
 	}
 	if err := revoker.Storage().RevokeToken(r.Context(), token, subject, clientID); err != nil {
 		RevocationRequestError(w, r, err)


Test patch:
diff --git a/pkg/oidc/userinfo_test.go b/pkg/oidc/userinfo_test.go
index f42ff3d..319a2fd 100644
--- a/pkg/oidc/userinfo_test.go
+++ b/pkg/oidc/userinfo_test.go
@@ -81,3 +81,39 @@ func TestUserInfoEmailVerifiedUnmarshal(t *testing.T) {
 		}, uie)
 	})
 }
+
+// issue 203 test case.
+func Test_userinfo_GetAddress_issue_203(t *testing.T) {
+	tests := []struct {
+		name string
+		data string
+	}{
+		{
+			name: "with address",
+			data: `{"address":{"street_address":"Test 789\nPostfach 2"},"email":"test","email_verified":true,"name":"Test","phone_number":"0791234567","phone_number_verified":true,"private_claim":"test","sub":"test"}`,
+		},
+		{
+			name: "without address",
+			data: `{"email":"test","email_verified":true,"name":"Test","phone_number":"0791234567","phone_number_verified":true,"private_claim":"test","sub":"test"}`,
+		},
+		{
+			name: "null address",
+			data: `{"address":null,"email":"test","email_verified":true,"name":"Test","phone_number":"0791234567","phone_number_verified":true,"private_claim":"test","sub":"test"}`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			info := &userinfo{}
+			err := json.Unmarshal([]byte(tt.data), info)
+			assert.NoError(t, err)
+
+			info.GetAddress().GetCountry() //<- used to panic
+
+			// now shortly assure that a marshalling still produces the same as was parsed into the struct
+			marshal, err := json.Marshal(info)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.data, string(marshal))
+		})
+	}
+}