@high-optician-2097 When you get a chance, could you please review the PR. Once we are through PAR, I wanted to move to the tls_client_auth. I am assuming all of these are going to be useful for Hydra as well.

@high-optician-2097 Not to bug you but I am just trying to get a sense of when you will be able to get to this PR. I don't want to work on the tls_client_auth and other pieces until this is merged in to avoid dealing with conflicts.

before starting mtls, PTAL: https://github.com/ory/fosite/pull/667

I am curious to know why you decided to move to interface functions to get config values. We had a need to support the ability to load Fosite config from a multi-tenanted config store and the approach we ended up taking was to store the tenant-specific compose.Config instance into the ctx at the API handler level, then modifying the Fosite handlers, where needed, to extract the Config struct. I was going to propose an interface for

MultiProviderConfigurator

that contains a single

GetConfig(context.Context) compose.Config

and modify the Fosite handlers to call into this to get the config object. With that approach, the thought process was to leave the Config object as a struct because we felt it would be easier to extend it with newer struct members. And of course modify the Fosite struct with

*compose.Config

as a member representing "default" configuration, rather than duplicate the properties. With an interface, every new capability with new properties (like tls_client_auth will have) is going to require a new interface provider. This isn't necessarily a problem at all but it also means any extensions to current capabilities will also need a new interface for backward compatibility. Have you considered an alternative where the config instance in Fosite would be retained as a struct object and not as an interface and, instead offer the option of the MultiProviderConfigurator, which would also provide the same capabilities to perform hot reloading or to support multiple OAuth providers using a single Fosite object?

The PR appears to have advanced a fair bit and I am not sure if this discussion is too late. But if it isn't, I would like to put this approach forward for consideration

btw we chose another approach because this is about hot reloading, and that doesn't really work well with static structs

sorry for the short reply, i was on the phone over the qeekend only

with funcs you just return the most recent value easily, with structs you replace the whole thing if that makes sense

FWIW just sharing perspective. Our provider config is served by a config API. We cache the config object in bigcache and make it available in our overriden handlers. The interface approach can be easily adapted but we do the same type of hot reloading, except the mechanism we use involves a config-change notification that evicts the cache and so, the next request for that provider/tenant results in the config object being reconstructed. The reason this scales well in general is because the config doesn't change often.

Now with regards to the PAR, should I be based it off this branch or are you fine with us adding an interface when the config PR is merged?

no that’s fine wrt to PAR, let’s get it merged

I can make the change in the same PR if you like. I thought I saw another PR that had the same issue.

@high-optician-2097 I am likely missing something. With the hot reloading PR, wouldn't every new feature effectively break the Fosite.Config implementation? Basically, the way I understood it is, if a new feature is being added with additional properties, the following would need to be done: 1. Add a new provider interface 2. Add the provider interface to the Configurator interface => breaks backward compatibility Or are you thinking that the Configurator interface is going to remain static and any additional handlers would need to type check the new provider interface?

I guess the latter is the approach to take.

@high-optician-2097 Will you require the fosite-example PR to be opened before reviewing and merging https://github.com/ory/fosite/pull/660? I am not sure if I can get to that before the weekend, so just wanted to check.

@high-optician-2097 The changes are in https://github.com/ory/fosite/pull/660. Please review when you get a chance. Hopefully we can merge before more changes introduce more conflicts 🙂