Identity System Overhaul

[Nicknakin]

Heya, this isn't meant to be merged quite yet, just want to get the PR out there while I have something that works. I'd love some feedback, I believe this addresses issue #43 in a round-about way.

The gist of the change is that all of the twitch specific code has been moved to /identity/twitch.ts and is now exposed by importing it in /identity/identity.ts which the rest of the application imports for any auth needs. It seems that the twitch integration was performing two primary tasks, acting as an identity server and allowing for fetching arbitrary user information by id, any oidc provider can be swapped in for the first, however getting arbitrary user information by subscriber id is unlikely to be supported by any given identity provider.

Forgejo is the identity provider I tested against locally for a generic oidc integration, I also threw my hat in at integrating a google identity provider and got it working, with the exception of arbitrary user polling.

[Nicknakin]

I'm noticing my editor seems to have automatically done a lot of style specific changes, is that a problem? I also ran linting afterwards, so none of it seems to be violating linting rules.

[Nicknakin]

Tested with a local copy of the whole stack and seems to work fine.

[SkyeOfBreeze]

we should probably remove this

[SkyeOfBreeze]

this line still mentions twitch

[SkyeOfBreeze]

it looks like this does not currently allow for multiple identities to coexist? do we want that, or in that case, that just uses equivalent of forgejo?

[Nicknakin]

mydnzi was saying they preferred that we only allow a single provider to function at a time, so this is mostly a refactor of the existing single-provider system that's modular enough that I can swap out what I'm using for my local version of the instance

[myndzi]

There was a discussion (sorry about the tangents) in the thread about the goal of the PR. Stated goal was for supporting self-hosting with other providers, and I think it's preferable to serve that need but not make the rest of the code more complicated by supporting multiple concurrent identities from various providers. It's not a hard veto or anything, but I think there are a lot of knock-on effects we'd have to think about and deal with when someone can, say, have the same username on Google as a different human on Twitch and they both play in the same room for example. There are also consequences for banning and ban evading and so on

[myndzi]

(In other words, if the site runs with exactly and only one auth provider, but you can choose which auth provider to run it with, you support "people running different servers" while not complicating the actual lobby and game code)

[Nicknakin]

I did, however, not do anything to intentionally prevent the future implementation of multiple Identity providers, just made it so it's easier to drop in a different set of handlers

[myndzi]

Left some general thoughts and some specific suggestions regarding simplifying all the overly defensive code + async/await and promise behavior.

I realize you probably just copypasted the twitch code into the google code, so you've inherited the sins of the past. It'd be good to reflect the same improvements to google.ts into twitch.ts as well. The code suggestions here won't provide a complete working Typescript solution; the return values of the providers need to be tightened up (no returning a promise for UserData|null rather than the interface loosened)

[myndzi]

This directory (entity) is for TypeORM models; this interface is probably better-kept in the identity directory alongside the oauth stuff that depends on it

[Nicknakin]

Existing response type definitions for the pure twitch implementation were in the entity directory, I can find a new place to move it to, but I can't put it in the identity.ts since it would cause a cyclical dependency.

[myndzi]

Suggested change

	const getRedirectURL = (redirectURL: string, scope: string, state: string): string => {
	const url = oauth2Client.generateAuthUrl({
	scope: scopes
	});
	return url;
	}
	const getRedirectURL = (redirectURL: string, scope: string, state: string): string =>
	oauth2Client.generateAuthUrl({
	scope: scopes
	});

(style nit: function body here is doing no work, so it's slightly better as an expression)

[myndzi]

Suggested change

	const handleRedirectResponse = async (code: string, state: string, redirectUri: string, grantType: string): Promise<UserData> => {
	const response = await oauth2Client.getToken({
	client_id: GOOGLE_CLIENT_ID,
	code: code,
	codeVerifier: state,
	redirect_uri: redirectUri,
	});
	const access_token = response.tokens.id_token ?? "";
	const userData = await getUser(access_token);
	return userData;
	};
	const handleRedirectResponse = (code: string, state: string, redirectUri: string, grantType: string): Promise<UserData> =>
	oauth2Client.getToken({
	client_id: GOOGLE_CLIENT_ID,
	code: code,
	codeVerifier: state,
	redirect_uri: redirectUri,
	}).then(({id_token}) => getUser(id_token));

getUser with an empty string as the id_token will never succeed. Indeed, getToken (the docs suggest) will never return an object without the id_token. In either case, the nullish-coalescing behavior here is unnecessary/overly defensive. What we expect is that if getToken fails, it will reject its promise - and that rejection will propagate to the caller.

We can return the promise given by getUser directly rather than "unpacking" it with await and "re-packing" it with return.

Converting this to a non-async function leads to clearer logic and expectations in this case too: the viewer is left with fewer questions about the content and consistency of the data, and the function now represents only the "wiring" of how to connect the pieces of "google oauth" with the interface consumed by the rest of the program.

The conversions from statements to expression in this file are stylistic only, and either way is fine; mostly, when I started cutting the unnecessary bits I was left with single-statement expressions (or, in this case, a simple promise chain) that naturally reduced themselves to this form.

[myndzi]

This is probably "sins of the past" or overly-defensive coding, but I'm not sure.

In an async function, a rejected promise already throws an error if not in a try/catch block. So really, this code should probably simply be:

await validateAuthServer()

...but with the expectation that validateAuthServer returns a valid object.

There's no real need to initiate and store the "validateAuthServer" promise separately on line 24, so that line can just be deleted and the function called directly here in the function body. Hanging on to the promise suggests it is used elsewhere, which (as best I can tell) it isn't. This also ensures that the contents of the promise's value are properly garbage collected.

[myndzi]

state in oauth is a concern of the oauth integration. So, the value of state should be opaque to this code and simply passed along to the identity provider for verification (if verification is desired) or whatever behavior is tied to its contents.

I honestly don't know what's going on with this code here; I think the whole storing user in the database thing (pendingConnections?) is unnecessary and may have various logical errors, but I'd have to investigate more interactively to reach a conclusion.

No change requested here, but the change from state.length to state?.length just makes me even more sus of this whole section of code.

[Nicknakin]

Not sure how that changed, happy to have it go either way

[myndzi]

I believe you asked a question somewhere about how to generalize this so that it doesn't require a code change to switch providers. Here's one way you can do that.

1. define each identity provider in a map (in this case, I'll use an object for convenience)
2. create a function to return the requested provider by its name

Example:

(current code)

import * as twitch from './twitch';
import * as google from './google';

const providers = {twitch, google};

export const getIdentityProvider = (name: keyof typeof providers) => providers[name];

export const getConfiguredProvider = () => {
  const name = process.env.IDENTITY_PROVIDER ?? 'twitch';
  if (Object.prototype.hasOwnProperty.call(providers, name)) return getIdentityProvider(name as any);
  throw new Error(`No such identity provider: '${name}'. Expected: ${Object.keys(providers).join(', ')}`);
}

getConfiguredProvider() will resolve to the set of exported functions using an environment variable to select the provider to use, or fail. getIdentityProvider('name') will return a specific provider.

If you want to ensure better conformance / compatibility using typescript, you can define the providers slightly differently to conform to an interface:

identity.ts:

import { twitchProvider } from './twitch';
import { googleProvider } from './google';

const providers = {
  twitch: twitchProvider as any as IdentityProvider<'twitch', UserData>,
  google: googleProvider
} satisfies Record<string, IdentityProvider<string, UserData>>;

export const getIdentityProvider = <T extends keyof typeof providers>(name: T): typeof providers[T] => providers[name];

export const getConfiguredProvider = (): typeof providers[keyof typeof providers] => {
  const name = process.env.IDENTITY_PROVIDER ?? 'twitch';
  if (Object.prototype.hasOwnProperty.call(providers, name)) return getIdentityProvider(name as any);
  throw new Error(`No such identity provider: '${name}'. Expected: ${Object.keys(providers).join(', ')}`);
}

export interface UserData {
  //Subscribder Id
  sub: string,
  preferred_username: string,
  picture?: string,
  email?: string,
}

export interface IdentityProvider<Name extends string = string, T extends UserData = UserData> {
  getUser(token: string): Promise<T>;
  getUsersById(userIds: string[]): Promise<T[]>;
  getRedirectURL(redirectURL: string, scope: string, state: string): string;
  handleRedirectResponse(code: string, state: string, redirectUri: string, grantType: string): Promise<T>;
  validateAuthServer(): Promise<Boolean>;
  provider: Name;
}

google.ts:

import { type IdentityProvider, type UserData } from "./identity";

export const googleProvider: IdentityProvider<'google', UserData & {'googleSpecificStuff': 'etc'}> = {
  getUser,
  getUsersById,
  getRedirectURL,
  handleRedirectResponse,
  validateAuthServer,

  provider,
}

Doing it this way will ensure that he functions exported by the identity provider conform to the defined interface in such a way that Typescript won't build if you've made an error. However, it also allows for specialization on top of that, as well as type discrimination:

However, it also allows for specialization: if the Google provider