Suggestion: Providers return config only

[jollytoad]

Currently all the providers create a config and then they all do exactly the same thing, create an OAuth2Client instance using that config, this ties deno_kv_oauth to the existing oauth2_client API quite tightly.

An OAuth2Client contains an amount of redundant features that are not used by deno_kv_oauth (ie. implicit, ropc, clientCredentials), also you never need both code and refresh together.

If the providers just returned the config, and this config was passed to signIn/handleCallback etc, those fns could then create the client, and allow for future evolution of the client API or even a completely alternative oauth2 lib.

Users of deno_kv_oauth need not care about the underlying client lib at all, they'll just be exposed to the config interface.

[iuioiua]

Interesting thought. I'll have to think about this. I'd be happy to hear other's opinions.

also you never need both code and refresh together.

Do you mean within the same function? Could you please elaborate?

[jollytoad]

So only client.code is used by signin & callback, and client.refresh is used by the session fn (can't remember the fn name off top of my head), ie. separate parts of the app, separate http requests. So there isn't really a need for all those grant type objects to be created and hanging around in the heap. To put it another way, the client only really needs to be created when required in a request, and then only a small part of what the client contains is ever used in that request.

[jollytoad]

@iuioiua I've just been scanning through the oidc branch, and the support in oauth2_client module too.

I'm thinking that this adaptation of providers returning a config would benefit this situation, in that the signIn/handleCallback could then internally pick the appropriate client class (OIDCClient, OAuth2Client, or even just AuthorizationCodeGrant) as necessary based on the config.

I've also been thinking it could be implemented in a backwards compatible fashion by exporting a separate function, maybe even as the default fn, this could make switching provider a lot easier and even configurable in the import map.

eg:

{
  "imports": {
    "$auth_provider": "https://.../providers/google.ts"
  }
}

import getAuthConfig from "$auth_provider";

const config = getAuthConfig();

// in your handler...
signIn(req, config);

or, without import map:

import getGoogleAuthConfig from "https://.../providers/google.ts"

createXxxOAuthClient fns could be deprecated, and have them just obtain config from the new fn.

signIn/handleCallback etc could have a transition period where they accept either the OAuth2Client or the config.

[iuioiua]

I'm thinking that this adaptation of providers returning a config would benefit this situation, in that the signIn/handleCallback could then internally pick the appropriate client class (OIDCClient, OAuth2Client, or even just AuthorizationCodeGrant) as necessary based on the config.

Yep, good point.

I've also been thinking it could be implemented in a backwards compatible fashion by exporting a separate function, maybe even as the default fn, this could make switching provider a lot easier and even configurable in the import map.

Yeah. Perhaps, createGitHubOAuthConfig() in /src/providers/github.ts, for example.

Ok, doing this might be a good idea. Thoughts, @kt3k?

[kt3k]

Sounds like a good abstraction to me.

Users of deno_kv_oauth need not care about the underlying client lib at all, they'll just be exposed to the config interface.

I agree with this.

[iuioiua]

Cool! Let's do it then. @jollytoad, would you like to take on the PR? I ask so we can see the extent of your suggestions. Either way, we can go from there.

[jollytoad]

I would certainly be happy too, I'm a bit busy until the end of next week at present. I have a demo to prep for work. I'll see what I can do, but I may not get around to it fully until after that.