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

Bitwarden mailconfig #1370

Merged
merged 26 commits into from
Aug 17, 2020
Merged

Bitwarden mailconfig #1370

merged 26 commits into from
Aug 17, 2020

Conversation

szaimen
Copy link
Collaborator

@szaimen szaimen commented Aug 15, 2020

No description provided.

@szaimen
Copy link
Collaborator Author

szaimen commented Aug 15, 2020

Some comments:

  • What does adminSettings__admins= do? it gives admin acces to specified emailaddresses - should be configurable
  • is it really enough to just restart bitwarden after writing those information to the file? yes!
  • Those need to get configured:
globalSettings__mail__smtp__host=REPLACE
globalSettings__mail__smtp__port=587
globalSettings__mail__smtp__ssl=false
globalSettings__mail__smtp__username=REPLACE
globalSettings__mail__smtp__password=REPLACE
  • maybe additionally:

you could make it a simple yes/no question while recommending to not disable ssl trust verification. or just leave a note that the option exists.
by default, that line does not exist in the global.override.env file, so you could insert it with "false" as well, so that one wouldn't have to look it up in the bitwarden documentation first.

@szaimen
Copy link
Collaborator Author

szaimen commented Aug 16, 2020

I haven't tested this but it should already work.

apps/tmbitwarden.sh Outdated Show resolved Hide resolved
menu/additional_apps.sh Outdated Show resolved Hide resolved
@enoch85
Copy link
Member

enoch85 commented Aug 16, 2020

Generally looks promising! Thank you!

@szaimen
Copy link
Collaborator Author

szaimen commented Aug 16, 2020

Since there is obviously an additional setting for starttls:
globalSettings__mail__smtp__startTls=true
bitwarden/server#451 (comment)
I would suggest to add this...

@enoch85
Copy link
Member

enoch85 commented Aug 16, 2020

I would suggest to add this...

Set it as default, and make it possible for the user to change port if wanted.

@szaimen
Copy link
Collaborator Author

szaimen commented Aug 16, 2020

I would suggest to add this...

Set it as default, and make it possible for the user to change port if wanted.

Already done by the latest commit 👍

@enoch85
Copy link
Member

enoch85 commented Aug 16, 2020

Are we ready to merge this?

@enoch85
Copy link
Member

enoch85 commented Aug 16, 2020

The text needs to be looked at, but I'm too tired now.

Code wise - looks good
Menu wise - looks good
Grammar - needs to be looked at

Thanks @szaimen! 🥇

Signed-off-by: enoch85 <github@hanssonit.se>
Signed-off-by: enoch85 <github@hanssonit.se>
Signed-off-by: enoch85 <github@hanssonit.se>
@enoch85
Copy link
Member

enoch85 commented Aug 17, 2020

Actually, a rebuild is not needed based on @mamama1's experience.

Done.

Haven't tested, but it looks good.

Would you say we can merge? Have you tested?

@enoch85 enoch85 marked this pull request as ready for review August 17, 2020 12:11
apps/bitwarden-mailconfig.sh Outdated Show resolved Hide resolved
@szaimen
Copy link
Collaborator Author

szaimen commented Aug 17, 2020

I've tested it on a test-VM with non-working bitwarden, so cannot say for sure, that everything works.
But at least the input and write process works. 👍

apps/bitwarden-mailconfig.sh Outdated Show resolved Hide resolved
Signed-off-by: enoch85 <github@hanssonit.se>
Signed-off-by: enoch85 <github@hanssonit.se>
Signed-off-by: enoch85 <github@hanssonit.se>
@enoch85 enoch85 merged commit dbd07a7 into master Aug 17, 2020
@enoch85 enoch85 deleted the bitwarden-mailconfig branch August 17, 2020 19:42
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.

None yet

2 participants