Refactor: Providers create config instead of client #186

jollytoad · 2023-09-06T10:18:50Z

This is an idealistic overhaul at replacing the OAuth2Client being passed around with a configuration, along with a few other refactors to plug leaky abstractions and future proof against potential changes or alternatives in oauth2_client API.

As mentioned in the discussions below it probably goes beyond the remit of the single issue it was meant to originally address, so I'm update this abstract to reflect that, raising new issues to cover the additional refactors and referencing them here. It's likely this PR will get closed with no further change and new PR's later raised, with that in mind I've also switched this back to a Draft.

Closes #174
Closes #193
Closes #195
Closes #196
Closes #197

iuioiua · 2023-09-10T12:16:02Z

This looks good so far!

Let's make this easier. Let's move to configs and drop the current instances as arguments. This is a breaking change, but it is fine as this module is still in beta. We can just be sure to provide adequate documentation of the change.

Also, please don't forget to run deno task ok to ensure everything's good before committing.

jollytoad · 2023-09-11T07:29:19Z

Thanks for the feedback. So you want to just ditch the existing providers (that return a client) and replace them with the config returning fns instead?

iuioiua · 2023-09-11T08:29:29Z

Yep, that sounds good 👍🏾

jollytoad · 2023-09-12T19:38:14Z

@iuioiua ok, I've done the full refactor now, ready for review, have not tested in the wild yet.

src/_internal/resolve_redirect_uri.ts

jollytoad · 2023-09-12T19:41:59Z

src/_internal/oauth2_client.ts

All oauth2_client interaction is abstracted away entirely in this file now

jollytoad · 2023-09-12T19:42:47Z

src/providers/_template.ts

This is a template for new providers, it's not intended to ever be imported directly.
Should probably document this.

jollytoad · 2023-09-12T19:47:40Z

Documentation definitely need a good review

iuioiua · 2023-09-13T14:19:46Z

Actually, I'd very much appreciate if this PR only took care of the objective, and nothing else. The changes in a PR should be single-purpose and this PR includes a heap of additional changes that haven't yet been discussed. I'm happy to discuss these suggestions in a new issue.

jollytoad · 2023-09-13T14:51:47Z

The majority of the changes are to support passing config instead of client around. I'm happy to discuss, but I'm not sure what specifically falls outside.

jollytoad · 2023-09-13T16:02:33Z

src/types.ts

+  clientSecret: string;
+
+  /** The custom domain of the authorization server, if supported by the provider. */
+  domain?: string;


I've added this, as it's a common case, not juist for auth providers such as Okta & Auth0, but also for services that allow self deployment, eg. GitLab (although the gitlab provider doesn't currently support this, it probably should in the future).

jollytoad · 2023-09-13T16:07:40Z

src/types.ts

+   * Default scopes to request, this should be the minimal read scopes for sign-in and
+   * user identity (eg. id, email), it should also include scopes for OpenID Connect if the provider supports it.
+   */
+  scope: string[];


I felt it made more sense to keep the config object flat, and move scope up, out of the 'defaults' prop, which is really an implementation detail within oauth2_client, and doesn't make a lot of sense in the config for this.

Also, I felt it would provide a better DX for a provider to supply a reasonable default scope so it's easier to get going with kv_oauth, ie. no need to go off hunting for scopes just for some basic user details.

This default scope is completely overridden if supplied by the user.

jollytoad · 2023-09-13T16:13:17Z

src/types.ts

This is probably one of the more major changes, introducing the config and some other types directly into kv_oauth. The main reasoning is to detach it from the hard oauth2_client dependency, and to provide a config more attuned to the needs of kv_oauth and its user over the needs of oauth2_client.

For example, bumping scope up, which felt out of place inside 'defaults' whilst refactoring, and ditching the additional request options, which felt very specific to oauth2_client and unnecessary for kv_oauth.

The config type is also broken down by source of config, so it's easier to compose the input and output types of the create config functions.

jollytoad · 2023-09-13T16:16:29Z

src/config.ts

+} from "./types.ts";
+import { assert } from "https://deno.land/[email protected]/assert/assert.ts";
+
+export function createOAuthConfig(


I should probably jsdoc this!

But this adds consistency to the providers by combining the hardcoded config with the env vars and user supplied config in a consistent manner.

jollytoad · 2023-09-13T16:18:57Z

src/core.ts

@@ -38,13 +39,6 @@ addEventListener("beforeunload", async () => {
  await kv.close();
 });

-// OAuth 2.0 session
-export interface OAuthSession {


I move this into types, so that the internal oauth2_client.ts can reference it without depending on this core module from which it doesn't required anything else.

jollytoad · 2023-09-13T16:20:08Z

deps.ts

-  type OAuth2ClientConfig,
-  OAuth2ResponseError,
-  type Tokens,
-} from "https://deno.land/x/[email protected]/mod.ts";


Removed because nothing else requires this other than the _internal/oauth2_client.ts module

jollytoad · 2023-09-13T16:23:02Z

demo.ts

-if (createOAuth2ClientFn === undefined) {
-  throw new Error("Provider not found");
-}
+function getOAuthConfig() {


I had to rejig this a little so that the test case could set the necessary env vars.

jollytoad · 2023-09-13T16:31:52Z

src/types.ts

+ */
+export interface OAuthProviderInfo {
+  /** The name of the provider (single PascalCase name that matches the function name and uppercase env var prefix) */
+  name: string;


I've added this additional info for two purposes:

The env var prefix is derived from this (uppercase).

The user can support multiple providers in their app, and switch UI features or API calls based on this value without having to keep a separate var of the provider chosen, the demo actually demonstrates this nicely.

iuioiua · 2023-09-14T01:58:37Z

I think this PR should take care of replacing OAuth2Client as an argument to OAuth2ClientConfig, wherever OAuth2Client was previously used. Frankly, it's fine if this module is tied to x/oauth2_client. I don't foresee that dependency changing anytime in the future. I think it best not to adjust for changes that may never happen. In the case that such a change does happen, we can make the appropriate adjustments then.

Anything outside of the OAuth2Client argument replacement should be handled in separate issues or PRs. This includes:

Having a /src/_internal folder
Adding support for custom domains of authorization servers
Creating a new OAuthConfig argument
Other refactors and new files

Additionally, we should export OAuth2ClientConfig as part of this module.

jollytoad · 2023-09-14T08:37:31Z

Fair points, I don't entirely agree with being so closely tied to the oauth2_client API, but we can work through this.
I'll have a think, and probably raise some separate issues to cover some of the more contentious ideas.

I'd like to ask for consideration of one compromise: a createOAuthConfig (or similar) fn to consolidate the repeative config logic (ie. consuming env vars)

iuioiua · 2023-09-16T01:15:37Z

I'd like to ask for consideration of one compromise: a createOAuthConfig (or similar) fn to consolidate the repeative config logic (ie. consuming env vars)

Ok, let's see how that looks in a separate PR 👍🏾

iuioiua · 2023-09-17T02:47:22Z

I'm unsure if I was clear, but it'd be great if this PR could continue and only migrate arguments from OAuth2Client to OAuth2ClientConfig. Other concerns can be addressed in independent PRs if that's what's concluded in their respective PRs 🙂

jollytoad · 2023-09-18T08:41:39Z

I'm unsure if I was clear, but it'd be great if this PR could continue and only migrate arguments from OAuth2Client to OAuth2ClientConfig.

What I mean is that it may be simpler to rebranch and cherry pick those parts, resulting in a new PR. I'm tied up on other things again atm, I'll see what I can squeeze in.

iuioiua · 2023-09-18T08:52:12Z

Fair enough. I'll submit a PR tomorrow which handles the migration and ping you once done 🙂

iuioiua · 2023-09-20T01:21:06Z

Closing in favour of #207. Thank you, either way, Mark!

jollytoad added 2 commits September 6, 2023 11:14

Initial proposal for provider configs

4616798

Merge branch 'denoland:main' into providers-config

9fe1168

jollytoad added 2 commits September 12, 2023 20:30

refactor: migrate all providers to supply config instead of client

e8d894b

Merge branch 'main' into providers-config

52c6f60

jollytoad changed the title ~~Draft: Initial proposal for provider configs~~ Refactor: Providers create config instead of client Sep 12, 2023

jollytoad commented Sep 12, 2023

View reviewed changes

src/_internal/resolve_redirect_uri.ts Show resolved Hide resolved

jollytoad commented Sep 12, 2023

View reviewed changes

src/_internal/oauth2_client.ts

Copy link

Contributor Author

jollytoad Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All oauth2_client interaction is abstracted away entirely in this file now

jollytoad commented Sep 12, 2023

View reviewed changes

jollytoad marked this pull request as ready for review September 13, 2023 07:06

jollytoad added 2 commits September 13, 2023 14:06

add test for resolveRedirectUri

f6d96c5

add assertStrictEquals dev dep

2594738

jollytoad commented Sep 13, 2023

View reviewed changes

jollytoad marked this pull request as draft September 14, 2023 09:11

iuioiua closed this Sep 20, 2023

iuioiua mentioned this pull request Sep 20, 2023

BREAKING: use and create OAuth2ClientConfig instead of OAuth2Client #207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Providers create config instead of client #186

Refactor: Providers create config instead of client #186

jollytoad commented Sep 6, 2023 •

edited

Loading

iuioiua commented Sep 10, 2023

jollytoad commented Sep 11, 2023

iuioiua commented Sep 11, 2023

jollytoad commented Sep 12, 2023

jollytoad Sep 12, 2023

jollytoad Sep 12, 2023

jollytoad commented Sep 12, 2023

iuioiua commented Sep 13, 2023

jollytoad commented Sep 13, 2023

jollytoad Sep 13, 2023

jollytoad Sep 13, 2023

jollytoad Sep 13, 2023 •

edited

Loading

jollytoad Sep 13, 2023

jollytoad Sep 13, 2023

jollytoad Sep 13, 2023

jollytoad Sep 13, 2023

jollytoad Sep 13, 2023 •

edited

Loading

iuioiua commented Sep 14, 2023

jollytoad commented Sep 14, 2023 •

edited

Loading

iuioiua commented Sep 16, 2023 •

edited

Loading

iuioiua commented Sep 17, 2023

jollytoad commented Sep 18, 2023

iuioiua commented Sep 18, 2023

iuioiua commented Sep 20, 2023 •

edited

Loading

Refactor: Providers create config instead of client #186

Refactor: Providers create config instead of client #186

Conversation

jollytoad commented Sep 6, 2023 • edited Loading

iuioiua commented Sep 10, 2023

jollytoad commented Sep 11, 2023

iuioiua commented Sep 11, 2023

jollytoad commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jollytoad commented Sep 12, 2023

iuioiua commented Sep 13, 2023

jollytoad commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jollytoad Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jollytoad Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

iuioiua commented Sep 14, 2023

jollytoad commented Sep 14, 2023 • edited Loading

iuioiua commented Sep 16, 2023 • edited Loading

iuioiua commented Sep 17, 2023

jollytoad commented Sep 18, 2023

iuioiua commented Sep 18, 2023

iuioiua commented Sep 20, 2023 • edited Loading

jollytoad commented Sep 6, 2023 •

edited

Loading

jollytoad Sep 13, 2023 •

edited

Loading

jollytoad Sep 13, 2023 •

edited

Loading

jollytoad commented Sep 14, 2023 •

edited

Loading

iuioiua commented Sep 16, 2023 •

edited

Loading

iuioiua commented Sep 20, 2023 •

edited

Loading