Feat: netlify deployments for frontend #3194

usu · 2022-12-03T16:14:20Z

As per Santa Claus wish from @manuelmeister:
Allows automatic netlify deployment of the frontend for each pull request. These frontend-deployments are then hard-wired to https://dev-api.ecamp3.ch

CORS is more or less disabled on dev to allow this. This is needed, because Netlify doesn't support custom domains for Preview Deployments (=Deployment for each PR).

Disabling CORS however needs to be triggered explicitly. Default behavior is (almost) the same as before (with the exception of one PR on LexikBundle that still needs to be merged).

A working demo is available under https://vermillion-travesseiro-ea7e4b.netlify.app/ and is connected to my own test API at https://api-ecamp3.mybytes.ch/

Once this PR is merged, I can setup the netlify workflows from our main Netlify account.

netlify · 2022-12-03T16:14:24Z

✅ Deploy Preview for ecamp3-frontend-preview ready!

Name	Link
🔨 Latest commit	`5d806ea`
🔍 Latest deploy log	https://app.netlify.com/sites/ecamp3-frontend-preview/deploys/638b7dfb8b1d57000893b21f
😎 Deploy Preview	https://deploy-preview-3194--ecamp3-frontend-preview.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

manuelmeister

Woooooooh 🤩😍, nice so cool!

carlobeltrame

Nice job, but at the same time a lot of alarm bells going off in my head when reading this.

I have to say overall I'm not a fan of opening these multiple security holes just for faster previews. I see the value of the quick frontend previews especially right now that the frontend is being worked on much more than the API. But opening CORS on dev will definitely bite us and community developers in the future, when we (or they) make first steps in developing more API clients on different domains. Such new clients will then work on dev but fail on prod. And also in general, this widens the gap between prod and dev deployments. So in my comments, I am trying to limit on all fronts the amount of security weakening which we perform.

I'd like to say right now, these preview deployments are no alternative to writing frontend tests and E2E tests. I see them only as a cheap and mobile-friendly alternative for previewing some simple UI change. They will not work for all features, and they do not replace a comprehensive code review. And I'd rather invest more time into improving the resource usage and developer setup of our application instead of creating many kinds of feature branch deployments which might or might not work for a new feature.

Is there some way to limit these frontend previews to only PRs which include solely frontend changes and no changes in the API etc.? (And ideally also exclude renovate and dependabot PRs, because these are rarely tested manually, but instead CI should handle them).

carlobeltrame · 2022-12-06T13:57:34Z

.github/workflows/continuous-deployment.yml

+            --set php.corsAllowOrigin='*' \
+            --set php.cookieSameSite='none' \
+            --set php.jwt.emptyBody='false' \
+            --set print.corsAllowAllOrigin='true' \


Can we implement a proper list of allowed origins for print as well, instead of an official "security hole switch"?

carlobeltrame · 2022-12-06T13:58:23Z

.github/workflows/continuous-deployment.yml

+            --set php.corsAllowOrigin='*' \
+            --set php.cookieSameSite='none' \


Can we limit these security holes to only the dev deployment, not the feature branch deployments? Or would it make sense to deploy a separate, deliberately insecure backend which is only used for the netlify frontend previews?

carlobeltrame · 2022-12-06T14:11:50Z

api/config/packages/lexik_jwt_authentication.yaml

@@ -10,6 +12,10 @@ lexik_jwt_authentication:
    # Of course it would be even better to have only short-lived tokens but renew them on every request.
    token_ttl: 43200

+    # include token in API response
+    # remove_token_from_body_when_cookies_used: '%env(bool:JWT_EMPTY_BODY)%' # this is possible after merge & release of https://github.com/lexik/LexikJWTAuthenticationBundle/pull/1092
+    remove_token_from_body_when_cookies_used: false


Can we modify lexik so that it only returns the header and payload information of the token in the response body? The reason that lexik does not want to return the token in the payload is reportedly that JS code should never have access to the full JWT token. I think lexik supports customizing the response body: https://github.com/lexik/LexikJWTAuthenticationBundle/blob/2.x/Resources/doc/2-data-customization.rst#adding-custom-data-or-headers-to-the-jwt

carlobeltrame · 2022-12-06T14:13:09Z

frontend/src/plugins/auth.js

+  const token = store.state.auth.token
+  if (token) {
+    return token.split('.')[1]
+  }


I don't really like that we will ship this in production, but I see no quick and easy way around it right now.

carlobeltrame · 2022-12-06T14:14:52Z

.helm/ecamp3/values.yaml

@@ -41,6 +41,7 @@ php:
    passphrase:
    privateKey:
    publicKey:
+    emptyBody:


Can you set the default in the helm chart to true? I know it's not respected by lexik yet, but as soon as lexik supports it we should make true the default.

carlobeltrame · 2022-12-06T14:15:57Z

.helm/ecamp3/values.yaml

@@ -30,7 +30,7 @@ php:
  appEnv: prod
  appDebug: "0"
  appSecret: ""
-  corsAllowOrigin: "^https://.*?\\.chart-example\\.local$"
+  corsAllowOrigin:


Maybe add a comment which still indicates (via an example or written description) that this value may be a regex, may assert on the protocol, etc.

carlobeltrame · 2022-12-06T14:18:08Z

frontend/src/plugins/auth.js

-  return apiStore.post(url, { identifier: email, password: password }).then(() => {
+  return axios.post(url, { identifier: email, password: password }).then((response) => {
+    if (response) {
+      store.commit('setToken', response.data.token)


Is the store really the correct place for this? This value will be undefined on deployments where we --set php.jwt.emptyBody='false' (which I expect for any production environment). If you put this value into the store, it signifies to me that any component may use this value, which will then break on production. In this case I would prefer a local let variable in the top scope of this file, so that only code in this file can access it.

carlobeltrame · 2022-12-06T14:18:43Z

frontend/src/views/auth/Login.vue

@@ -165,7 +164,7 @@ export default {
        })
        .catch((e) => {
          this.authenticationInProgress = false
-          this.error = serverErrorToString(e)
+          this.error = e.response?.data?.message


Why this change? Was the old implementation wrong?

carlobeltrame · 2022-12-06T14:33:20Z

frontend/src/plugins/__tests__/auth.spec.js

@@ -2,6 +2,7 @@ import Vue from 'vue'
 import { auth } from '@/plugins/auth'
 import storeLoader, { store, apiStore } from '@/plugins/store'
 import Cookies from 'js-cookie'
+import axios from 'axios'


If you agree with removing the token from the store, don't forget to revert this

manuelmeister · 2022-12-06T22:19:50Z

Core Team Decision

Deployment Optimierungen (chore(deploy): performance + autoscalers #3192) hat nicht die gewünschten Verbesserungen gebracht
5 Frontend PRs im Browser gleichzeitig anschauen nicht möglich (aktuell nur Dev + ein zusätzliches Deployment)
Wir streben eine zusätzliche Lösung mit einem dezidierten Endpunkt an, erst wenn eine generelle Verbesserung am ganzen Kubernetes Cluster erfolgt ist.

Anforderungen an den Deployment Prozess:

Zwischen Push und Live 2min

Anforderungen an Netlify:

4 gleichzeitige Deployments möglich (Dev, Endpunkt, 2 PR deployments)

BacLuc · 2023-07-15T06:43:58Z

can we close this PR?

usu · 2023-07-15T12:26:12Z

Will be closed due to several improvements on PR deployments, notably:
#3237
#3584

usu added 11 commits December 3, 2022 11:08

add build step & environement file for netlify

a366dc3

make corsAllowOrigin configurable

d96694c

test on usu's own cluster

2045071

fix api url

15f52ab

config parameter for COOKIE_SAMESITE

1b8dd83

add netlify config with redirect

de520a4

fix cookie usage

32c900f

allow token to be send in response body as alternative

3ad6a6e

fix cors for print

3305e8d

minor fixes on login error and deployment

ff2bce9

switch from usu's test cluster to ecamp3-dev

abb38f6

usu changed the title ~~Feat/netlify deployment ecamp~~ Feat: netlify deployments for frontend Dec 3, 2022

manuelmeister approved these changes Dec 3, 2022

View reviewed changes

usu requested a review from carlobeltrame December 3, 2022 16:36

fix tests

5d806ea

carlobeltrame requested changes Dec 6, 2022

View reviewed changes

BacLuc mentioned this pull request Dec 28, 2022

Make reviewing frontend changes of PR's faster #3214

Closed

usu marked this pull request as draft February 26, 2023 09:16

usu closed this Jul 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: netlify deployments for frontend #3194

Feat: netlify deployments for frontend #3194

usu commented Dec 3, 2022

netlify bot commented Dec 3, 2022 •

edited

Loading

manuelmeister left a comment •

edited

Loading

carlobeltrame left a comment

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

carlobeltrame Dec 6, 2022

manuelmeister commented Dec 6, 2022 •

edited

Loading

BacLuc commented Jul 15, 2023

usu commented Jul 15, 2023

		--set php.corsAllowOrigin='*' \
		--set php.cookieSameSite='none' \

Feat: netlify deployments for frontend #3194

Feat: netlify deployments for frontend #3194

Conversation

usu commented Dec 3, 2022

netlify bot commented Dec 3, 2022 • edited Loading

✅ Deploy Preview for ecamp3-frontend-preview ready!

manuelmeister left a comment • edited Loading

Choose a reason for hiding this comment

carlobeltrame left a 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

manuelmeister commented Dec 6, 2022 • edited Loading

Core Team Decision

Anforderungen an den Deployment Prozess:

Anforderungen an Netlify:

BacLuc commented Jul 15, 2023

usu commented Jul 15, 2023

netlify bot commented Dec 3, 2022 •

edited

Loading

manuelmeister left a comment •

edited

Loading

manuelmeister commented Dec 6, 2022 •

edited

Loading