[#228] Implement Privacy Center client app #239

elliotbonneville · 2022-03-01T01:09:46Z

Purpose

Implements Next.js / React Privacy Center client app based on the fidesui-template repo. Demo here.

Note: because there is no hosted fidesops server any requests made through this UI will fail.

Changes

Creates clients directory at repo root
Implements Next.js client app
Implements testing strategy for Next app

Checklist

Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram)
Good unit test/integration test coverage
This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Described by #228

Adds query execution (access and erasure) support for BigQuery.

…time.utcnow() in the API view

seanpreston · 2022-03-09T20:41:19Z

clients/privacy-center/.env.development

@@ -0,0 +1 @@
+NEXT_PUBLIC_AUTH_ENDPOINT=http://0.0.0.0:8080/api/v1/oauth/token


It'd be good to pass this URL down through config.json, so anyone hosting this has the option to connect it to a different server should they wish.

Yes, this is the point I mentioned on our call the other day – we need to decide where we want environment variables to live for this specific project, because it has an additional config entry point in config.json. Either we should specify variables in config.json or in a .env file, but not both.

My thought is that we should just remove this file – what do you think?

Agree, I think all the config is best off in one place

seanpreston · 2022-03-10T17:02:35Z

clients/privacy-center/README.md

+To serve this application locally, run:
+
+```bash
+npm run dev


I believe this assumes that next is already installed, we should add explicit instructions to npm install next if it's not already installed

It doesn't need to be installed globally – it's a local dependency, and NPM scripts can pick up on that.

FWIW I'm only making that comment as when I ran npm run dev for the first time on this fork it complained next couldn't be found. I'm not sure the local deps are auto-installing in that case?

UPDATE: Maybe it's npm install we need to be adding here instead?

Yep, I've added that command to the readme.

seanpreston · 2022-03-10T18:17:48Z

clients/privacy-center/README.md

+To run the interactive test interface, run:
+
+```bash
+npm run test


Interesting that this is an interactive interface — what's the command here to automatically run all tests? We'll want to add this to the CI jobs too.

Great point. I'll add a command for it.

(It's simply jest, instead of jest --watch)

seanpreston · 2022-03-10T19:15:27Z

clients/privacy-center/components/RequestModal.tsx

+
+      onClose();
+    },
+    validate: (values) => {


What are your thoughts on invoking this function on form submit, rather than on unfocus of an input, as this results in weird looking scenarios like:

Form validation on field blur is a fairly standard pattern, and in fact it's the standard one that Formik (the form library we're using) recommends. I suspect this only looks strange because you're deliberately testing the input – most people will enter something before moving on to the next field. :)

If you'd prefer to validate the form on submission – and keep the "Continue" button active initially instead of having it become active when the form is valid – I can implement that. It would require reworking some of the other logic as well, but shouldn't be a problem. That's also a fairly common pattern.

That's totally fair, let's leave it

seanpreston · 2022-03-10T19:44:19Z

clients/privacy-center/components/RequestModal.tsx

+        errors.email = 'Required';
+      } else if (
+        values.email &&
+        !/^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}$/i.test(values.email)


I'd like for us to get better at documenting regex's in general. This is obviously validating an email address but can we add a comment here breaking down exactly what'll match and what won't? Then if we found this anywhere, a link to the original source.

It looks as though the regex here might not cover all the cases we need too, for example we're not including the tilde ~ character in the set of valid chars.

I understand this particular topic is hard to pin down, Wikipedia's resource looks well informed: https://en.wikipedia.org/wiki/Email_address#Local-part_normalization

I suspect it might be better here to err on the side of letting through emails that might be invalid, since the worst case scenario is that it gets run through Fidesops as a privacy request and if the email is invalid then nothing will be found.

OWASP's input validation cheatsheet has a pretty good section on email address validation which might be helpful here, @seanpreston?

Link: https://cheatsheetseries.owasp.org/cheatsheets/Input_Validation_Cheat_Sheet.html#email-address-validation

Basically, it's surprisingly very difficult to do it right and the extended validation should be offloaded to a mail server if possible, but some basic initial validation could look like this:

The email address contains two parts, separated with an @ symbol.

The email address does not contain dangerous characters (such as backticks, single or double quotes, or null bytes).

Exactly which characters are dangerous will depend on how the address is going to be used (echoed in page, inserted into database, etc).

The domain part contains only letters, numbers, hyphens (-) and periods (.).

The email address is a reasonable length:

The local part (before the @) should be no more than 63 characters.

The total length should be no more than 254 characters.

I've been baited into this conversation by @seanpreston asking for strong opinions

My strong opinion is that we shouldn't try to do this kind of validation ourselves; <input type="email"> exists and the browsers handle it...

Hmmmm. Front-end validation is trivial to bypass anyway.

@seanpreston pydantic has an "EmailStr" type that can do this validation.

from pydantic import EmailStr EmailStr.validate("foo") # Throws an error EmailStr.validate('"><script>alert(1);</script>"@example.org') # Throws an error EmailStr.validate("[email protected]") # Valid EmailStr.validate("[email protected]") # Valid

For context, the regex in this PR comes from the Formik docs (Formik is an extremely popular React form management library).

Given @Neville's opinion, I'll remove email validation for now, and we can add it back in a future PR if needed. FWIW, I think he's on point here. Also, if additional validation is required by a consumer, it's not complicated to add.

seanpreston · 2022-03-10T19:53:24Z

clients/privacy-center/pages/404.tsx

+import NextLink from 'next/link';
+import { Stack, Heading, Box, Text, Button } from '@fidesui/react';
+
+const Custom404 = () => (


Great attention to detail @elliotbonneville — I don't think this was in the initial spec was it?

Came in from the app template!

seanpreston · 2022-03-10T19:55:38Z

clients/privacy-center/pages/404.tsx

+          </Stack>
+        </Box>
+        <Box display={['none', 'none', 'inherit']}>
+          <Image


Should we make this an anchor link back to the homepage too?

Sure, good call. Added.

seanpreston · 2022-03-10T19:59:20Z

clients/privacy-center/pages/index.tsx

+              <CloseButton
+                position="absolute"
+                right="8px"
+                onClick={() => setAlert(null)}


Again, not necessary for this PR, since we're hiding UI elements (in this case the header) with this alert, we should have it fade out or auto close after a certain time

Will leave it for now but that's a great suggestions.

seanpreston · 2022-03-10T20:01:06Z

clients/privacy-center/components/RequestModal.tsx

+      }
+
+      if (!values.phone && action.identity_inputs.phone === 'required') {
+        errors.phone = 'Required';


We'll have to come up with a way to validate the phone number if supplied too, but since they're optional for now it's OK to leave this to subsequent release I think

seanpreston

Fantastic work so far @elliotbonneville. Couple of comments to address still but we're tantalisingly close!

- Remove .env.development file as config lives in `config/config.json` - Update docs to include instructions to `npm install` - Add `test:ci` command - Remove email validation from form - Add anchor logo on 404 page

seanpreston · 2022-03-14T20:27:18Z

This branch is a fork, so cannot access the secrets, hence the unsafe checks are failing

seanpreston · 2022-03-14T20:31:16Z

clients/privacy-center/config/config.json

+{
+  "title": "Take control of your data",
+  "description": "When you use our services, you’re trusting us with your information. We understand this is a big responsibility and work hard to protect your information and put you in control.",
+  "fidesops_host_development": "http://localhost:8080/api/v1",


One last thing @elliotbonneville — I think we wanted to add the auth URL here?

Specifically wrt this change

* Implements initial privacy center layout * Implement privacy request modal * 80 Query Execution BigQuery (#215) Adds query execution (access and erasure) support for BigQuery. * makes client_id optional on the PrivacyRequest model * makes PrivacyRequestCreate.requested_at optional, and default to datetime.utcnow() in the API view * remove authentication requirements from privacy request create tests * Update request format slightly * Switch to babel for compilation to prevent swc build error * Implement alerts * Implement optionally required fields * Move types out of ignored lib folder * Commit updated files referring to types * Tweak styles to match Figma designs * Correct action card layout for all numbers of cards * Correct logo placement behind alerts * Correct merge changes * Implement testing * Document functionality in README * Correct README header * Correct deployment instructions * Move test setup file to __tests__ folder * simplify initial config * Address PR comments - Remove .env.development file as config lives in `config/config.json` - Update docs to include instructions to `npm install` - Add `test:ci` command - Remove email validation from form - Add anchor logo on 404 page Co-authored-by: Catherine Smith <[email protected]> Co-authored-by: Sean Preston <[email protected]>

elliotbonneville and others added 8 commits February 28, 2022 14:56

Implements initial privacy center layout

a6893a1

Implement privacy request modal

fd23999

80 Query Execution BigQuery (ethyca#215)

b18e99a

Adds query execution (access and erasure) support for BigQuery.

makes client_id optional on the PrivacyRequest model

e1f9473

makes PrivacyRequestCreate.requested_at optional, and default to date…

51604b8

…time.utcnow() in the API view

remove authentication requirements from privacy request create tests

ac25374

Update request format slightly

7f342c8

Switch to babel for compilation to prevent swc build error

7ce47d0

seanpreston added the DON'T MERGE label Mar 2, 2022

elliotbonneville added 7 commits March 2, 2022 14:09

Implement alerts

c784864

Implement optionally required fields

34a6532

Move types out of ignored lib folder

d6f98a1

Commit updated files referring to types

8e55a2b

Tweak styles to match Figma designs

466c0e5

Correct action card layout for all numbers of cards

0b38167

Correct logo placement behind alerts

74b29d5

seanpreston changed the title ~~[wip] Implement Privacy Center client app~~ [#228] Implement Privacy Center client app Mar 3, 2022

elliotbonneville added 7 commits March 7, 2022 11:59

Merge branch 'main' of github.com:ethyca/fidesops

298be90

Correct merge changes

0114a48

Implement testing

8f04ea3

Document functionality in README

5807874

Correct README header

6f026d5

Correct deployment instructions

e443dd5

Move test setup file to __tests__ folder

20f9acc

seanpreston self-assigned this Mar 9, 2022

seanpreston reviewed Mar 9, 2022

View reviewed changes

seanpreston removed the DON'T MERGE label Mar 10, 2022

seanpreston reviewed Mar 10, 2022

View reviewed changes

simplify initial config

224c550

seanpreston reviewed Mar 10, 2022

View reviewed changes

seanpreston suggested changes Mar 10, 2022

View reviewed changes

elliotbonneville added 2 commits March 14, 2022 12:59

Address PR comments

30d2013

- Remove .env.development file as config lives in `config/config.json` - Update docs to include instructions to `npm install` - Add `test:ci` command - Remove email validation from form - Add anchor logo on 404 page

Merge branch 'main' of github.com:elliotbonneville/fidesops

d59327d

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Mar 14, 2022

seanpreston approved these changes Mar 14, 2022

View reviewed changes

seanpreston reviewed Mar 14, 2022

View reviewed changes

seanpreston merged commit 78f3ab3 into ethyca:main Mar 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#228] Implement Privacy Center client app #239

[#228] Implement Privacy Center client app #239

elliotbonneville commented Mar 1, 2022 •

edited

Loading

seanpreston Mar 9, 2022 •

edited

Loading

elliotbonneville Mar 10, 2022

seanpreston Mar 10, 2022

seanpreston Mar 10, 2022

elliotbonneville Mar 10, 2022

seanpreston Mar 10, 2022 •

edited

Loading

elliotbonneville Mar 14, 2022

seanpreston Mar 10, 2022

elliotbonneville Mar 10, 2022

elliotbonneville Mar 10, 2022

seanpreston Mar 10, 2022

elliotbonneville Mar 14, 2022

seanpreston Mar 14, 2022

seanpreston Mar 10, 2022

daveqnet Mar 10, 2022

NevilleS Mar 10, 2022

daveqnet Mar 10, 2022

ThomasLaPiana Mar 10, 2022 •

edited

Loading

elliotbonneville Mar 14, 2022 •

edited

Loading

seanpreston Mar 10, 2022

elliotbonneville Mar 14, 2022

seanpreston Mar 10, 2022

elliotbonneville Mar 14, 2022

seanpreston Mar 10, 2022

elliotbonneville Mar 14, 2022

seanpreston Mar 10, 2022

seanpreston left a comment

seanpreston commented Mar 14, 2022

seanpreston Mar 14, 2022

seanpreston Mar 14, 2022

		@@ -0,0 +1 @@
		NEXT_PUBLIC_AUTH_ENDPOINT=http://0.0.0.0:8080/api/v1/oauth/token

[#228] Implement Privacy Center client app #239

[#228] Implement Privacy Center client app #239

Conversation

elliotbonneville commented Mar 1, 2022 • edited Loading

Purpose

Changes

Checklist

Ticket

seanpreston Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpreston Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasLaPiana Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

elliotbonneville Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpreston left a comment

Choose a reason for hiding this comment

seanpreston commented Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliotbonneville commented Mar 1, 2022 •

edited

Loading

seanpreston Mar 9, 2022 •

edited

Loading

seanpreston Mar 10, 2022 •

edited

Loading

ThomasLaPiana Mar 10, 2022 •

edited

Loading

elliotbonneville Mar 14, 2022 •

edited

Loading