Skip to content

Commit 2adba17

Browse files
committed
refactor: rename user.username to trinoUser
The field holds the Trino user we impersonate, not a login name. Rename the column to trino_user in the initial migration; there is no release yet, so no new migration is needed.
1 parent b62cfcc commit 2adba17

16 files changed

Lines changed: 60 additions & 57 deletions

File tree

‎e2e/auth/auth.spec.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,19 +48,22 @@ test.describe('Authentication', () => {
4848
await expect(page.getByRole('heading', { name: 'Dashboard' })).toBeVisible();
4949
});
5050

51-
test('username cannot be changed through the update-user endpoint', async ({ page, baseURL }) => {
51+
test('trinoUser cannot be changed through the update-user endpoint', async ({
52+
page,
53+
baseURL
54+
}) => {
5255
const res = await page.request.post('/api/auth/update-user', {
5356
headers: { origin: baseURL! },
54-
data: { username: 'someone-else' }
57+
data: { trinoUser: 'someone-else' }
5558
});
5659
expect(res.status()).toBe(403);
57-
expect(await res.json()).toMatchObject({ message: 'username is not allowed to be set' });
60+
expect(await res.json()).toMatchObject({ message: 'trinoUser is not allowed to be set' });
5861

5962
const session = await (await page.request.get('/api/auth/get-session')).json();
60-
expect(session.user.username).toMatch(/^testuser\d+$/);
63+
expect(session.user.trinoUser).toMatch(/^testuser\d+$/);
6164
});
6265

63-
test('a later login refreshes the username from the OIDC claim', async ({ browser }) => {
66+
test('a later login refreshes the Trino user from the OIDC claim', async ({ browser }) => {
6467
const context = await browser.newContext({ storageState: { cookies: [], origins: [] } });
6568
const page = await context.newPage();
6669
const sub = `relogin-${crypto.randomUUID()}`;
@@ -91,11 +94,11 @@ test.describe('Authentication', () => {
9194

9295
try {
9396
const first = await loginAs(`${sub}-old`);
94-
expect(first.username).toBe(`${sub}-old`);
97+
expect(first.trinoUser).toBe(`${sub}-old`);
9598

9699
const second = await loginAs(`${sub}-new`);
97100
expect(second.id).toBe(first.id);
98-
expect(second.username).toBe(`${sub}-new`);
101+
expect(second.trinoUser).toBe(`${sub}-new`);
99102
} finally {
100103
await context.close();
101104
}

‎e2e/support/mock-trino.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export function mockMarker(): string {
2222
}
2323

2424
/** Statements containing `marker` that reached the mock, and their cancellations. */
25-
async function mockTrinoLog(marker: string): Promise<MockTrinoLogEntry[]> {
25+
export async function mockTrinoLog(marker: string): Promise<MockTrinoLogEntry[]> {
2626
const res = await fetch(`${MOCK_TRINO_URL}/__mock/log?marker=${encodeURIComponent(marker)}`);
2727
if (!res.ok) {
2828
throw new Error(`Mock Trino log request failed: ${res.status} ${res.statusText}`);

‎e2e/trino/query-api.spec.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,10 @@ test.describe('Trino API', () => {
8787
});
8888

8989
const session = await (await page.request.get('/api/auth/get-session')).json();
90-
expect(session.user.username).toMatch(/^testuser\d+$/);
90+
expect(session.user.trinoUser).toMatch(/^testuser\d+$/);
9191
await expect
9292
.poll(async () => (await mockTrinoLog(marker)).map((e) => e.user))
93-
.toEqual([session.user.username]);
93+
.toEqual([session.user.trinoUser]);
9494
});
9595

9696
// The marker goes into a name the endpoint puts in its SQL, so the mock log
@@ -105,8 +105,8 @@ test.describe('Trino API', () => {
105105
expect(res.status()).toBe(200);
106106

107107
const session = await (await page.request.get('/api/auth/get-session')).json();
108-
expect(session.user.username).toMatch(/^testuser\d+$/);
109-
expect((await mockTrinoLog(marker)).map((e) => e.user)).toEqual([session.user.username]);
108+
expect(session.user.trinoUser).toMatch(/^testuser\d+$/);
109+
expect((await mockTrinoLog(marker)).map((e) => e.user)).toEqual([session.user.trinoUser]);
110110
});
111111
}
112112

‎src/lib/server/auth-schema.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ export const user = pgTable('user', {
1212
.defaultNow()
1313
.$onUpdate(() => /* @__PURE__ */ new Date())
1414
.notNull(),
15-
username: text('username')
15+
trinoUser: text('trino_user')
1616
});
1717

1818
export const session = pgTable(

‎src/lib/server/auth-utils.test.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ describe('getUserId', () => {
2121
describe('getTrinoUser', () => {
2222
const logger = { error: vi.fn() };
2323

24-
it('returns the username of the signed-in user', () => {
25-
const locals = { user: { id: 'u1', username: 'alice' }, logger } as unknown as App.Locals;
24+
it('returns the Trino user of the signed-in user', () => {
25+
const locals = { user: { id: 'u1', trinoUser: 'alice' }, logger } as unknown as App.Locals;
2626
expect(getTrinoUser(locals)).toBe('alice');
2727
});
2828

@@ -31,8 +31,8 @@ describe('getTrinoUser', () => {
3131
expect(getTrinoUser(locals)).toBe('anonymous');
3232
});
3333

34-
it('fails instead of falling back to "anonymous" when the username is missing', () => {
35-
const locals = { user: { id: 'u1', username: null }, logger } as unknown as App.Locals;
34+
it('fails instead of falling back to "anonymous" when the Trino user is missing', () => {
35+
const locals = { user: { id: 'u1', trinoUser: null }, logger } as unknown as App.Locals;
3636
expect(() => getTrinoUser(locals)).toThrow(expect.objectContaining({ status: 500 }));
3737
expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String));
3838
});
@@ -41,8 +41,8 @@ describe('getTrinoUser', () => {
4141
describe('findTrinoUser', () => {
4242
const logger = { error: vi.fn() };
4343

44-
it('returns null instead of "anonymous" when the username is missing', () => {
45-
const locals = { user: { id: 'u1', username: null }, logger } as unknown as App.Locals;
44+
it('returns null instead of "anonymous" when the Trino user is missing', () => {
45+
const locals = { user: { id: 'u1', trinoUser: null }, logger } as unknown as App.Locals;
4646
expect(findTrinoUser(locals)).toBeNull();
4747
expect(logger.error).toHaveBeenCalledWith({ user_id: 'u1' }, expect.any(String));
4848
});

‎src/lib/server/auth-utils.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,17 @@ export function getUserId(locals: App.Locals): string {
66

77
/**
88
* The user to impersonate in Trino. Without OIDC there is no signed-in user,
9-
* so queries run as 'anonymous'. A signed-in user without a username means
9+
* so queries run as 'anonymous'. A signed-in user without a Trino user means
1010
* sign-up lost the OIDC claim; return null instead of falling back to 'anonymous'.
1111
*/
1212
export function findTrinoUser(locals: App.Locals): string | null {
1313
if (!locals.user) return 'anonymous';
14-
if (locals.user.username) return locals.user.username;
15-
locals.logger.error({ user_id: locals.user.id }, 'Signed-in user has no Trino username');
14+
if (locals.user.trinoUser) return locals.user.trinoUser;
15+
locals.logger.error({ user_id: locals.user.id }, 'Signed-in user has no Trino user');
1616
return null;
1717
}
1818

19-
/** Like {@link findTrinoUser}, but fails the request when there is no username. */
19+
/** Like {@link findTrinoUser}, but fails the request when there is no Trino user. */
2020
export function getTrinoUser(locals: App.Locals): string {
21-
return findTrinoUser(locals) ?? error(500, 'User has no Trino username');
21+
return findTrinoUser(locals) ?? error(500, 'User has no Trino user');
2222
}

‎src/lib/server/auth.test.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,29 +8,29 @@ const create = (data: Record<string, unknown>, path: string) =>
88
const update = (data: Record<string, unknown>, path: string) =>
99
hooks.update!.before!(data as never, { path } as never);
1010

11-
describe('user.additionalFields.username', () => {
11+
describe('user.additionalFields.trinoUser', () => {
1212
it('is taken from the OIDC provider profile', () => {
1313
expect(
14-
parseAdditionalUserInputFromProviderProfile(auth.options, { username: 'alice' }, 'create')
15-
).toMatchObject({ username: 'alice' });
14+
parseAdditionalUserInputFromProviderProfile(auth.options, { trinoUser: 'alice' }, 'create')
15+
).toMatchObject({ trinoUser: 'alice' });
1616
});
1717

1818
it('may be written by the OIDC callback', async () => {
1919
await expect(
20-
create({ email: 'alice@example.com', username: 'alice' }, '/oauth2/callback/:providerId')
20+
create({ email: 'alice@example.com', trinoUser: 'alice' }, '/oauth2/callback/:providerId')
2121
).resolves.toBeUndefined();
2222
});
2323

2424
it('rejects a rewrite through the built-in update-user endpoint', async () => {
25-
await expect(update({ username: 'someone-else' }, '/update-user')).rejects.toThrow(
26-
/username is not allowed to be set/
25+
await expect(update({ trinoUser: 'someone-else' }, '/update-user')).rejects.toThrow(
26+
/trinoUser is not allowed to be set/
2727
);
2828
});
2929

3030
it('rejects a rewrite smuggled in at sign-up', async () => {
3131
await expect(
32-
create({ email: 'eve@example.com', username: 'someone-else' }, '/sign-up/email')
33-
).rejects.toThrow(/username is not allowed to be set/);
32+
create({ email: 'eve@example.com', trinoUser: 'someone-else' }, '/sign-up/email')
33+
).rejects.toThrow(/trinoUser is not allowed to be set/);
3434
});
3535

3636
it('leaves other user updates alone', async () => {

‎src/lib/server/auth.ts‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,16 @@ const env = envModule?.env ?? (process.env as Record<string, string | undefined>
1616
const getRequestEvent = appServer?.getRequestEvent ?? (() => undefined as unknown as RequestEvent);
1717
const log = loggingModule?.logger.child({ module: 'auth' });
1818

19-
// The username is the Trino user we impersonate, so only the OIDC callback may
19+
// trinoUser is the Trino user we impersonate, so only the OIDC callback may
2020
// write it. Marking the field `input: false` would also block that: better-auth
2121
// drops such fields from the provider profile on sign-up (see #371). Database
2222
// hooks see the endpoint's route template, not the request path.
23-
const usernameWritePath = '/oauth2/callback/:providerId';
23+
const trinoUserWritePath = '/oauth2/callback/:providerId';
2424

25-
function guardUsernameWrite(data: Record<string, unknown>, context: { path?: string } | null) {
26-
if (!('username' in data) || context?.path === usernameWritePath) return;
27-
log?.warn({ path: context?.path }, 'Rejected username write outside the OIDC callback');
28-
throw new APIError('FORBIDDEN', { message: 'username is not allowed to be set' });
25+
function guardTrinoUserWrite(data: Record<string, unknown>, context: { path?: string } | null) {
26+
if (!('trinoUser' in data) || context?.path === trinoUserWritePath) return;
27+
log?.warn({ path: context?.path }, 'Rejected trinoUser write outside the OIDC callback');
28+
throw new APIError('FORBIDDEN', { message: 'trinoUser is not allowed to be set' });
2929
}
3030

3131
const trinoUserClaim = env.STACKABLE_COCKPIT_TRINO_USER_CLAIM ?? 'preferred_username';
@@ -68,16 +68,16 @@ export const auth = betterAuth({
6868
},
6969
user: {
7070
additionalFields: {
71-
username: {
71+
trinoUser: {
7272
type: 'string',
7373
required: false
7474
}
7575
}
7676
},
7777
databaseHooks: {
7878
user: {
79-
create: { before: async (user, context) => guardUsernameWrite(user, context) },
80-
update: { before: async (user, context) => guardUsernameWrite(user, context) }
79+
create: { before: async (user, context) => guardTrinoUserWrite(user, context) },
80+
update: { before: async (user, context) => guardTrinoUserWrite(user, context) }
8181
}
8282
},
8383
plugins: [
@@ -101,8 +101,8 @@ export const auth = betterAuth({
101101
const fullName = [profile.given_name, profile.family_name]
102102
.filter(Boolean)
103103
.join(' ');
104-
const trinoUser = profile[trinoUserClaim];
105-
if (trinoUser) {
104+
const claimedTrinoUser = profile[trinoUserClaim];
105+
if (claimedTrinoUser) {
106106
log?.debug(
107107
{ trino_user_claim: trinoUserClaim },
108108
'Resolved Trino user from OIDC claim'
@@ -119,8 +119,8 @@ export const auth = betterAuth({
119119
emailVerified: profile.email_verified ?? false,
120120
image: profile.picture || null,
121121
// No `|| ''`: without an email better-auth rejects the sign-in before
122-
// creating the user, so this never stores an empty username.
123-
username: trinoUser || profile.preferred_username || profile.email
122+
// creating the user, so this never stores an empty Trino user.
123+
trinoUser: claimedTrinoUser || profile.preferred_username || profile.email
124124
};
125125
}
126126
}

‎src/lib/server/migrations/0000_initial_schema.sql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ CREATE TABLE "user" (
4747
"image" text,
4848
"created_at" timestamp DEFAULT now() NOT NULL,
4949
"updated_at" timestamp DEFAULT now() NOT NULL,
50-
"username" text,
50+
"trino_user" text,
5151
CONSTRAINT "user_email_unique" UNIQUE("email")
5252
);
5353
--> statement-breakpoint

‎src/lib/server/migrations/meta/0000_snapshot.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,8 +358,8 @@
358358
"notNull": true,
359359
"default": "now()"
360360
},
361-
"username": {
362-
"name": "username",
361+
"trino_user": {
362+
"name": "trino_user",
363363
"type": "text",
364364
"primaryKey": false,
365365
"notNull": false

0 commit comments

Comments
 (0)