[CAPTCHA] Replace ReCAPTCHA with Turnstile #3249
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3249 +/- ##
==========================================
- Coverage 74.76% 74.52% -0.25%
==========================================
Files 278 279 +1
Lines 10652 10669 +17
Branches 1284 1287 +3
==========================================
- Hits 7964 7951 -13
- Misses 2322 2357 +35
+ Partials 366 361 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 14 of 25 files at r1, 2 of 7 files at r2, all commit messages.
Reviewable status: 16 of 29 files reviewed, 10 unresolved discussions (waiting on @imnasnainaec)
.github/workflows/deploy_qa.yml line 7 at r2 (raw file):
branches: - master #- turnstile
Remove dead code.
Code quote:
#- turnstile.github/workflows/deploy_qa.yml line 98 at r2 (raw file):
needs: build # Only push to the QA server when built on the master branch #if: ${{ github.ref_name == 'master' }}
Re-enable this line.
Code quote:
#if: ${{ github.ref_name == 'master' }}Backend/Startup.cs line 182 at r2 (raw file):
emailServiceFailureMessage); // Should we add a COMBINE_TURNSTILE_SECRET_KEY check?
The secret key should be set and an additional variable should be set to indicate whether turnstile should be used:
- in the Kubernetes environment, the environment variable should be set - the ConfigMap is not setup to have optional members
- It is not good design to have values with special meanings, e.g. if the secret key is "Override" then don't use turnstile.
Code quote:
// Should we add a COMBINE_TURNSTILE_SECRET_KEY check?Backend/Controllers/UserController.cs line 27 at r2 (raw file):
private readonly IPermissionService _permissionService; private const string TurnstileVerifyUrl = "https://challenges.cloudflare.com/turnstile/v0/siteverify";
This is better as a configuration item than hard-coded in the source.
Code quote:
private const string TurnstileVerifyUrl = "https://challenges.cloudflare.com/turnstile/v0/siteverify";Backend/Controllers/UserController.cs line 38 at r2 (raw file):
} /// <summary> Validates a Cloudflare Turnstile token </summary>
If I understand it, this is only invoked when requested by the UI, correct? The function title makes it sound (to me) as though it is part of startup validation. Even so, the function should test that turnstile is enabled before trying to send the request.
Code quote:
/// <summary> Validates a Cloudflare Turnstile token </summary>deploy/helm/thecombine/values.yaml line 29 at r2 (raw file):
combineSmtpUsername: "Override" combineSmtpPassword: "Override" combineTurnstileSecretKey: "Override"
Add enable/disable variable.
Code quote:
combineTurnstileSecretKey: "Override"deploy/helm/thecombine/charts/backend/values.yaml line 24 at r2 (raw file):
combineSmtpUsername: "Override" combineSmtpPassword: "Override" combineTurnstileSecretKey: "Override"
Again, add enable/disable variable.
Code quote:
combineTurnstileSecretKey: "Override"deploy/scripts/install-combine.sh line 12 at r2 (raw file):
COMBINE_JWT_SECRET_KEY=`LC_ALL=C tr -dc 'A-Za-z0-9*\-_@!' </dev/urandom | head -c 64; echo` # Collect values from user read -p "Enter COMBINE_TURNSTILE_SECRET_KEY: " COMBINE_TURNSTILE_SECRET_KEY
Remove.
install-combine.sh is only called by the self-extracting installer when installing on a Linux laptop. If this is required, it no longer is a secret - we would need to distribute it to people who request it.
I am currently updating the script. I will add a comment at the beginning explaining its use. I can also rename it to linux-install.sh if you think that would help. I may be able to move it out of ../deploy/scripts as well.
Code quote:
read -p "Enter COMBINE_TURNSTILE_SECRET_KEY: " COMBINE_TURNSTILE_SECRET_KEYdeploy/scripts/install-combine.sh line 18 at r2 (raw file):
cat <<.EOF > ${CONFIG_DIR}/env export COMBINE_JWT_SECRET_KEY="${COMBINE_JWT_SECRET_KEY}" export COMBINE_TURNSTILE_SECRET_KEY="${COMBINE_TURNSTILE_SECRET_KEY}"
Remove this as well.
Code quote:
export COMBINE_TURNSTILE_SECRET_KEY="${COMBINE_TURNSTILE_SECRET_KEY}"public/locales/en/translation.json line 79 at r2 (raw file):
}, "turnstile": { "error": "Cloudflare Turnstile verification failed or expired. Please refresh the page to try again."
I think the reference to Cloudflare Turnstile is confusing for users - I do not expect them to know what it is. Perhaps Page verification expired. Please refresh the page.
Code quote:
Cloudflare Turnstile verification failed or expired. Please refresh the page to try again.
There was a problem hiding this comment.
Reviewable status: 6 of 38 files reviewed, 9 unresolved discussions (waiting on @jmgrady)
.github/workflows/deploy_qa.yml line 7 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Remove dead code.
I first want to uncomment this when we are ready to test on QA.
.github/workflows/deploy_qa.yml line 98 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Re-enable this line.
I first want to test on QA before uncommenting this after review is done.
Backend/Startup.cs line 182 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
The secret key should be set and an additional variable should be set to indicate whether turnstile should be used:
- in the Kubernetes environment, the environment variable should be set - the ConfigMap is not setup to have optional members
- It is not good design to have values with special meanings, e.g. if the secret key is "Override" then don't use turnstile.
I've yet to create another environment variable for "whether turnstile should be used" because environment variables are strings and I'm not sure the best way to represent a bool that won't require an arbitrary string-value check somewhere.
Backend/Controllers/UserController.cs line 27 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
This is better as a configuration item than hard-coded in the source.
Done.
Backend/Controllers/UserController.cs line 38 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
If I understand it, this is only invoked when requested by the UI, correct? The function title makes it sound (to me) as though it is part of startup validation. Even so, the function should test that turnstile is enabled before trying to send the request.
It seems that the functions presence in a Controller should provide the necessary context. This has a very similar summary to alike functions below.
public/locales/en/translation.json line 79 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
I think the reference to
Cloudflare Turnstileis confusing for users - I do not expect them to know what it is. Perhaps Page verification expired. Please refresh the page.
Done.
deploy/scripts/install-combine.sh line 12 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Remove.
install-combine.shis only called by the self-extracting installer when installing on a Linux laptop. If this is required, it no longer is a secret - we would need to distribute it to people who request it.I am currently updating the script. I will add a comment at the beginning explaining its use. I can also rename it to
linux-install.shif you think that would help. I may be able to move it out of../deploy/scriptsas well.
Done.
deploy/scripts/install-combine.sh line 18 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Remove this as well.
Done.
There was a problem hiding this comment.
Reviewable status: 6 of 44 files reviewed, 9 unresolved discussions (waiting on @jmgrady)
Backend/Startup.cs line 182 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I've yet to create another environment variable for "whether turnstile should be used" because environment variables are strings and I'm not sure the best way to represent a bool that won't require an arbitrary string-value check somewhere.
I'm also wondering whether it's worthwhile to engineer that variable in the backend when the only function that uses it will never be called if its false.
There was a problem hiding this comment.
Reviewed 14 of 19 files at r3, 4 of 12 files at r4, 3 of 7 files at r5, 9 of 10 files at r7, all commit messages.
Reviewable status: 31 of 44 files reviewed, 11 unresolved discussions (waiting on @imnasnainaec)
README.md line 146 at r5 (raw file):
1. If you want the email services to work you will need to set the following environment variables. These values must be kept secret, so ask your email administrator to supply them. Set them in your `.profile` (Linux or Mac 10.14-), your
What about setting COMBINE_JWT_SECRET_KEY?
Code quote:
1. If you want the email services to work you will need to set the following environment variables. These values must be
kept secret, so ask your email administrator to supply them. Set them in your `.profile` (Linux or Mac 10.14-), your.github/workflows/deploy_qa.yml line 7 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I first want to uncomment this when we are ready to test on QA.
That make sense. I will leave this unresolved until the testing is complete and this file is updated.
.github/workflows/deploy_qa.yml line 98 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I first want to test on QA before uncommenting this after review is done.
Ditto.
Backend/Startup.cs line 182 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I'm also wondering whether it's worthwhile to engineer that variable in the backend when the only function that uses it will never be called if its
false.
Sorry I was not clearer. My point was not that the issue is that there is a mixing of data types but that there is a mixing of function. On purpose is to enable/disable turnstile; the other is to provide the secret to make turnstile work.
As you stated, environment variables are strings and the API should be defined as: If the environment variable's value equals the string "true", then the functionality is enabled. It is not enabled for any other value.
Backend/Startup.cs line 198 at r5 (raw file):
null, turnstileFailureMessage); });
These log errors if turnstile is not setup but it is a valid configuration if it is not enabled. These errors should only be logged if Turnstile is turned on. Perhaps _logger.LogInformation can be called when Turnstile is disabled.
Code quote:
const string turnstileFailureMessage = "Turnstile verification will not be available.";
options.TurnstileSecretKey = CheckedEnvironmentVariable(
"TURNSTILE_SECRET_KEY",
null,
turnstileFailureMessage);
options.TurnstileVerifyUrl = CheckedEnvironmentVariable(
"TURNSTILE_VERIFY_URL",
null,
turnstileFailureMessage);
});Backend/Startup.cs line 246 at r5 (raw file):
// Turnstile types services.AddTransient<ITurnstileContext, TurnstileContext>(); services.AddTransient<ITurnstileService, TurnstileService>();
Similarly, shouldn't these transients only be added when Turnstile is enabled?
Code quote:
// Turnstile types
services.AddTransient<ITurnstileContext, TurnstileContext>();
services.AddTransient<ITurnstileService, TurnstileService>();deploy/helm/thecombine/values.yaml line 59 at r6 (raw file):
configShowCertExpiration: false configAnalyticsWriteKey: "" configTurnstileRequired: false
This should be in the global variables. I still maintain that the backend should know whether turnstile is enabled rather than assume no requests will come in - the backend should behave reasonably if it receives unexpected data.
Code quote:
configTurnstileRequired: falsedeploy/scripts/setup_files/profiles/dev.yaml line 15 at r7 (raw file):
frontend: configTurnstileRequired: "true" # https://developers.cloudflare.com/turnstile/troubleshooting/testing/
This comment would be more helpful if there were some explanation of what the URL is for.
Code quote:
# https://developers.cloudflare.com/turnstile/troubleshooting/testing/nginx/init/25-combine-runtime-config.sh line 53 at r7 (raw file):
["CONFIG_USE_CONNECTION_URL"]="useConnectionBaseUrlForApi" ["CONFIG_TURNSTILE_REQUIRED"]="turnstileRequired" ["CONFIG_TURNSTILE_SITE_KEY"]="turnstileSiteKey"
Technically, renaming CAPTCHA to Turnstile in all of the files is not necessary. CAPTCHA stands for "Completely Automated Public Turing test to tell Computers and Humans Apart." The Google implementation is reCAPTCHA.
Code quote:
["CONFIG_TURNSTILE_REQUIRED"]="turnstileRequired"
["CONFIG_TURNSTILE_SITE_KEY"]="turnstileSiteKey"
There was a problem hiding this comment.
Reviewable status: 12 of 50 files reviewed, 10 unresolved discussions (waiting on @jmgrady)
README.md line 146 at r5 (raw file):
Previously, jmgrady (Jim Grady) wrote…
What about setting
COMBINE_JWT_SECRET_KEY?
I added it to Backend/Properties/launchSettings.json so it's no longer needed for npm run backend or npm start. It is still required for a dev docker build, so I moved the instruction to ### Setup Environment Variables below.
Backend/Startup.cs line 182 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Sorry I was not clearer. My point was not that the issue is that there is a mixing of data types but that there is a mixing of function. On purpose is to enable/disable turnstile; the other is to provide the secret to make turnstile work.
As you stated, environment variables are strings and the API should be defined as: If the environment variable's value equals the string "true", then the functionality is enabled. It is not enabled for any other value.
Added, though it should be "True" as that is the bool.TrueString in C#. I also added an analogous backend config for email enabled, to make sure they are both implemented the same way.
Backend/Startup.cs line 246 at r5 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Similarly, shouldn't these transients only be added when Turnstile is enabled?
I think conditionally adding transients for Email/Turnstile contexts/services is a whole new ballgame.
deploy/helm/thecombine/values.yaml line 29 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Add enable/disable variable.
Done.
deploy/helm/thecombine/values.yaml line 59 at r6 (raw file):
Previously, jmgrady (Jim Grady) wrote…
This should be in the global variables. I still maintain that the backend should know whether turnstile is enabled rather than assume no requests will come in - the backend should behave reasonably if it receives unexpected data.
I added it as a backend variable separately because of different handling (bool for the frontend config and "True"/"False" for backend environment variable parsing).
deploy/helm/thecombine/charts/backend/values.yaml line 24 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Again, add enable/disable variable.
Done.
deploy/scripts/setup_files/profiles/dev.yaml line 15 at r7 (raw file):
Previously, jmgrady (Jim Grady) wrote…
This comment would be more helpful if there were some explanation of what the URL is for.
Done.
nginx/init/25-combine-runtime-config.sh line 53 at r7 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Technically, renaming CAPTCHA to Turnstile in all of the files is not necessary. CAPTCHA stands for "Completely Automated Public Turing test to tell Computers and Humans Apart." The Google implementation is reCAPTCHA.
Good point. And such a fun acronym...
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed 1 of 25 files at r1, 2 of 7 files at r2, 13 of 35 files at r8, 11 of 29 files at r9, 19 of 20 files at r10, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @imnasnainaec)
README.md line 146 at r5 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I added it to
Backend/Properties/launchSettings.jsonso it's no longer needed fornpm run backendornpm start. It is still required for a dev docker build, so I moved the instruction to### Setup Environment Variablesbelow.
This is a small additional scope for the PR. It would be good to mention it in the PR description.
Backend/Startup.cs line 182 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Added, though it should be "True" as that is the
bool.TrueStringin C#. I also added an analogous backend config for email enabled, to make sure they are both implemented the same way.
Let's discuss this in Slack so that we can have a more interactive discussion and have the same design intent to reduce the number of code changes you end up making.
Backend/Startup.cs line 246 at r5 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I think conditionally adding transients for Email/Turnstile contexts/services is a whole new ballgame.
My concern is mostly why run services that are not supposed to be enabled? It looks like the service is still needed because that is what will throw the exception that says it is not enabled.
deploy/helm/thecombine/values.yaml line 59 at r6 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I added it as a backend variable separately because of different handling (bool for the frontend config and "True"/"False" for backend environment variable parsing).
I see it is done.
deploy/helm/thecombine/charts/backend/values.yaml line 21 at r10 (raw file):
awsDefaultRegion: "Override" pullSecretName: "None" combineJwtSecretKey: "Override"
captchaRequired and emailEnabled need to be added to the global section.
deploy/helm/thecombine/charts/backend/templates/backend-config-map.yaml line 12 at r10 (raw file):
{{- else }} COMBINE_CAPTCHA_REQUIRED: "False" {{- end }}
Should be:
COMBINE_CAPTCHA_REQUIRED: {{ .Values.global.captchaRequired | quote }}
bool.Parse is case insensitive. As the documentation states, it tests that the string is equivalent to (not equal to) bool.TrueString.
Code quote:
{{- if .Values.global.captchaRequired }}
COMBINE_CAPTCHA_REQUIRED: "True"
{{- else }}
COMBINE_CAPTCHA_REQUIRED: "False"
{{- end }}deploy/helm/thecombine/charts/backend/templates/backend-config-map.yaml line 18 at r10 (raw file):
{{- else }} COMBINE_EMAIL_ENABLED: "False" {{- end }}
Similarly, should be:
COMBINE_EMAIL_ENABLED: {{ .Values.global.emailEnabled | quote }}
Code quote:
{{- if .Values.global.emailEnabled }}
COMBINE_EMAIL_ENABLED: "True"
{{- else }}
COMBINE_EMAIL_ENABLED: "False"
{{- end }}deploy/scripts/install-combine.sh line 21 at r10 (raw file):
export COMBINE_CAPTCHA_REQUIRED="False" export COMBINE_EMAIL_ENABLED="False" export COMBINE_JWT_SECRET_KEY="${COMBINE_JWT_SECRET_KEY}"
The captcha and email enabled environment variables should not be needed. These values should be driven by the profile.
I don't think that any of the changes for Turnstile should affect this file.
Code quote:
export AWS_DEFAULT_REGION="us-east-1"
export AWS_ACCESS_KEY_ID="${AWS_ACCESS_KEY_ID}"
export AWS_SECRET_ACCESS_KEY="${AWS_SECRET_ACCESS_KEY}"
export COMBINE_CAPTCHA_REQUIRED="False"
export COMBINE_EMAIL_ENABLED="False"
export COMBINE_JWT_SECRET_KEY="${COMBINE_JWT_SECRET_KEY}"deploy/scripts/setup_files/profiles/nuc_qa.yaml line 14 at r10 (raw file):
global: awsS3Location: prod.thecombine.app captchaRequired: false
We probably want these to be true. The nuc_qa profile should not be shipped for use in a workshop. It installs The Combine on the NUC with the QA environment, that is, it is setup to be able to pull unreleased code images from the AWS private container registry. Normally the NUC can only install release images from the AWS Public container registry. (Although with the annoying Google reCAPTCHA I would probably want it set to false!)
Of course, if you change them, then the CAPTCHA site key will need to be added.
There was a problem hiding this comment.
Reviewable status: 48 of 52 files reviewed, 12 unresolved discussions (waiting on @jmgrady)
deploy/helm/thecombine/charts/backend/values.yaml line 21 at r10 (raw file):
Previously, jmgrady (Jim Grady) wrote…
captchaRequiredandemailEnabledneed to be added to the global section.
Aren't default values being pulled from deploy\helm\thecombine\values.yaml?
deploy/helm/thecombine/charts/backend/templates/backend-config-map.yaml line 12 at r10 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Should be:
COMBINE_CAPTCHA_REQUIRED: {{ .Values.global.captchaRequired | quote }}
bool.Parseis case insensitive. As the documentation states, it tests that the string is equivalent to (not equal to)bool.TrueString.
Done.
deploy/helm/thecombine/charts/backend/templates/backend-config-map.yaml line 18 at r10 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Similarly, should be:
COMBINE_EMAIL_ENABLED: {{ .Values.global.emailEnabled | quote }}
Done.
deploy/scripts/install-combine.sh line 21 at r10 (raw file):
Previously, jmgrady (Jim Grady) wrote…
The captcha and email enabled environment variables should not be needed. These values should be driven by the profile.
I don't think that any of the changes for Turnstile should affect this file.
You're right about COMBINE_CAPTCHA_REQUIRED and COMBINE_EMAIL_ENABLED--I retracted those. But I think COMBINE_SMTP_USERNAME is no longer required with the new handling of the emailEnabled config.
deploy/scripts/setup_files/profiles/nuc_qa.yaml line 14 at r10 (raw file):
Previously, jmgrady (Jim Grady) wrote…
We probably want these to be
true. Thenuc_qaprofile should not be shipped for use in a workshop. It installs The Combine on the NUC with the QA environment, that is, it is setup to be able to pull unreleased code images from the AWS private container registry. Normally the NUC can only install release images from the AWS Public container registry. (Although with the annoying Google reCAPTCHA I would probably want it set tofalse!)Of course, if you change them, then the CAPTCHA site key will need to be added.
The Turnstile site key won't work outside of the domains set for it in the Cloudflare account. Should nuc*.thecombine.app be added?
There was a problem hiding this comment.
Reviewed 2 of 4 files at r11, 4 of 4 files at r12, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @imnasnainaec)
Backend/Properties/launchSettings.json line 31 at r12 (raw file):
"Key": "Value", "ASPNETCORE_ENVIRONMENT": "Development", "COMBINE_CAPTCHA_REQUIRED": "true",
This is not really required since it is the default, correct?
Code quote:
"COMBINE_CAPTCHA_REQUIRED": "true",deploy/scripts/setup_files/profiles/nuc_qa.yaml line 14 at r10 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
The Turnstile site key won't work outside of the domains set for it in the Cloudflare account. Should
nuc*.thecombine.appbe added?
It should work. I will test it and let you know. The domain is thecombine.app; nuc*.thecombine.app is a host in that domain.
There was a problem hiding this comment.
Dismissed @Github-advanced-security[bot] from 4 discussions.
Reviewable status: 48 of 52 files reviewed, 5 unresolved discussions (waiting on @jmgrady)
Backend/Properties/launchSettings.json line 31 at r12 (raw file):
Previously, jmgrady (Jim Grady) wrote…
This is not really required since it is the default, correct?
This is required for npm start and npm run backend because the Helm charts haven't been run.
deploy/scripts/setup_files/profiles/nuc_qa.yaml line 14 at r10 (raw file):
Previously, jmgrady (Jim Grady) wrote…
It should work. I will test it and let you know. The domain is
thecombine.app;nuc*.thecombine.appis a host in that domain.
We added both thecombine.app and qa-kube.thecombine.app explicitly in generating the keys, but it would be good to know if it works on other *.thecombine.app domains.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r13, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @imnasnainaec)
deploy/scripts/setup_files/profiles/nuc_qa.yaml line 14 at r10 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
We added both
thecombine.appandqa-kube.thecombine.appexplicitly in generating the keys, but it would be good to know if it works on other*.thecombine.appdomains.
It looks like CloudFlare is using a different scope for their domains than I would expect:
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @imnasnainaec)
deploy/helm/thecombine/charts/backend/values.yaml line 21 at r10 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Aren't default values being pulled from
deploy\helm\thecombine\values.yaml?
I had thought that the global values needed to be re-declared in the sub-charts - similar to defining an external reference. Perhaps that is not the case. It seems to work correctly and is cleaner this way.
There was a problem hiding this comment.
Reviewable status: 51 of 52 files reviewed, 3 unresolved discussions (waiting on @jmgrady)
.github/workflows/deploy_qa.yml line 7 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
That make sense. I will leave this unresolved until the testing is complete and this file is updated.
Done.
.github/workflows/deploy_qa.yml line 98 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Ditto.
Done.
Backend/Startup.cs line 198 at r5 (raw file):
Previously, jmgrady (Jim Grady) wrote…
These log errors if turnstile is not setup but it is a valid configuration if it is not enabled. These errors should only be logged if Turnstile is turned on. Perhaps
_logger.LogInformationcan be called when Turnstile is disabled.
Done.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)
deploy/helm/thecombine/charts/frontend/values.yaml line 17 at r15 (raw file):
imageRegistry: "" imageTag: "latest" pullSecretName: aws-login-credentials
The default pullSecretName should be the same for all the subcharts. It is "aws-login-credentials" here and "None" for the backend. This was my error.
"None" is probably the better value. Originally, all deployments needed the image pull secrets to pull images from the AWS ECR private registry. Now the QA server is the only one that does that; the rest pull releases from the public registry.
I am fine with deferring this to a later clean up task.
Code quote:
pullSecretName: aws-login-credentials
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)
Resolves #2407
Also:
emailEnabledconfig and implement it inEmailService.csBackend/Properties/launchSettings.jsonand updatesREADMEaccordinglyThis change is