Merge pull request #52 from MicahParks/check_response_status

Change default ResponseExtractor behavior, add `ResponseExtractorStatusAny`, add `Len` method, and fix bug in `ResponseExtractorStatusOK`

README.md

@@ -70,7 +70,7 @@ jwksURL := os.Getenv("JWKS_URL")
 
 // Confirm the environment variable is not empty.
 if jwksURL == "" {
-log.Fatalln("JWKS_URL environment variable must be populated.")
+	log.Fatalln("JWKS_URL environment variable must be populated.")
 }
 ```
 
@@ -81,7 +81,7 @@ Via HTTP:
 // Create the JWKS from the resource at the given URL.
 jwks, err := keyfunc.Get(jwksURL, keyfunc.Options{}) // See recommended options in the examples directory.
 if err != nil {
-log.Fatalf("Failed to get the JWKS from the given URL.\nError: %s", err)
+	log.Fatalf("Failed to get the JWKS from the given URL.\nError: %s", err)
 }
 ```
 Via JSON:
@@ -92,7 +92,7 @@ var jwksJSON = json.RawMessage(`{"keys":[{"kid":"zXew0UJ1h6Q4CCcd_9wxMzvcp5cEBif
 // Create the JWKS from the resource at the given URL.
 jwks, err := keyfunc.NewJSON(jwksJSON)
 if err != nil {
-log.Fatalf("Failed to create JWKS from JSON.\nError: %s", err)
+	log.Fatalf("Failed to create JWKS from JSON.\nError: %s", err)
 }
 ```
 Via a given key:
@@ -103,7 +103,7 @@ uniqueKeyID := "myKeyID"
 
 // Create the JWKS from the HMAC key.
 jwks := keyfunc.NewGiven(map[string]keyfunc.GivenKey{
-uniqueKeyID: keyfunc.NewGivenHMAC(key),
+	uniqueKeyID: keyfunc.NewGivenHMAC(key),
 })
 ```
 
@@ -117,7 +117,7 @@ features mentioned at the bottom of this `README.md`.
 // Parse the JWT.
 token, err := jwt.Parse(jwtB64, jwks.Keyfunc)
 if err != nil {
-return nil, fmt.Errorf("failed to parse token: %w", err)
+	return nil, fmt.Errorf("failed to parse token: %w", err)
 }
 ```
 
@@ -152,9 +152,10 @@ These features can be configured by populating fields in the
 * A custom HTTP request factory can be provided to create HTTP requests for the remote JWKS resource. For example, an
   HTTP header can be added to indicate a User-Agent.
 * A custom HTTP response extractor can be provided to get the raw JWKS JSON from the `*http.Response`. For example, the
-  HTTP response code could be checked. Implementations are responsible for closing the response body. By default, the
-  response body is read and closed, the status code is ignored. The default behavior is likely to be changed soon.
-  See https://github.com/MicahParks/keyfunc/issues/48.
+  HTTP response code could be checked. Implementations are responsible for closing the response body.
+	* By default,
+	  the [`keyfunc.ResponseExtractorStatusOK`](https://pkg.go.dev/github.com/MicahParks/keyfunc#ResponseExtractorStatusOK)
+	  function is used. The default behavior changed in `v1.4.0`.
 * A map of JWT key IDs (`kid`) to keys can be given and used for the `jwt.Keyfunc`. For an example, see
   the `examples/given` directory.
 * A copy of the latest raw JWKS `[]byte` can be returned.

examples/aws_cognito/main.go

@@ -30,7 +30,6 @@ func main() {
 		RefreshRateLimit:  time.Minute * 5,
 		RefreshTimeout:    time.Second * 10,
 		RefreshUnknownKID: true,
-		ResponseExtractor: keyfunc.ResponseExtractorStatusOK,
 	}
 
 	// Create the JWKS from the resource at the given URL.

examples/ctx/main.go

@@ -22,7 +22,6 @@ func main() {
 		RefreshErrorHandler: func(err error) {
 			log.Printf("There was an error with the jwt.Keyfunc\nError: %s", err.Error())
 		},
-		ResponseExtractor: keyfunc.ResponseExtractorStatusOK,
 	}
 
 	// Create the JWKS from the resource at the given URL.

examples/given/main.go

@@ -45,7 +45,6 @@ func main() {
 		RefreshRateLimit:  time.Minute * 5,
 		RefreshTimeout:    time.Second * 10,
 		RefreshUnknownKID: true,
-		ResponseExtractor: keyfunc.ResponseExtractorStatusOK,
 	}
 
 	// Create the JWKS from the resource at the given URL.

examples/interval/main.go

@@ -23,7 +23,6 @@ func main() {
 		RefreshErrorHandler: func(err error) {
 			log.Printf("There was an error with the jwt.Keyfunc\nError: %s", err.Error())
 		},
-		ResponseExtractor: keyfunc.ResponseExtractorStatusOK,
 	}
 
 	// Create the JWKS from the resource at the given URL.

examples/keycloak/main.go

@@ -26,7 +26,6 @@ func main() {
 		RefreshRateLimit:  time.Minute * 5,
 		RefreshTimeout:    time.Second * 10,
 		RefreshUnknownKID: true,
-		ResponseExtractor: keyfunc.ResponseExtractorStatusOK,
 	}
 
 	// Create the JWKS from the resource at the given URL.

examples/recommended_options/main.go

@@ -31,7 +31,6 @@ func main() {
 		RefreshRateLimit:  time.Minute * 5,
 		RefreshTimeout:    time.Second * 10,
 		RefreshUnknownKID: true,
-		ResponseExtractor: keyfunc.ResponseExtractorStatusOK,
 	}
 
 	// Create the JWKS from the resource at the given URL.

get.go

@@ -3,9 +3,7 @@ package keyfunc
 import (
 	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
-	"io"
 	"net/http"
 	"sync"
 	"time"
@@ -32,14 +30,7 @@ func Get(jwksURL string, options Options) (jwks *JWKS, err error) {
 		jwks.requestFactory = defaultRequestFactory
 	}
 	if jwks.responseExtractor == nil {
-		jwks.responseExtractor = func(ctx context.Context, resp *http.Response) (json.RawMessage, error) {
-			// This behavior is likely going to change in favor of checking the response code.
-			// See https://github.com/MicahParks/keyfunc/issues/48
-
-			//goland:noinspection GoUnhandledErrorResult
-			defer resp.Body.Close()
-			return io.ReadAll(resp.Body)
-		}
+		jwks.responseExtractor = ResponseExtractorStatusOK
 	}
 	if jwks.refreshTimeout == 0 {
 		jwks.refreshTimeout = defaultRefreshTimeout

go.mod

@@ -2,4 +2,6 @@ module github.com/MicahParks/keyfunc
 
 go 1.16
 
-require github.com/golang-jwt/jwt/v4 v4.4.1
+require github.com/golang-jwt/jwt/v4 v4.4.2
+
+retract v1.3.0 // Contains a bug in ResponseExtractorStatusOK where the *http.Response body is not closed. https://github.com/MicahParks/keyfunc/issues/51

go.sum

@@ -1,2 +1,2 @@
-github.com/golang-jwt/jwt/v4 v4.4.1 h1:pC5DB52sCeK48Wlb9oPcdhnjkz1TKt1D/P7WKJ0kUcQ=
-github.com/golang-jwt/jwt/v4 v4.4.1/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0=
+github.com/golang-jwt/jwt/v4 v4.4.2 h1:rcc4lwaZgFMCZ5jxF9ABolDcIHdBytAFgqFPbSJQAYs=
+github.com/golang-jwt/jwt/v4 v4.4.2/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0=

jwks.go

@@ -125,6 +125,13 @@ func (j *JWKS) KIDs() (kids []string) {
 	return kids
 }
 
+// Len returns the number of keys in the JWKS.
+func (j *JWKS) Len() int {
+	j.mux.RLock()
+	defer j.mux.RUnlock()
+	return len(j.keys)
+}
+
 // RawJWKS returns a copy of the raw JWKS received from the given JWKS URL.
 func (j *JWKS) RawJWKS() []byte {
 	j.mux.RLock()

jwks_test.go

@@ -205,6 +205,34 @@ func TestJWKS_KIDs(t *testing.T) {
 	}
 }
 
+// TestJWKS_Len confirms the JWKS.Len returns the number of keys in the JWKS.
+func TestJWKS_Len(t *testing.T) {
+	jwks, err := keyfunc.NewJSON([]byte(jwksJSON))
+	if err != nil {
+		t.Fatalf(logFmt, "Failed to create a JWKS from JSON.", err)
+	}
+
+	expectedKIDs := []string{
+		"zXew0UJ1h6Q4CCcd_9wxMzvcp5cEBifH0KWrCz2Kyxc",
+		"ebJxnm9B3QDBljB5XJWEu72qx6BawDaMAhwz4aKPkQ0",
+		"TVAAet63O3xy_KK6_bxVIu7Ra3_z1wlB543Fbwi5VaU",
+		"arlUxX4hh56rNO-XdIPhDT7bqBMqcBwNQuP_TnZJNGs",
+		"tW6ae7TomE6_2jooM-sf9N_6lWg7HNtaQXrDsElBzM4",
+		"Lx1FmayP2YBtxaqS1SKJRJGiXRKnw2ov5WmYIMG-BLE",
+		"gnmAfvmlsi3kKH3VlM1AJ85P2hekQ8ON_XvJqs3xPD8",
+		"CGt0ZWS4Lc5faiKSdi0tU0fjCAdvGROQRGU9iR7tV0A",
+		"C65q0EKQyhpd1m4fr7SKO2He_nAxgCtAdws64d2BLt8",
+		"Q56A",
+		"hmac",
+	}
+
+	actualLen := jwks.Len()
+	expectedLen := len(expectedKIDs)
+	if actualLen != expectedLen {
+		t.Fatalf("The number of key IDs was not as expected.\n  Expected length: %d\n  Actual length: %d\n", expectedLen, actualLen)
+	}
+}
+
 // TestRateLimit performs a test to confirm the rate limiter works as expected.
 func TestRateLimit(t *testing.T) {
 	tempDir, err := os.MkdirTemp("", "*")

options.go

@@ -66,23 +66,30 @@ type Options struct {
 	// HTTP header could be added to indicate a User-Agent.
 	RequestFactory func(ctx context.Context, url string) (*http.Request, error)
 
-	// ResponseExtractor consumes a *http.Response and produces the raw JSON for the JWKS. By default, the raw JSON is
-	// expected in the response body and the response's status code is not checked.
-	//
-	// The default behavior is likely to change soon. See this relevant GitHub issue:
-	// https://github.com/MicahParks/keyfunc/issues/48
+	// ResponseExtractor consumes a *http.Response and produces the raw JSON for the JWKS. By default, the
+	// ResponseExtractorStatusOK function is used. The default behavior changed in v1.4.0.
 	ResponseExtractor func(ctx context.Context, resp *http.Response) (json.RawMessage, error)
 }
 
 // ResponseExtractorStatusOK is meant to be used as the ResponseExtractor field for Options. It confirms that response
 // status code is 200 OK and returns the raw JSON from the response body.
 func ResponseExtractorStatusOK(ctx context.Context, resp *http.Response) (json.RawMessage, error) {
+	//goland:noinspection GoUnhandledErrorResult
+	defer resp.Body.Close()
 	if resp.StatusCode != http.StatusOK {
 		return nil, fmt.Errorf("%w: %d", ErrInvalidHTTPStatusCode, resp.StatusCode)
 	}
 	return io.ReadAll(resp.Body)
 }
 
+// ResponseExtractorStatusAny is meant to be used as the ResponseExtractor field for Options. It returns the raw JSON
+// from the response body regardless of the response status code.
+func ResponseExtractorStatusAny(ctx context.Context, resp *http.Response) (json.RawMessage, error) {
+	//goland:noinspection GoUnhandledErrorResult
+	defer resp.Body.Close()
+	return io.ReadAll(resp.Body)
+}
+
 // applyOptions applies the given options to the given JWKS.
 func applyOptions(jwks *JWKS, options Options) {
 	if options.Ctx != nil {

options_test.go

@@ -43,3 +43,37 @@ func TestResponseExtractorStatusOK(t *testing.T) {
 		t.Fatalf("Expected error to be ErrInvalidHTTPStatusCode.\nError: %s", err)
 	}
 }
+
+func TestResponseExtractorStatusAny(t *testing.T) {
+	var mux sync.Mutex
+	statusCode := http.StatusOK
+
+	server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
+		mux.Lock()
+		writer.WriteHeader(statusCode)
+		mux.Unlock()
+		_, _ = writer.Write([]byte(jwksJSON))
+	}))
+	defer server.Close()
+
+	options := keyfunc.Options{
+		ResponseExtractor: keyfunc.ResponseExtractorStatusAny,
+	}
+	jwks, err := keyfunc.Get(server.URL, options)
+	if err != nil {
+		t.Fatalf("Failed to get JWK Set from server.\nError: %s", err)
+	}
+
+	if len(jwks.ReadOnlyKeys()) == 0 {
+		t.Fatalf("Expected JWK Set to have keys.")
+	}
+
+	mux.Lock()
+	statusCode = http.StatusInternalServerError
+	mux.Unlock()
+
+	_, err = keyfunc.Get(server.URL, options)
+	if err != nil {
+		t.Fatalf("Expected error no error for 500 status code.\nError: %s", err)
+	}
+}