2FA Confirmation Code Email subject line change to fix triggering Google spam blocker

[aureateflux]

Changed subject lines in both the regular and HTML version of the 2FA confirmation email because previous subject line ("Your Two-step Login Verification Code") was being blocked by Google's servers.

The new subject line is "Vaultwarden Confirmation Code"

Please comment if a different subject line is preferable for any reason.

I've only tested this subject line with the emailer configured to use a Google SMTP server. If you have Vaultwarden configured to use another email service, please test this template using your email service.

[tessus]

Personally I think that verification is a better term than confirmation.

Also, Google shouldn't just block messages because of a subject line. IMO they should check headers, DKIM, SPF, DMARC, and do a proper assessment. Blocking a subject line because they just feel like it is a joke. What's next? They don't like long time, no see. what's up? and then what?

[aureateflux]

I considered confirmation, verification, or authentication. If there's agreement, I'll make the change!

And agreed, super awesome that a 2FA feature gets turned into a game of whack-a-mole over something so arbitrary...

[tessus]

It's not a big deal, since people can still change it to whatever they want by creating their own templates.

Somebody should hold Google accountable for this idiocracy. But I also know how futile that is. Luckily I never trusted any mail servers that are not under my control, so I don't have that problem.

I guess the default should work for the majority of users, so I will step back and let this run its course.

[aureateflux]

Personally I think that verification is a better term than confirmation.

Applied this suggestion for the sake of internal consistency with how the codes are referenced both in the email itself and in the app itself.

[aureateflux]

Can we get a review on this? Super small change: just tweaking a template. Should help a lot of people who are using gmail smtp. Sorry to ask, but I don't see another way to request a review.

[BlackDex]

Wouldn't Your Vaultwarden Login Verification Code be better?
If that works too, i would prefer that actually.

The reason is, that what i have learned with sending mails is that it is important to use the name of the product or site in the subject instead of some general subject line. But also be a bit more descriptive where it is for.

A verification code for what? So adding Login would make it more descriptive.

[aureateflux]

Makes sense. I tested several variations, but I didn't write down all the ones that worked aside from the ones I mentioned in the other discussion thread.

I'll test with "login" to see if it passes through Google's filter.

ETA: New subject line is "Vaultwarden Login Verification Code" per @BlackDex's suggestion. Incidentally, in testing this, I confirmed that the "incomplete 2fa login" email is not blocked by Google; also in case anyone was wondering how robust that function is, it worked correctly across a container reboot. 👍

[BlackDex]

Could you squash these commits? 6 commits for such a small change is a bit of clutter in the git history.

[tessus]

@BlackDex In theory you are correct. But I have learned from my own and other projects that squashing (and force-pushing) within a PRs should be avoided. I am not talking about rebasing the PR.

However, it should be noted that a merge commit is always a no-no. I have no idea why this is done in this project. The PRs should be squashed upon merging. There is even an option to make this the default and also a possibility to make this the only merge option.

I am sorry for interjecting here. I just always wondered why this project uses merge commits. It is much nicer to follow the progress of a PR with multiple commits. But I never undrstood why one would want to keep all those commits when merging to master. That is the part that makes no sense. Squashing when merging makes perfect sense, but when someone looks at the PR they can still see all the commits and why they were done.

Anyway, sorry for the wall of text. I thought about opening a discussion about this, but all of a sudden your comment gave me the perfect opportunity to reply.

[BlackDex]

@BlackDex In theory you are correct. But I have learned from my own and other projects that squashing (and force-pushing) within a PRs should be avoided. I am not talking about rebasing the PR.

However, it should be noted that a merge commit is always a no-no. I have no idea why this is done in this project. The PRs should be squashed upon merging. There is even an option to make this the default and also a possibility to make this the only merge option.

I am sorry for interjecting here. I just always wondered why this project uses merge commits. It is much nicer to follow the progress of a PR with multiple commits. But I never undrstood why one would want to keep all those commits when merging to master. That is the part that makes no sense. Squashing when merging makes perfect sense, but when someone looks at the PR they can still see all the commits and why they were done.

Anyway, sorry for the wall of text. I thought about opening a discussion about this, but all of a sudden your comment gave me the perfect opportunity to reply.

I think that is very easy to answer, i think @dani-garcia uses the cli to merge multiple PR's and pushes that in one go, also to prevent multiple triggers of the GitHub Actions. If you click on the or view command line instructions. link next to the merge button (Where you can indeed select the squash option), it only shows the default merge option as cli options which we use to copy/paste the commands from.

But, I'm all for streamlining this a bit more, and to be fair, i actually never looked at this with to much attention.
So, I'm glad you bring this to our attention!

[tessus]

If you click on the or view command line instructions. link next to the merge button (Where you can indeed select the squash option), it only shows the default merge option as cli options which we use to copy/paste the commands from.

This is correct, but I have done the squash-merge via cli before myself, I just can't reacall the steps off the top of my head. But doing this via the gh interface shouldn't be a huge inconvenience. It's not that there are thousands of PRs every month. ;-) (But I do understand the preference of doing everything via he command line.)

[BlackDex]

I think instead of using --no-ff it should be --squash, but not sure from the top of my head.

[aureateflux]

Frankly, I wasn't sure what the correct way to keep the branch updated is, or if it even needed to be kept up-to-date pending review. Should I be updating with rebase? Just wait until it gets reviewed?

I'll do whatever is appropriate here, assuming I can figure out how to do it (this is the first time I've ever submitted a Pull Request as I'm not a developer).

[snp88]

Did this change will be in main branch any time soon ? ;)?

[dani-garcia]

Honestly I haven't put much thought in the past about how PRs are commited. I have some git aliases for the commands I use based on what GitHub recommends by default, and I've never had to look too far in the repository's history that the strange mess of commits a merge makes was too problematic.

Now that you bring it up, seems pretty reasonable to just force squash merging on GitHub's settings and update my git aliases.