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

update to web-v2023.8.2 and rename to Vaultwarden Web #138

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

stefan0xC
Copy link
Contributor

It seems that in the next release, the translation key pageTitle will be removed (see bitwarden/clients#5780) and replaced by a new one called bitWebVault where Bitwarden is hardcoded in the messages.json files. I would use that as opportunity to rectify the issue that @tessus has brought up in #131 by reverting the change and renaming the modified web-vault to Vaultwarden Web instead (omitting the last Vault as we already have that in Vaultwarden itself).

If we want to credit Bitwardenı® Web Vault in the client itself (which arguably we don't have to), I think we could add another row to the footer, where we can have a better phrase without parentheses, e.g.

Vaultwarden is an unofficial rewrite of the Bitwarden® server. The Vaultwarden Web client is a modified version of the Bitwarden® Web Vault.
The Vaultwarden project is not associated with the Bitwarden® project nor Bitwarden Inc.

vaultwarden-footer

Since we don't use this 404 page anymore, I think it would make sense to remove it from the patch file and just don't ship it in apps/web/webpack.config.js.

And because I have been testing a different way of managing the changes (see #137), I would appreciate it, if we could run a linter and prettier before creating a patch, so it's easier to keep the changes in sync. I mean, those are mostly superficial changes but I think they don't really hurt either and we also require cargo fmt and clippy for Vaultwarden.

@tessus
Copy link
Contributor

tessus commented Aug 15, 2023

Have you also tested how it looks like after logging in?

The long text is actually nice on the login page, but I doubt it would looks nice in the rest of the app:

image

@stefan0xC

This comment was marked as outdated.

@BlackDex
Copy link
Collaborator

I personally would still like to have some notice of the copyright or origin that the web-vault is a fork from Bitwarden®. Mostly to still indicate a link.

But, it doesn't have to be that long though haha.
I'm also not sure what a good alternative would be right now.

@tessus
Copy link
Contributor

tessus commented Aug 15, 2023

I think the long text would be great for the login page. But only for the login page.

In the rest of the app, I would just

  • not populate the middle column at all
  • use a short text in the middle column: (unofficial Bitwarden® server). And in this case the parenthesis are ok (I think I would prefer this one)

@stefan0xC

This comment was marked as off-topic.

@tessus
Copy link
Contributor

tessus commented Aug 15, 2023

I am hopeful that the smiley means that this was sarcasm. Haha.

@tessus
Copy link
Contributor

tessus commented Aug 15, 2023

v2023.8.0 was just released.

@stefan0xC stefan0xC changed the title rename to Vaultwarden Web and get rid of 404 page rename to Vaultwarden Web, get rid of 404 page and update to web-v2023.8.0 Aug 15, 2023
@tessus
Copy link
Contributor

tessus commented Aug 15, 2023

I am still waiting for some feedback about the footer in the app (not the login page).

Is it even possible to have different text on the login page and the rest of the app?
As mentioned before, the long text looks really bad in the app. Please don't use this in the app.

But it looks great on the login page (the first image you poated).

@stefan0xC
Copy link
Contributor Author

Yes, those are two different files (footer.component.html vs frontend-layout.component.html), so it's possible to have different texts.

Before updating to v2023.8.0 I've already added the third version for now (until I get or have a better suggestion for how to display it better) because I think it looks better than having nothing and I'm not sure I like a free-floating "(unofficial Bitwarden® server)"

@tessus
Copy link
Contributor

tessus commented Aug 15, 2023

I certainly like the (unofficial Bitwarden® server) in the middle column best. I really dislike the long text in the middle. So in that case, why not use the previous layout for the app.

left: Vaultwarden Web (unofficial Bitwarden® server)
right: Version 2023.8.0

if you don't like that, how about:

left: Vaultwarden Web
middle: running on Vaultwarden, an unofficial Bitwarden® server
right: Version 2023.8.0

@stefan0xC

This comment was marked as outdated.

@tessus
Copy link
Contributor

tessus commented Aug 16, 2023

I don't want to be ungrateful for your attempt to make the long text shorter, but this doesn't look like a footer, nor does it seem any shorter.
Try like 10-12 words in total. Your idea is too overwhelming for a footer and requires to spend cognitive load on something that is basically unimportant.

I really do no understand why you want to plaster a banner (this is what it rather looks like) on every single page in the web app. Or maybe it is your idea of annoy people so that they won't misunderstand versioning. The operative word here is "annoy". It has a nag screen feel to it.

A footer should be unobtrusive. Do the examples (all but the one where you omitted the long text) strike you as discreet?

Please tell me what you think of my suggestions? Why are they not ok? I checked how they look, and they look great for a footer. They tell you everything there is to know.

The long text is more like documentation or an appeal to people to finally understand the point you are trying to make. This is why it is great for the login page. It is the first thing people see. If they haven't read it there, they won't read it in th app either. It just makes the whole experience more clunky and annoying for people who know what the text means but have to see that text on every operation in the app.
This is really bad UX. Don't take this personal, I am not trying to belittle your effort.

I have mentioned this before and it is not only my opinion but also a fact:

Making the obvious more obvious will not yield a different result.

If people are ignorant or think they know everything, they won't read documentation, or a wiki page, or an annoying footer, or an issue template.
I have tried the same thing you are trying to do with a real banner in a forum, a template in certain categories of a forum, and I have also used issue templates. Guess what, all these things didn't work. People delete the templates, or even keep the templates and just ignore them. I am just trying to explain that a long text won't change anything, but to annoy people who do not need to read the text, because they already understand the components and versioning concept.

@stefan0xC
Copy link
Contributor Author

As said in the beginning, I'm totally okay with having no text there but I also don't think that a longer text is that distracting or bad UI as you make it out to be. Please don't overexaggerate the issue because you disagree with me.

Maybe I was unclear but I have suggested those three lines with reasons, which is why I have not adopted different ones or taking your suggestions seriously. Sorry, about that but I did not see the point.

Trying to be serious for a moment and defending my proposal and why I am suggesting three short sentences instead of being content with something like "Vaultwarden Web (unofficial Bitwarden® server)", here's what I'm thinking:

  1. They clearly identify what the users are looking at without calling the web-client a server.
  2. We make our relations to the Bitwarden® project clear in proper accordance with their trademark guidelines and the requirements of the GPLv3.
  3. We do that even if we legally don't have to, since it's only fair and in the spirit of cooperation.

Legally, the trademark guidelines would be fulfilled by not mentioning Bitwarden at all. The GPLv3 (§ 5.) also states that

d) If the work has interactive user interfaces, each must display Appropriate Legal Notices; however, if the Program has interactive interfaces that do not display Appropriate Legal Notices, your work need not make them do so.

However since you repeated something you already said in #131 and what I do have to make clear is that my changes are not to imply that our users are ignorant or stupid. So please stop suggesting that. Also I am not the one who decides what gets added (I'm just a contributor making suggestions). It may just be a difference of opinion or aesthetics and I definitely disagree with your assessments. But since I agree with you that is not that important I don't really see the point in arguing this further with you (especially if you want to insult other users).

@tessus
Copy link
Contributor

tessus commented Aug 16, 2023

Just to be clear (and this is important to me), I was not insulting anyone, I was stating a fact. Please look up the words (ignorance and stupidity) in a dictionary and check, whether they fit or not. Although stupidity might be too harsh when you look at that word alone, you have to read it in context with ignorance, as it was meant.
How is someone who ignores issue templates, documentation, and READMEs not ignorant?
If someone is offended, why do you think that is?

Anyway, this is my last comment on that matter. This is way off-topic and I certainly did not want to offend anyone or have the dicussion go in this direction. But I can't be made the bad guy, because I stated a fact. Please don't twist my words or interpret something that isn't there.

When it comes to the footer we apparently won't agree on it ever.
Luckily I can just create my own version and all is good.

I know that you do not make the decisions, but you have been an avid contributor to this project and the project owners will listen to you more than they would to me.
So I wanted to at least voice my concerns and my opinion.

@stefan0xC

This comment was marked as off-topic.

@stefan0xC
Copy link
Contributor Author

stefan0xC commented Aug 17, 2023

Back to topic, I think we have multiple options from minimalist to more verbose for the footer:

I think the minimalist change (i.e. "Vaultwarden Web" on the left, "Version v2023.8.0" on the right) serves as the basis of the following suggestions and I would like to say that I slightly prefer it over the current/previous options but not very much.

I think that I can also live with slight variation of @tessus' last suggestion (i.e. "running on Vaultwarden, an unofficial [rewrite of the] Bitwarden® server". I am not particularly fond of it and I would rather we brainstorm some more or get some more perspectives before settling on this one but I'd definitely prefer it over "Vaultwarden Web (unofficial Bitwarden® server)" (or the current/previous version)...

Looking back to the suggestions from the previous PR, maybe we can do something like a mix. I.e. explain about Vaultwarden (the server) on the left and Vaultwarden Web (the client) on the right?

As already stated my most preferred option at the moment would be to use the same three statements on both footers (so we can mention "Bitwarden®" as often as possible or see my better reasons above ☝️ ), which I have tried to reduce to the essentials I would like to convey: "Vaultwarden is an unofficial Bitwarden® server. Vaultwarden Web is a modified version of the Bitwarden® Web Vault. The Vaultwarden project is not associated with the Bitwarden® project nor Bitwarden, Inc." To make the footer less obtrusive we could also reduce the font-size and/or the whitespace around it (which is rather high with a padding of 40px).

However, I agree that it might not be necessary to use the same statements in both footers. So maybe we can save some space by not using the last sentence? (Which is probably the most dispensable one because I rather we mention both Vaultwarden as well as the modifications to the Bitwarden® Web-Vault and not something that is probably superfluous).

@stefan0xC
Copy link
Contributor Author

stefan0xC commented Aug 18, 2023

maybe we can save some space by not using the last sentence?

Vaultwarden is an unofficial rewrite of the Bitwarden® server using a modified version of the Bitwarden® Web Vault.

If we were okay with such a statement the footer would (currently) look like this
Proposal at 2023-08-19 at 00-53-04 Vaultwarden Web

or more succinctly:
A modified version of the Bitwarden® Web Vault for Vaultwarden

@stefan0xC stefan0xC force-pushed the rename-vaultwarden-web branch from 933109d to 60bde95 Compare August 18, 2023 22:59
@stefan0xC
Copy link
Contributor Author

stefan0xC commented Aug 24, 2023

While testing the web-v2023.8.2 release I noticed that the organization creation has changed between web-v2023.7.1 and web-v2023.8.0, so I'll have to take another look at it. I'm not sure what the web-vault expects (i.e. getPlans() asks for /plans/all instead of /plans/).

edit: should be an easy fix dani-garcia/vaultwarden#3797

@stefan0xC stefan0xC changed the title rename to Vaultwarden Web, get rid of 404 page and update to web-v2023.8.0 rename to Vaultwarden Web, get rid of 404 page and update to web-v2023.8.2 Aug 25, 2023
@tessus
Copy link
Contributor

tessus commented Aug 26, 2023

So does this mean that the org creation will fail on 2023.8.2, if dani-garcia/vaultwarden#3797 has not been merged?

@stefan0xC
Copy link
Contributor Author

It will not work and say something along the line that you have not selected a plan. (And you can't because the required component is just missing and I have not figured out how to make the API call obsolete.)

@stefan0xC
Copy link
Contributor Author

stefan0xC commented Aug 29, 2023

after testing my 2fa policy removal change with web-v2023.8.2 i noticed that it also expects some UserDecryptionOptions in the /identity/connect/token response on a successful login (something like dani-garcia/vaultwarden#3813) so that the vault is shown instead of the set-password component.
Screenshot 2023-08-29 at 23-36-26 Set master password Vaultwarden Web

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

Overall it looks good.
I have not build it my self, and browsed through it though.

patches/v2023.7.1.patch Outdated Show resolved Hide resolved
patches/v2023.8.0.patch Outdated Show resolved Hide resolved
patches/v2023.8.2.patch Outdated Show resolved Hide resolved
patches/v2023.8.2.patch Outdated Show resolved Hide resolved
patches/v2023.8.2.patch Outdated Show resolved Hide resolved
@stefan0xC stefan0xC changed the title rename to Vaultwarden Web, get rid of 404 page and update to web-v2023.8.2 update to web-v2023.8.2 and rename to Vaultwarden Web Aug 31, 2023
@stefan0xC stefan0xC force-pushed the rename-vaultwarden-web branch from 53537cb to 88ca714 Compare August 31, 2023 17:13
in web-v2023.8.x the pageTitle (i.e. "$APP_NAME Web Vault") was removed,
so instead of using the hardcoded replacement, we replace the name of
the project `Vaultwarden Web`

the footer is changed again from v2023.7.1 for better legibility.

compared to the previous patch files some changes are purely cosmetic due
to running prettier and eslint.

and we don't need to change or pack the 404.html file since Vaultwarden
already provides a 404 page
@stefan0xC stefan0xC force-pushed the rename-vaultwarden-web branch from 88ca714 to 5f58716 Compare August 31, 2023 17:19
@BlackDex
Copy link
Collaborator

Ok, i think this works, I'm not fully sure on the text in the middle, but i'm also not against it. Not sure what @dani-garcia thinks about this change?

Regarding the npm audit fix discussed somewhere else (can't find it right now that quickly for some reason).
I think it's good to remove this from our builds, to keep stuff more the same as upstream Bitwarden.

I'm fine if you want to remove this via this PR.

currently it fails (because it can't resolve the $tailwindcss reference)
but also in general we might not want to depend on automatic updating the
dependencies without checking them first.
@stefan0xC
Copy link
Contributor Author

okay, I've added a commit that removes the npm audit fix (so we can close #139)

Copy link
Owner

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks everyone!

@dani-garcia dani-garcia merged commit 73ceed3 into dani-garcia:master Aug 31, 2023
@stefan0xC stefan0xC deleted the rename-vaultwarden-web branch August 31, 2023 18:51
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.

4 participants