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

Implement user storage consent #1644

merged 22 commits into from
Jan 27, 2025

Conversation

aaschlote
Copy link
Contributor

No description provided.

@aaschlote aaschlote marked this pull request as draft January 22, 2025 14:08
@aaschlote aaschlote marked this pull request as ready for review January 24, 2025 13:41
Copy link
Member

@pgurusinga pgurusinga left a comment

Choose a reason for hiding this comment

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

I am unsure about the async action naming. what do you think?

app/services/flow/server/flowTransitionValidation.ts Outdated Show resolved Hide resolved
app/services/s3/__test__/storeConsentFgrToS3Bucket.test.ts Outdated Show resolved Hide resolved
app/services/s3/storeConsentFgrToS3Bucket.ts Outdated Show resolved Hide resolved
app/services/s3/storeConsentFgrToS3Bucket.ts Outdated Show resolved Hide resolved
app/services/s3/storeConsentFgrToS3Bucket.ts Outdated Show resolved Hide resolved
Comment on lines +18 to +22
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;
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: 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!

.replaceAll(".", "-");
};

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.

region_name="eu-central-1"
)

s3_client.create_bucket(Bucket="a2j-rechtsantragstelle-data-consent-fgr", CreateBucketConfiguration={
Copy link
Member

Choose a reason for hiding this comment

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

does the hardcoded name here matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, if I change the name, I need to change the name for the local development .env as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and btw, this file is executed on the docker container and not in your local development or app

Comment on lines +50 to +52
const asyncFlowActions = {
"/grundvoraussetzungen/datenverarbeitung": storeConsentFgrToS3Bucket,
};
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 😃

aaschlote and others added 2 commits January 27, 2025 14:40
Co-authored-by: Pram Gurusinga <pratama.gurusinga@digitalservice.bund.de>
Co-authored-by: judithmh <40442827+judithmh@users.noreply.github.com>
@aaschlote aaschlote merged commit 69058bb into main Jan 27, 2025
21 checks passed
@aaschlote aaschlote deleted the implement-user-consent-storage branch January 27, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants