Add getOAuthHelpers method #117
Conversation
…e of a fetch call
|
|
@mattzcarey - Can you give this a review? @kentonv - Found a neat way to add methods to the provider without exposing them over RPC |
| * @param env - Cloudflare Worker environment variables | ||
| * @returns An instance of OAuthHelpers | ||
| */ | ||
| export function getOAuthHelpers(provider: OAuthProvider, env: any): OAuthHelpers { |
There was a problem hiding this comment.
Hmm, so, this API works (and it's what I asked for), but it still feels weird to me.
I wonder if it should be something like:
function getInternalApi(options: OAuthProviderOptions, storage: KvNamespace): OAuthHelpers;
That is, you don't pass it an existing provider instance, nor env... you just pass in the options and storage namespace, and it makes a new one. (This might require making a hidden OAuthProviderImpl instance under the hood, but that's OK.)
Feels a little cleaner as a library function. OAuthProvider has a sort of weird design since it's specifically designed to be a wrapper around your whole app. But the point of this function is for when you don't want OAuthProvider wrapping your whole app...
There was a problem hiding this comment.
Thinking out loud, we can frame the elements this way:
OAuthProviderImpl: Core OAuth toolset
OAuthHelpers/OAuthHelpersImpl: Take in OAuthProviderImpl and returns a subset of features publicly
OAuthProvider: Takes in OAuthProviderOptions, creates OAuthProviderImpl, and returns OAuthHelpers+ a routing layer.
New element: Takes in OAuthProviderOptions, creates OAuthProviderImpl and returns OAuthHelpers.
Yeah it's a messy task because on one hand, the last two elements do nearly the same thing, but on the other, OAuthProvider's primary purpose could be seen as routing and reusing it does mix concerns. Then again, OAuthProviderOptions deals mostly with routing so we're mixing concerns either way.
There was a problem hiding this comment.
This is how I currently use it:
// Create the OAuth provider instance
const oauthProvider = new OAuthProvider({
apiHandlers: {},
defaultHandler: defaultHandler,
authorizeEndpoint: '/authorize',
tokenEndpoint: '/token',
clientRegistrationEndpoint: '/register',
tokenExchangeCallback: tokenExchangeCallback,
scopesSupported: AllScopes,
allowImplicitFlow: false,
allowTokenExchangeGrant: false
});
export default class extends WorkerEntrypoint<AuthWorkerEnv> {
async fetch(request: Request) {
// ... other code ...
return oauthProvider.fetch(request, this.env, this.ctx);
}
/** Manually register a client (RPC) */
public async upsertClient(data: OAuthClientInfo, clientType: ClientType): Promise<OAuthClient> {
const oauthHelpers = getOAuthHelpers(oauthProvider, this.env);
return await oauthHelpers.lookupClient(data.clientId ?? '');
}
}
This is the alternative:
const oauthOptions = {
apiHandlers: {},
defaultHandler: defaultHandler,
authorizeEndpoint: '/authorize',
tokenEndpoint: '/token',
clientRegistrationEndpoint: '/register',
tokenExchangeCallback: tokenExchangeCallback,
scopesSupported: AllScopes,
allowImplicitFlow: false,
allowTokenExchangeGrant: false
};
// Create the OAuth provider instance
const oauthProvider = new OAuthProvider(oauthOptions);
export default class extends WorkerEntrypoint<AuthWorkerEnv> {
async fetch(request: Request) {
// ... other code ...
return oauthProvider.fetch(request, this.env, this.ctx);
}
/** Manually register a client (RPC) */
public async upsertClient(data: OAuthClientInfo, clientType: ClientType): Promise<OAuthClient> {
const oauthHelpers = getOAuthHelpers(oauthOptions, this.env);
return await oauthHelpers.lookupClient(data.clientId ?? '');
}
}
There was a problem hiding this comment.
I'm ok with the alternative (sharing options), but would keep the signature as (options, env) since env is used everywhere down the line. Can always add a KVNamespace to OAuthProviderOptions if we want to make it configurable later.
There was a problem hiding this comment.
Hmm well the env part is actually the part that bugs me more, it doesn't feel like you should have to give it your whole env. But I'm always the guy who is weirdly protective of my env in a way nobody else seems to care about.
At the very least though I don't think this function should inject OAUTH_PROVIDER into env... that seems unnecessary, given the caller already has a reference to the provider and can do it themselves if they really want to.
There was a problem hiding this comment.
I'll put the OAUTH_PROVIDER line back into the fetch method where it came from. Right, no one will be using that_and_ this method.
I wouldn't remove env though. Between OAuthProviderImpl.fetch(req, env, ctx) and import { env } from "cloudflare:workers", it's getting in there anyway. Even in RPC methods, you can still
public async bypass() {
// Dummy call to initialize the OAuth provider
await this.fetch(new Request('https://test.com/token', { method: 'GET' }));
// Now we can use the OAuth provider with env in it
this.env.OAUTH_PROVIDER.lookupClient('test');
}
There was a problem hiding this comment.
A bit busy today and tomorrow but will implement any changes later this week!
This PR makes OAuthHelpers available outside of the
fetchmethod (i.e. in a worker's RPC methods).Problem
this.env.OAUTH_PROVIDERgets populated inOAuthProviderImpl.fetch(), so if we try to use it in a worker's RPC methods, for example, it will always be undefined.Solution
We need a method
OAuthProvider.getOAuthHelpers(env: any)that initializes and retrieves OAuthHelpers directly from the provider.❌ We can't put the method directly onto the provider because it will get exposed, intentionally or otherwise, as a public RPC method on the worker.
✅ But if we use a Symbol to name the method, it appears on the provider like we want, but excluded from the worker's RPC methods.