Inject RefreshErrorHandler option

[q42jaap]

jwkset uses slog.Default() which, in an application that doesn't use slog feels off.

Also slog.Default() might be setup with a special handler, where an application that uses slog might have a different approach.

In my project I'm using zap and use logger.With to set tagging, and put it in the context I pass down so code down the line can use the logger whenever it needs to.

For the jwkset this isn't all that important, but having no option to set the slog handler that jwkset makes it so I can't tag the logs that come from jwkset with anything. We're using google and apple oidc urls with keyfunc/v3, so I'd like to be able to tag the logs with the provider. If that's not possible (it might mean a breaking change) at the very least I'd want to set the default slog logger jwkset uses: jwkset.SetLogger(logger *slog.Logger) and use that instead of slog.Default()

If interested I'm happy to make a PR.

[MicahParks]

The jwkset.com website and jwksetinfer CLI tool, both of which is included in this repository, use log/slog. That code is not meant to be included as an external dependency. The website and CLI tool are likely to use log/slog indefinitely.

The top level package, jwkset, is meant to be included as an external dependency and does use log/slog in the below snippet. I don't think it is used anywhere else. https://github.com/MicahParks/jwkset/blob/00771507f16ef5908cb45e9df0175b3e37042391/http.go#L102-L107

This snippet is from the NewDefaultHTTPClientCtx function. This function is meant to automatically configure a jwkset.Storage using defaults that I personally think would fit a general use case. I recognize that my opinion of a general use case likely does not align with many software engineers, so there are other functions exported for more customized approaches. In this case, the jwkset.NewHTTPClient function would be the lower level alternative to jwkset.NewDefaultHTTPClientCtx function. The jwkset.NewHTTPClient would require more lines added to your project to create the jwkset.HTTPClientOptions.

At the moment, I think this will behavior will remain the jwkset project. If you would like to use your own logger, I recommend using the jwkset.NewHTTPClient function directly. If you would like to simply turn off the default log/slog logger, the below snippet should help. It can be placed near the top of your main() function. This suggestion may be out of date soon, it looks like the Golang project may include an easier way to do a no-operation log/slog. See this issue to track that.

slog.SetDefault(slog.New(slog.NewTextHandler(io.Discard, nil)))

Specifically regarding a function with the signature jwkset.SetLogger(logger *slog.Logger), I would not be in favor of a change like this at the moment because this appears to set a global logger variable for the package. I would prefer to keep this project global variable free.

If you are coming from the github.com/MicahParks/keyfunc project, I do plan on improving that project to be easier to use for non-default use cases. You can see an issue related to that linked here. If you are using that project and want to add a few comments about how I can make that project easier for you, please do comment on the issue or create a new issue.

Thank you.

[q42jaap]

Thanks for your answer. I've decided to use the jwkset.NewHTTPClient function directly, but keep all the defaults. The function currently looks like this:

type DefaultHTTPClientOptions struct {
    Urls                []string
    RefreshErrorHandler func(url string) RefreshErrorHandler
}

type RefreshErrorHandler func(ctx context.Context, err error)

// NewDefaultHTTPClientCtxWithOpts is equivalent to jwkset.NewDefaultHTTPClientCtx with additional options.
func NewDefaultHTTPClientCtxWithOpts(ctx context.Context, options DefaultHTTPClientOptions) (jwkset.Storage, error) {
    clientOptions := jwkset.HTTPClientOptions{
        HTTPURLs:          make(map[string]jwkset.Storage),
        RateLimitWaitMax:  time.Minute,
        RefreshUnknownKID: rate.NewLimiter(rate.Every(5*time.Minute), 1),
    }
    for _, u := range options.Urls {
        parsed, err := url.ParseRequestURI(u)
        if err != nil {
            return nil, fmt.Errorf("failed to parse given URL %q: %w", u, errors.Join(err, jwkset.ErrNewClient))
        }
        u = parsed.String()
        options := jwkset.HTTPClientStorageOptions{
            Ctx:                       ctx,
            NoErrorReturnFirstHTTPReq: true,
            RefreshErrorHandler:       options.RefreshErrorHandler(u),
            RefreshInterval:           time.Hour,
        }
        c, err := jwkset.NewStorageFromHTTP(parsed, options)
        if err != nil {
            return nil, fmt.Errorf("failed to create HTTP client storage for %q: %w", u, errors.Join(err, jwkset.ErrNewClient))
        }
        clientOptions.HTTPURLs[u] = c
    }
    return jwkset.NewHTTPClient(clientOptions)
}

As you see it's just copied from NewDefaultHTTPClientCtx with an options struct to pass in the RefreshErrorHandler. Add a function that takes the RefreshErrorHandler could be a non-breaking change in jwkset. The currentNewDefaultHTTPClientCtx func could then call into NewDefaultHTTPClientCtxWithOpts with the RefreshErrorHandler that has the current dependency on slog.

For zap my alternative Keyfunc constructor function looks like this:

package keyfunc_zap

// ...

func NewDefaultCtxZap(ctx context.Context, urls []string, logger *zap.Logger) (keyfunc.Keyfunc, error) {
    opts := DefaultHTTPClientOptions {
        Urls: urls,
        RefreshErrorHandler: func(url string) RefreshErrorHandler {
            urlLogger := logger.With(zap.String("url", url))
            return func(ctx context.Context, err error) {
                urlLogger.Error("Failed to refresh HTTP JWK Set from remote HTTP resource.", zap.Error(err))
            }
        },
    }

    client, err := NewDefaultHTTPClientCtxWithOpts(ctx, opts)
    if err != nil {
        return nil, err
    }
    options := keyfunc.Options{
        Storage: client,
    }
    return keyfunc.New(options)
}

If you're interested I could still make a PR for this, or feel free to adopt this in a different style. The main difference I wanted to achieve is to pass in the error handler and leave the defaults as is.

[MicahParks]

I'm glad to see your goal is now accomplished :rocket:

My current plan is to create a new function or two, something named like DefaultHTTPClientOptions and DefaultHTTPClientStorageOptions. These functions would build the existing options data structures with default values and return them. The goal would be to use these new functions to create options that make using jwkset.NewHTTPClient more convenient to get to. I would prefer to not declare any new data structures for this.

In the keyfunc project, there will probably a new data structure to declare what is different from the default, since that project is past the stable release for semver. That will require comparing the default data structure to the user provided data structure in order to build a new data structure with user-provided values and default values.

I haven't started on that work yet, it's been a busy few months. But it's still on my to do list.

[q42jaap]

Nice work, I'll close this issue now.