Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identity System Overhaul #167

Open
wants to merge 7 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions nt-web-app/entity/TwitchGetUsersResponse.ts

This file was deleted.

20 changes: 10 additions & 10 deletions nt-web-app/entity/User.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import {Entity, PrimaryColumn, Column, UpdateDateColumn, CreateDateColumn} from "typeorm"
import { Entity, PrimaryColumn, Column, UpdateDateColumn, CreateDateColumn } from "typeorm"

export type LoginProvider = 'local'|'twitch'
export type LoginProvider = string

/**
* @module DBUser
*/
@Entity({name: "user", synchronize: true})
export class User{
@Entity({ name: "user", synchronize: true })
export class User {
@PrimaryColumn()
id: string

Expand All @@ -16,7 +16,7 @@ export class User{
@Column()
display_name: string

private _access: Role|null = null;
private _access: Role | null = null;

@Column({ type: "text", nullable: false })
get access(): string {
Expand All @@ -30,8 +30,8 @@ export class User{
return '';
}

set access(value: string|null) {
if(!value){
set access(value: string | null) {
if (!value) {
this._access = new RoleImpl()
return
}
Expand All @@ -51,9 +51,9 @@ export class User{

uaccess?: number

constructor(id: string, twitch_user_name: string, access: Role, provider: LoginProvider) {
constructor(id: string, user_name: string, access: Role, provider: LoginProvider) {
this._access = access
this.display_name = twitch_user_name
this.display_name = user_name
this.id = id
this.provider = provider
this.uaccess = 0
Expand Down Expand Up @@ -109,4 +109,4 @@ export const defaultRoles = {
canListLobbies: true,
elevatedRoomPermissions: false,
isModerator: false
} as Role
} as Role
8 changes: 8 additions & 0 deletions nt-web-app/entity/identity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export interface UserData {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

//Subscribder Id
sub: string,
preferred_username: string,
picture?: string,
email?: string,
}

107 changes: 107 additions & 0 deletions nt-web-app/identity/forgejo.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/* =======
* NOTES
* =======
*
* Scope:
* This implementation should be a pretty cut and dry OIDC
* integration, very little should need to be changed to
* interface with any other standard oidc provider. It was
* developed against forgejo since it's the oidc provider
* I had available locally.
*
* Documentation Referenced In Implementation
* https://forgejo.org/docs/v1.19/user/oauth2-provider/
* https://forgejo.org/docs/v1.19/user/api-usage/
*
* Vars
* FORGEJO_API_KEY: Key for general api, needs access to read users
* FORGEJO_CLIENT_SECRET: OIDC Client Secret, generated in /user/settings/applications
* FORGEJO_CLIENT_ID: OIDC Client id, generated in /user/settings/applications
*
* FORGEJO_GETUSER_ENDPOINT: Forgejo Static API endpoint that uses general API key to get user info by id
*
* The following vars are all ripped from https://[YOUR-FORGEJO-URL]/.well-known/openid-configuration
* This likely could be used to automatically grab the appropriate url, it simply isn't in this
* implementation for convenience.
*
* FORGEJO_USERINFO_ENDPOINT
* FORGEJO_AUTHORIZATION_ENDPOINT
* FORGEJO_ACCESSTOKEN_ENDPOINT
*
*/

// =====================
// Imports and Globals
// =====================
import axios, { AxiosResponse } from "axios";
import { UserData } from "../entity/identity";

const FORGEJO_API_KEY = process.env.FORGEJO_API_KEY as string
const FORGEJO_CLIENT_SECRET = process.env.FORGEJO_CLIENT_SECRET as string
const FORGEJO_CLIENT_ID = process.env.FORGEJO_CLIENT_ID as string

const FORGEJO_USERINFO_ENDPOINT = process.env.FORGEJO_USERINFO_ENDPOINT as string
const FORGEJO_AUTHORIZATION_ENDPOINT = process.env.FORGEJO_AUTHORIZATION_ENDPOINT as string
const FORGEJO_GETUSER_ENDPOINT = process.env.FORGEJO_GETUSER_ENDPOINT as string
const FORGEJO_ACCESSTOKEN_ENDPOINT = process.env.FORGEJO_ACCESSTOKEN_ENDPOINT as string

const provider = "forgejo";

// ====================
// Exported Functions
// ====================

const getUser = async (accessToken: string): Promise<UserData> => {
return axios.get(FORGEJO_USERINFO_ENDPOINT, {
headers: {
'Authorization': `Bearer ${accessToken}`
}
})
.then((res) => res.data);
}

const getUsersById = async (userIds: string[]): Promise<UserData[] | null> => {
const userDataPromises = userIds.map((userId) =>
axios.get(`${FORGEJO_GETUSER_ENDPOINT}?access_token=${FORGEJO_API_KEY}&uid=${userId}`)
.then(resp => resp.data)
.then((data: UserData) => data)
);
return Promise.all(userDataPromises);
}

const validateAuthServer = async (): Promise<Boolean> => {
return new Promise(() => true);
}

const getRedirectURL = (redirectURL: string, scope: string, state: string): string =>
`${FORGEJO_AUTHORIZATION_ENDPOINT}?response_type=code&client_id=${FORGEJO_CLIENT_ID}&redirect_uri=${redirectURL}&scope=${scope}&state=${state}`;

const handleRedirectResponse = async (code: string, state: string, redirectUri: string, grantType: string): Promise<UserData> => {
const response = await axios.post(FORGEJO_ACCESSTOKEN_ENDPOINT, {
client_id: FORGEJO_CLIENT_ID,
client_secret: FORGEJO_CLIENT_SECRET,
code: code,
grant_type: grantType,
redirect_uri: redirectUri,
}, {
headers: {
'Content-Type': 'application/json',
},
});

const { access_token } = response.data;

const userData = await getUser(access_token)

return userData;
};

export {
getUser,
getUsersById,
getRedirectURL,
handleRedirectResponse,
validateAuthServer,

provider,
}
112 changes: 112 additions & 0 deletions nt-web-app/identity/google.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/* =======
* NOTES
* =======
*
* Scope:
* Simple plug and play usage of the google api library to run identity provider functions
* Currently lacks the ability to fetch a user by subscriber identifier, might not be possible
* with the tooling provided.
*
* Documentation Referenced In Implementation
* https://github.com/googleapis/google-api-nodejs-client?tab=readme-ov-file#oauth2-client
*
* Vars
* GOOGLE_CLIENT_SECRET: OIDC Client Secret, accessed in google developer console
* GOOGLE_CLIENT_ID: OIDC Client id, accessed in google developer console
*
*/

// =====================
// Imports and Globals
// =====================
import axios, { AxiosResponse } from "axios";
import { UserData } from "../entity/identity";
import { google } from 'googleapis';
import jwt from "jsonwebtoken";

const GOOGLE_CLIENT_SECRET = process.env.GOOGLE_CLIENT_SECRET as string
const GOOGLE_CLIENT_ID = process.env.GOOGLE_CLIENT_ID as string


const OAUTH_REDIRECT_URI = process.env.OAUTH_REDIRECT_URI as string;

const provider = "google";

const oauth2Client = new google.auth.OAuth2(
GOOGLE_CLIENT_ID,
GOOGLE_CLIENT_SECRET,
OAUTH_REDIRECT_URI + "/api/auth/code",
);

const scopes = [
'email',
'profile',

];
// ====================
//
// Exported Functions
// ====================

const getUser = async (id_token: string): Promise<UserData> => {
try {
const userData = jwt.decode(id_token, {
json: true
});

if (userData) {
return Promise.resolve({
sub: userData.sub!,
preferred_username: userData.name!,
picture: userData.picture!,
email: userData.email!,
});
}
} catch (ex) {
console.error(ex);
}
return new Promise(() => { });
}

//TODO ADAPT TO GOOGLE
const getUsersById = async (userIds: string[]): Promise<UserData[] | null> => {
return null
}

const validateAuthServer = async (): Promise<Boolean> => {
return new Promise(() => true);
}
Nicknakin marked this conversation as resolved.
Show resolved Hide resolved

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

return url;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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)



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;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


export {
getUser,
getUsersById,
getRedirectURL,
handleRedirectResponse,
validateAuthServer,

provider,
}
28 changes: 28 additions & 0 deletions nt-web-app/identity/identity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { getUser, getUsersById, getRedirectURL, handleRedirectResponse, validateAuthServer, provider } from './twitch';
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

image

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


export {
getUser,
getUsersById,
getRedirectURL,
handleRedirectResponse,
provider,
validateAuthServer,
}


/*
import { UserData } from '../entity/identity';

Potential IdentityProvider Interface to make sure identity providers are handled consistently

interface IdentityProvider {
getUser(): Promise<UserData>;
getUsersById(): Promise<UserData[]>;
getRedirectURL(): string;
handleRedirectResponse(): UserData;
validateAuthServer(): Promise<Boolean>;

provider: string;
}
*/

Loading