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

Implement user storage consent #1644

Merged
merged 22 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ad9aa63
feat: first version to store in s3 bucket
aaschlote Jan 23, 2025
be526b8
Merge branch 'main' into implement-user-consent-storage
aaschlote Jan 23, 2025
4e83535
feat(getSessionIdByFlowId): create function to retrieve session id
aaschlote Jan 23, 2025
698ae0b
refactor: set AWS S3 endpoint
aaschlote Jan 23, 2025
a6fff85
refactor(storeConsentToBucket): store correct file and data to s3 bucket
aaschlote Jan 23, 2025
d4adfef
Merge branch 'main' into implement-user-consent-storage
aaschlote Jan 23, 2025
62b687b
Merge branch 'main' into implement-user-consent-storage
aaschlote Jan 23, 2025
9d14d99
feat(docker-compose): install locally localstack s3 bucket
aaschlote Jan 23, 2025
970cc41
feat: store consent when on the specific step id
aaschlote Jan 24, 2025
7393cab
test(createClientDataConsentFgr): add tests
aaschlote Jan 24, 2025
3ca2291
test(storeConsentFgrToS3Bucket): add tests
aaschlote Jan 24, 2025
61a349d
Merge branch 'main' into implement-user-consent-storage
aaschlote Jan 24, 2025
75c4399
refactor(executeAsyncFlowActionByStepId): create function and call in…
aaschlote Jan 24, 2025
a0de057
test(executeAsyncFlowActionByStepId): add tests
aaschlote Jan 24, 2025
648cca7
refactor: rename folder
aaschlote Jan 24, 2025
4a9f389
refactor(storeConsentFgrToS3Bucket): use toGermanDateFormat to create…
aaschlote Jan 24, 2025
fcfeb9c
refactor(storeConsentFgrToS3Bucket): simplify function parameters
aaschlote Jan 24, 2025
4ae4c23
refactor(executeAsyncFlowActionByStepId): move function to correct file
aaschlote Jan 24, 2025
f658274
test: move tests to correct path
aaschlote Jan 24, 2025
c144b7b
Merge branch 'main' into implement-user-consent-storage
aaschlote Jan 27, 2025
c6a392b
Merge branch 'main' into implement-user-consent-storage
aaschlote Jan 27, 2025
d4547cd
refactor: set environment variables to app
aaschlote Jan 27, 2025
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
1 change: 1 addition & 0 deletions app/domains/flows.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export type Flow = {
migration?: FlowMigration;
flowTransitionConfig?: FlowTransitionConfig;
stringReplacements?: (context: Context) => Replacements;
asyncFlowActions?: Record<string, (request: Request) => Promise<void>>;
};

export const flows = {
Expand Down
6 changes: 6 additions & 0 deletions app/domains/fluggastrechte/formular/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { AllContextKeys } from "~/domains/common";
import type { Flow } from "~/domains/flows.server";
import type { ArrayConfigServer } from "~/services/array";
import type { FlowTransitionConfig } from "~/services/flow/server/flowTransitionValidation";
import { storeConsentFgrToS3Bucket } from "~/services/s3/storeConsentFgrToS3Bucket";
import abgabeFlow from "./abgabe/flow.json";
import type { FluggastrechtContext } from "./context";
import { flugdatenDone } from "./flugdaten/doneFunctions";
Expand Down Expand Up @@ -46,6 +47,10 @@ const flowTransitionConfig: FlowTransitionConfig = {
eligibleSourcePages: ["/ergebnis/erfolg"],
};

const asyncFlowActions = {
"/grundvoraussetzungen/datenverarbeitung": storeConsentFgrToS3Bucket,
};
Comment on lines +50 to +52
Copy link
Member

Choose a reason for hiding this comment

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

one alternative I just thought of: adding it in the flow config, specifically under meta of that step. not sure whether that is more or less confusing 😅

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 would prefer to have here for now, but we can move to the steps config from this PR once it's implemented 😃


export const fluggastrechtFlow = {
flowType: "formFlow",
migration: {
Expand Down Expand Up @@ -142,4 +147,5 @@ export const fluggastrechtFlow = {
},
guards: fluggastrechteGuards,
flowTransitionConfig,
asyncFlowActions,
} satisfies Flow;
3 changes: 3 additions & 0 deletions app/routes/shared/formular.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { buildFlowController } from "~/services/flow/server/buildFlowController"
import {
validateFlowTransition,
getFlowTransitionConfig,
executeAsyncFlowActionByStepId,
} from "~/services/flow/server/flowTransitionValidation";
import { insertIndexesIntoPath } from "~/services/flow/stepIdConverter";
import { navItemsFromStepStates } from "~/services/flowNavigation.server";
Expand Down Expand Up @@ -282,6 +283,8 @@ export const action = async ({ request }: ActionFunctionArgs) => {
});
}

await executeAsyncFlowActionByStepId(flows[flowId], stepId, request);

const destination =
flowController.getNext(stepId) ?? flowController.getInitial();

Expand Down
13 changes: 13 additions & 0 deletions app/services/env/env.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ type Config = {
SAML_SP_METADATA_PATH: string;
SAML_SP_SECRET_KEY_PATH: string;
SAML_IDP_CERT?: string;
AWS_S3_REGION: string;
AWS_S3_ENDPOINT: string;
AWS_S3_DATA_CONSENT_FGR_ACCESS_KEY: string;
AWS_S3_DATA_CONSENT_FGR_SECRET_KEY: string;
AWS_S3_DATA_CONSENT_FGR_BUCKET_NAME: string;
Comment on lines +18 to +22
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should drop the AWS_ prefix here (and everywhere throughout)? 😅

Copy link
Member

Choose a reason for hiding this comment

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

I think I understand why Alex did this, based on my experience with the aws cli. This is how AWS typically shows examples for environment variables. The CLI tools also accept environment variables in this format.

reference: https://docs.aws.amazon.com/cli/v1/userguide/cli-configure-envvars.html

};

let instance: Config | undefined = undefined;
Expand Down Expand Up @@ -47,6 +52,14 @@ export function config(): Config {
process.env.SAML_SP_SECRET_KEY_PATH?.trim() ??
path.join(process.cwd(), "data/saml/sp_privateKey.pem"),
SAML_IDP_CERT: process.env.SAML_IDP_CERT?.trim(),
AWS_S3_REGION: process.env.AWS_S3_REGION?.trim() ?? "",
AWS_S3_ENDPOINT: process.env.AWS_S3_ENDPOINT?.trim() ?? "",
AWS_S3_DATA_CONSENT_FGR_ACCESS_KEY:
process.env.AWS_S3_DATA_CONSENT_FGR_ACCESS_KEY?.trim() ?? "",
AWS_S3_DATA_CONSENT_FGR_SECRET_KEY:
process.env.AWS_S3_DATA_CONSENT_FGR_SECRET_KEY?.trim() ?? "",
AWS_S3_DATA_CONSENT_FGR_BUCKET_NAME:
process.env.AWS_S3_DATA_CONSENT_FGR_BUCKET_NAME?.trim() ?? "",
};
}

Expand Down
38 changes: 38 additions & 0 deletions app/services/flow/server/__test__/flowTransitionValidation.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import type { Flow } from "~/domains/flows.server";
import { executeAsyncFlowActionByStepId } from "../flowTransitionValidation";

describe("flowTransitionValidation", () => {
describe("executeAsyncFlowActionByStepId", () => {
const mockRequest = new Request("http://localhost");
const mockAsyncFlowAction = vi.fn().mockResolvedValue(undefined);

it("should execute the async flow action for the given stepId", async () => {
const mockAsyncFlowAction = vi.fn().mockResolvedValue(undefined);
const mockFlow: Flow = {
asyncFlowActions: {
"test-step": mockAsyncFlowAction,
},
} as unknown as Flow;

await executeAsyncFlowActionByStepId(mockFlow, "test-step", mockRequest);

expect(mockAsyncFlowAction).toHaveBeenCalledWith(mockRequest);
});

it("should not execute the async flow action for the given another stepId", async () => {
const mockFlow: Flow = {
asyncFlowActions: {
"test-step": mockAsyncFlowAction,
},
} as unknown as Flow;

await executeAsyncFlowActionByStepId(
mockFlow,
"another-step",
mockRequest,
);

expect(mockAsyncFlowAction).not.toHaveBeenCalledWith(mockRequest);
});
});
});
18 changes: 18 additions & 0 deletions app/services/flow/server/flowTransitionValidation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,24 @@ export function getFlowTransitionConfig(currentFlow: Flow) {
: undefined;
}

function getAsyncFlowActions(currentFlow: Flow) {
aaschlote marked this conversation as resolved.
Show resolved Hide resolved
return "asyncFlowActions" in currentFlow
? currentFlow.asyncFlowActions
: undefined;
}

export async function executeAsyncFlowActionByStepId(
currentFlow: Flow,
stepId: string,
request: Request,
) {
const asyncFlowActions = getAsyncFlowActions(currentFlow);

if (asyncFlowActions?.[stepId]) {
await asyncFlowActions[stepId](request);
}
}

export async function validateFlowTransition(
flows: Record<FlowId, Flow>,
cookieHeader: CookieHeader,
Expand Down
46 changes: 46 additions & 0 deletions app/services/s3/__test__/createClientDataConsentFgr.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { S3Client } from "@aws-sdk/client-s3";
import { describe, it, expect, vi, beforeEach } from "vitest";
import { config } from "~/services/env/env.server";
import { createClientDataConsentFgr } from "~/services/s3/createClientDataConsentFgr";

vi.mock("@aws-sdk/client-s3", () => ({
S3Client: vi.fn(),
}));

vi.mock("~/services/env/env.server", () => ({
config: vi.fn(),
}));

describe("createClientDataConsentFgr", () => {
beforeEach(() => {
vi.clearAllMocks();
});

it("should create a new S3Client instance if not already created only once", () => {
const mockConfig = {
...config(),
AWS_S3_DATA_CONSENT_FGR_ACCESS_KEY: "test-access-key",
AWS_S3_DATA_CONSENT_FGR_SECRET_KEY: "test-secret-key",
AWS_S3_REGION: "test-region",
AWS_S3_ENDPOINT: "test-endpoint",
};

vi.mocked(config).mockReturnValue(mockConfig);

const firstClient = createClientDataConsentFgr();

expect(S3Client).toHaveBeenCalledWith({
region: mockConfig.AWS_S3_REGION,
credentials: {
accessKeyId: mockConfig.AWS_S3_DATA_CONSENT_FGR_ACCESS_KEY,
secretAccessKey: mockConfig.AWS_S3_DATA_CONSENT_FGR_SECRET_KEY,
},
endpoint: mockConfig.AWS_S3_ENDPOINT,
});
expect(firstClient).toBeInstanceOf(S3Client);

const secondClient = createClientDataConsentFgr();
expect(S3Client).toHaveBeenCalledTimes(1);
expect(firstClient).toBe(secondClient);
});
});
96 changes: 96 additions & 0 deletions app/services/s3/__test__/storeConsentFgrToS3Bucket.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import { PutObjectCommand, S3Client } from "@aws-sdk/client-s3";
import { describe, it, expect, vi } from "vitest";
import { config } from "~/services/env/env.server";
import { sendSentryMessage } from "../../logging";
import { getSessionIdByFlowId } from "../../session.server";
import { createClientDataConsentFgr } from "../createClientDataConsentFgr";
import { storeConsentFgrToS3Bucket } from "../storeConsentFgrToS3Bucket";

vi.mock("@aws-sdk/client-s3", () => ({
PutObjectCommand: vi.fn(),
}));

vi.mock("../createClientDataConsentFgr", () => ({
createClientDataConsentFgr: vi.fn(),
}));

vi.mock("../../logging", () => ({
sendSentryMessage: vi.fn(),
}));

vi.mock("../../session.server", () => ({
getSessionIdByFlowId: vi.fn(),
}));

vi.mock("~/services/env/env.server", () => ({
config: vi.fn(),
}));

const mockS3Client = { send: vi.fn() } as unknown as S3Client;

const mockRequest = new Request("http://localhost", {
headers: {
Cookie: "test-cookie",
"user-agent": "test-agent",
},
});

beforeEach(() => {
vi.clearAllMocks();
vi.useFakeTimers();
});

afterEach(() => {
vi.useRealTimers();
});
aaschlote marked this conversation as resolved.
Show resolved Hide resolved

describe("storeConsentFgrToS3Bucket", () => {
it("should store consent data to S3 bucket", async () => {
const mockDate = new Date("2025-01-01");
vi.setSystemTime(mockDate);

const mockSessionId = "test-session-id";
const mockConfig = {
...config(),
AWS_S3_DATA_CONSENT_FGR_BUCKET_NAME: "test-bucket",
};
vi.mocked(createClientDataConsentFgr).mockReturnValue(mockS3Client);
vi.mocked(getSessionIdByFlowId).mockResolvedValue(mockSessionId);
vi.mocked(config).mockReturnValue(mockConfig);
const mockBuffer = Buffer.from(
`${mockSessionId};${mockDate.getTime()};test-agent`,
"utf8",
);
const mockKey = "01-01-2025/test-session-id.csv";

await storeConsentFgrToS3Bucket(mockRequest);

expect(createClientDataConsentFgr).toHaveBeenCalled();
expect(getSessionIdByFlowId).toHaveBeenCalledWith(
"/fluggastrechte/formular",
"test-cookie",
);
expect(mockS3Client.send).toHaveBeenCalledWith(
new PutObjectCommand({
Bucket: mockConfig.AWS_S3_DATA_CONSENT_FGR_BUCKET_NAME,
Body: mockBuffer,
Key: mockKey,
}),
);
});

it("should send a Sentry message on error", async () => {
const mockError = new Error("Test error");

vi.mocked(createClientDataConsentFgr).mockImplementation(() => {
throw mockError;
});

await storeConsentFgrToS3Bucket(mockRequest);

expect(sendSentryMessage).toHaveBeenCalledWith(
`Error storing consent fgr data to S3 bucket: ${mockError.message}`,
"error",
);
});
});
21 changes: 21 additions & 0 deletions app/services/s3/createClientDataConsentFgr.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { S3Client } from "@aws-sdk/client-s3";
import { config } from "~/services/env/env.server";

let instance: S3Client | undefined = undefined;

export const createClientDataConsentFgr = () => {
Copy link
Member

Choose a reason for hiding this comment

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

I would (strongly) vote for having a shared storage service, that we can then scope by app service (consent, file-upload, ...).
So this would be a generic s3 instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be discussed in the Dev Exchange next week!

if (typeof instance === "undefined") {
const credentials = {
accessKeyId: config().AWS_S3_DATA_CONSENT_FGR_ACCESS_KEY,
secretAccessKey: config().AWS_S3_DATA_CONSENT_FGR_SECRET_KEY,
};

instance = new S3Client({
region: config().AWS_S3_REGION,
credentials,
endpoint: config().AWS_S3_ENDPOINT,
});
}

return instance;
};
51 changes: 51 additions & 0 deletions app/services/s3/storeConsentFgrToS3Bucket.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { PutObjectCommand } from "@aws-sdk/client-s3";
aaschlote marked this conversation as resolved.
Show resolved Hide resolved
import { config } from "~/services/env/env.server";
import { today } from "~/util/date";
import { createClientDataConsentFgr } from "./createClientDataConsentFgr";
import { sendSentryMessage } from "../logging";
import { getSessionIdByFlowId } from "../session.server";

const createConsentDataBuffer = (sessionId: string, request: Request) => {
aaschlote marked this conversation as resolved.
Show resolved Hide resolved
const userAgent = request.headers.get("user-agent");
return Buffer.from(`${sessionId};${Date.now()};${userAgent}`, "utf8");
};

const getFolderDate = () => {
return today()
.toLocaleDateString("de-DE", {
day: "2-digit",
month: "2-digit",
year: "numeric",
})
.replaceAll(".", "-");
};
aaschlote marked this conversation as resolved.
Show resolved Hide resolved

export const storeConsentFgrToS3Bucket = async (request: Request) => {
Copy link
Member

Choose a reason for hiding this comment

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

if we use a shared storage service, this could be moved into its own consent service and act as the main interface to the application

Copy link
Member

Choose a reason for hiding this comment

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

also, I think this function only needs the request.headers, not the whole object? (just to simplify the interface)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the first comment, this is file is related store something in the S3 bucket. As we already discussed, we renamed the folder to be externalDataStorage.

For the second comment, I can change, but in case a new flow action is added and needs another data from the request (body, formData, referrer or etc), all the functions must be changed again.

try {
const s3Client = createClientDataConsentFgr();
const cookieHeader = request.headers.get("Cookie");
const sessionId = await getSessionIdByFlowId(
"/fluggastrechte/formular",
cookieHeader,
);

const buffer = createConsentDataBuffer(sessionId, request);
const key = `${getFolderDate()}/${sessionId}.csv`;

await s3Client.send(
new PutObjectCommand({
Bucket: config().AWS_S3_DATA_CONSENT_FGR_BUCKET_NAME,
Body: buffer,
Key: key,
}),
);
} catch (error) {
const errorDescription =
error instanceof Error ? error.message : "Unknown error";

sendSentryMessage(
`Error storing consent fgr data to S3 bucket: ${errorDescription}`,
"error",
);
}
};
9 changes: 9 additions & 0 deletions app/services/session.server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,15 @@ export const updateSession = (session: Session, validatedData: Context) => {
});
};

export const getSessionIdByFlowId = async (
flowId: FlowId,
cookieHeader: CookieHeader,
) => {
const contextSession = getSessionManager(flowId);
const { id } = await contextSession.getSession(cookieHeader);
return id;
};

export type CookieHeader = string | null | undefined;
export const mainSessionFromCookieHeader = async (cookieHeader: CookieHeader) =>
getSessionManager("main").getSession(cookieHeader);
12 changes: 12 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,15 @@ services:
--tls-cert-file /usr/local/etc/redis/redis-server.crt
--tls-key-file /usr/local/etc/redis/redis-server.key
--tls-auth-clients no

s3-bucket:
container_name: local-s3-bucket
image: localstack/localstack:s3-latest
environment:
- DEBUG=1
ports:
- "4566:4566"
volumes:
- "./init-s3-bucket.py:/etc/localstack/init/ready.d/init-s3-bucket.py"
- "${LOCALSTACK_VOLUME_DIR:-./volume}:/var/lib/localstack"
- "/var/run/docker.sock:/var/run/docker.sock"
Loading
Loading