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.

Sign up for GitHub

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

Ma/ticket #1131

Merged
merged 4 commits into from
Nov 28, 2024
Merged

Ma/ticket #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
906deae
fix: start refactor
markusahlstrand Nov 28, 2024
6a8ee1c
fix: use code instead of tickets
markusahlstrand Nov 28, 2024
331ee59
fix: update lock file
markusahlstrand Nov 28, 2024
ab0c786
fix: review comments
markusahlstrand Nov 28, 2024
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"lru-cache": "^11.0.2",
"nanoid": "5.0.8",
"oslo": "^1.2.1",
"playwright": "^1.44.1",
"playwright": "1.44.1",
"xml-crypto": "^6.0.0",
"xmldsigjs": "^2.6.1",
"zod": "3.23.8"
Expand Down
25 changes: 15 additions & 10 deletions src/authentication-flows/ticket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,31 +31,36 @@ export async function ticketAuth(

ctx.set("connection", realm);

const ticket = await env.data.tickets.get(tenant_id, ticketId);

if (!ticket) {
const code = await env.data.codes.get(tenant_id, ticketId, "ticket");
if (!code) {
throw new HTTPException(403, { message: "Ticket not found" });
}

const client = await getClient(ctx.env, ticket.client_id);
ctx.set("client_id", ticket.client_id);
const login = await env.data.logins.get(tenant_id, code.login_id);

if (!login || !login.authParams.username) {
markusahlstrand marked this conversation as resolved.
Show resolved Hide resolved
throw new HTTPException(403, { message: "Session not found" });
}

const client = await getClient(ctx.env, login.authParams.client_id);
ctx.set("client_id", login.authParams.client_id);

await env.data.tickets.remove(tenant_id, ticketId);
await env.data.codes.remove(tenant_id, ticketId);

const provider = getProviderFromRealm(realm);

let user = await getPrimaryUserByEmailAndProvider({
userAdapter: env.data.users,
tenant_id,
email: ticket.email,
email: login.authParams.username,
markusahlstrand marked this conversation as resolved.
Show resolved Hide resolved
provider,
});

if (!user) {
user = await env.data.users.create(tenant_id, {
user_id: `email|${userIdGenerate()}`,
email: ticket.email,
name: ticket.email,
email: login.authParams.username,
name: login.authParams.username,
provider: "email",
connection: "email",
email_verified: true,
Expand All @@ -74,7 +79,7 @@ export async function ticketAuth(
return generateAuthResponse({
ctx,
authParams: {
scope: ticket.authParams?.scope,
scope: login.authParams?.scope,
...authParams,
},
user,
Expand Down
45 changes: 25 additions & 20 deletions src/routes/oauth2/authenticate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Env, Var } from "../../types";
import { HTTPException } from "hono/http-exception";
import { getClient } from "../../services/clients";
import { loginWithPassword } from "../../authentication-flows/password";
import { Ticket } from "@authhero/adapter-interfaces";
import { Login } from "authhero";

function createUnauthorizedResponse() {
return new HTTPException(403, {
Expand Down Expand Up @@ -73,51 +73,56 @@ export const authenticateRoutes = new OpenAPIHono<{

const email = username.toLocaleLowerCase();

const ticket: Ticket = {
id: nanoid(),
tenant_id: client.tenant.id,
client_id: client.id,
email: email,
created_at: new Date(),
expires_at: new Date(Date.now() + TICKET_EXPIRATION_TIME),
};
let loginSession: Login;

if (otp) {
const code = await ctx.env.data.codes.get(client.tenant.id, otp, "otp");
if (!code) {
throw createUnauthorizedResponse();
}

const loginSession = await ctx.env.data.logins.get(
const existingLoginSession = await ctx.env.data.logins.get(
client.tenant.id,
code.login_id,
);
if (!loginSession || loginSession.authParams.username !== email) {
if (
!existingLoginSession ||
existingLoginSession.authParams.username !== email
) {
markusahlstrand marked this conversation as resolved.
Show resolved Hide resolved
throw createUnauthorizedResponse();
}

if (loginSession.authParams.username !== email) {
throw createUnauthorizedResponse();
}

// TODO: A temporary solution as the ticket doesn't have a username field. We should move the tickets over to the codes table instead
const { username, ...authParams } = loginSession.authParams;
ticket.authParams = authParams;
loginSession = existingLoginSession;
} else if (password) {
// This will throw if the login fails
await loginWithPassword(ctx, client, {
username,
password,
client_id,
});

loginSession = await ctx.env.data.logins.create(client.tenant.id, {
expires_at: new Date(
Date.now() + TICKET_EXPIRATION_TIME,
).toISOString(),
authParams: {
client_id: client.id,
username: email,
},
});
} else {
throw new HTTPException(400, { message: "Code or password required" });
}

await ctx.env.data.tickets.create(ticket);
const code = await ctx.env.data.codes.create(client.tenant.id, {
code_id: nanoid(),
code_type: "ticket",
login_id: loginSession.login_id,
expires_at: new Date(Date.now() + TICKET_EXPIRATION_TIME).toISOString(),
});
Comment on lines +117 to +122
Copy link

@coderabbitai coderabbitai bot Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update code_type to reflect the new authentication mechanism.

The code_type is set to "ticket", which may not align with the shift from tickets to codes. Consider updating code_type to "code" to accurately represent the new system and prevent potential confusion.

Apply this diff to update the code_type:

const code = await ctx.env.data.codes.create(client.tenant.id, {
  code_id: nanoid(),
- code_type: "ticket",
+ code_type: "code",
  login_id: loginSession.login_id,
  expires_at: new Date(Date.now() + TICKET_EXPIRATION_TIME).toISOString(),
});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const code = await ctx.env.data.codes.create(client.tenant.id, {
code_id: nanoid(),
code_type: "ticket",
login_id: loginSession.login_id,
expires_at: new Date(Date.now() + TICKET_EXPIRATION_TIME).toISOString(),
});
const code = await ctx.env.data.codes.create(client.tenant.id, {
code_id: nanoid(),
code_type: "code",
login_id: loginSession.login_id,
expires_at: new Date(Date.now() + TICKET_EXPIRATION_TIME).toISOString(),
});

Copy link
Author

Choose a reason for hiding this comment

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

The code type is ticket. It's a single table that can be used in different flows and in this case it's the ticket flow

Copy link

Choose a reason for hiding this comment

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

@markusahlstrand, thank you for the clarification. I understand that code_type should remain "ticket" to represent the ticket flow within the shared codes table.

✏️ Learnings added 
Learnt from: markusahlstrand
PR: sesamyab/auth#1131
File: src/routes/oauth2/authenticate.ts:121-126
Timestamp: 2024-11-28T12:14:26.935Z
Learning: In `src/routes/oauth2/authenticate.ts`, when replacing tickets with codes, the `code_type` should remain `"ticket"` to represent the ticket flow within the shared codes table, even though codes are used instead of tickets.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


return ctx.json({
login_ticket: ticket.id,
login_ticket: code.code_id,
// TODO: I guess these should be validated when accepting the ticket
co_verifier: randomString(32),
co_id: randomString(12),
Expand Down
18 changes: 9 additions & 9 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6660,17 +6660,17 @@ pkijs@^3.0.10:
pvutils "^1.1.3"
tslib "^2.6.3"

playwright-core@1.49.0:
version "1.49.0"
resolved "https://registry.yarnpkg.com/playwright-core/-/playwright-core-1.49.0.tgz#8e69ffed3f41855b854982f3632f2922c890afcb"
integrity sha512-R+3KKTQF3npy5GTiKH/T+kdhoJfJojjHESR1YEWhYuEKRVfVaxH3+4+GvXE5xyCngCxhxnykk0Vlah9v8fs3jA==
playwright-core@1.44.1:
version "1.44.1"
resolved "https://registry.yarnpkg.com/playwright-core/-/playwright-core-1.44.1.tgz#53ec975503b763af6fc1a7aa995f34bc09ff447c"
integrity sha512-wh0JWtYTrhv1+OSsLPgFzGzt67Y7BE/ZS3jEqgGBlp2ppp1ZDj8c+9IARNW4dwf1poq5MgHreEM2KV/GuR4cFA==

playwright@^1.44.1:
version "1.49.0"
resolved "https://registry.yarnpkg.com/playwright/-/playwright-1.49.0.tgz#df6b9e05423377a99658202844a294a8afb95d0a"
integrity sha512-eKpmys0UFDnfNb3vfsf8Vx2LEOtflgRebl0Im2eQQnYMA4Aqd+Zw8bEOB+7ZKvN76901mRnqdsiOGKxzVTbi7A==
playwright@1.44.1:
version "1.44.1"
resolved "https://registry.yarnpkg.com/playwright/-/playwright-1.44.1.tgz#5634369d777111c1eea9180430b7a184028e7892"
integrity sha512-qr/0UJ5CFAtloI3avF95Y0L1xQo6r3LQArLIg/z/PoGJ6xa+EwzrwO5lpNr/09STxdHuUoP2mvuELJS+hLdtgg==
dependencies:
playwright-core "1.49.0"
playwright-core "1.44.1"
optionalDependencies:
fsevents "2.3.2"

Expand Down
Loading