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

Editable config #209

Merged
merged 29 commits into from
Dec 21, 2022
Merged

Editable config #209

merged 29 commits into from
Dec 21, 2022

Conversation

wuan
Copy link
Contributor

@wuan wuan commented Dec 13, 2022

  • Introduced getIt service locator and streaming shared preferences.
  • Replace inheritance from Preferences object
  • Add an Widget to edit Preference Strings including one or more default values and validation.
  • Fix Version display
  • Create Config object from Preferences for every action
  • In order to allow using an up to date package info the ffi dependency of the submodule has been updated.

Fixes #206


Code Review Checklist

  • Description accurately reflects what changes are being made.
  • Description explains why the changes are being made (or references an issue containing one).
  • The PR appropriately sized.
  • New code has enough tests.
  • New code has enough documentation to answer "how do I use it?" and "what does it do?".
  • Existing documentation is up-to-date, if impacted.

@wuan wuan marked this pull request as draft December 13, 2022 22:05
@wuan wuan force-pushed the editable_config branch 5 times, most recently from a01c191 to 2a44ca1 Compare December 14, 2022 00:16
@wuan wuan marked this pull request as ready for review December 14, 2022 01:06
@donpui
Copy link
Contributor

donpui commented Dec 18, 2022

We need to update copy/paste/selectall design as it is more actual here and now looks unreadable in at least android:
image

@donpui
Copy link
Contributor

donpui commented Dec 18, 2022

One more thing, during Security Audit, one finding was that the code by default has unsecure connection urls. During mitigation of issue, we agreed that when we will make url editable, we should either notify users if they use unsecure connection either deny to enter:
Original OP task: https://leastauthority.openproject.com/projects/destiny/work_packages/1325/activity

Maybe we could go minimum, and show additional text if user enters ws:/ .

@donpui
Copy link
Contributor

donpui commented Dec 18, 2022

Overall looks good, we would need to make more testing on all platforms.

Optional thing, if I set wrong address, we should friendly message that destiny could not connect server, but no details to view what is exactly wrong (it can be mistype in url, not available port and etc..) on sender and receiver side.

@wuan
Copy link
Contributor Author

wuan commented Dec 18, 2022

We need to update copy/paste/selectall design as it is more actual here and now looks unreadable in at least android:

We did not declare that our color scheme is of type "dark". This should be fixed now.

@meejah
Copy link
Collaborator

meejah commented Dec 19, 2022

we should either notify users if they use unsecure connection either deny to enter:

There are currently two well-known public mailbox servers, and one of them uses ws:// only. So if you deny them from editing that, they can't use the software with the default server. If you warn them, what will the warning say that they can meaningfully take action upon?

@wuan
Copy link
Contributor Author

wuan commented Dec 19, 2022

No windows version works.

Hope you meant "Now" ;-)

@wuan
Copy link
Contributor Author

wuan commented Dec 19, 2022

we should either notify users if they use unsecure connection either deny to enter:

There are currently two well-known public mailbox servers, and one of them uses ws:// only. So if you deny them from editing that, they can't use the software with the default server. If you warn them, what will the warning say that they can meaningfully take action upon?

Then we need to separate warning from validation.

@meejah
Copy link
Collaborator

meejah commented Dec 19, 2022

Then we need to separate warning from validation.

Yeah I think that's what @donpui was suggesting: but what, why and how will a user take meaningful action.

That is, there's currently really only two things they might type in that field: our mailbox (the default) and Brian's mailbox. And now you want to "warn" them about something to do with the public default sever, but it's not clear what or why to me...

@meejah
Copy link
Collaborator

meejah commented Dec 19, 2022

We can add simple verification of the response being an URL for now. We have to be more verbose regarding errors which happen during a transmission attempt.

I think what @donpui was getting at here is that we could test whether the URL the user typed in is immediately reachable and speaks the right protocol -- thus saving them from a bunch of "couldn't send file" etc errors when they actually do get around to trying it.

(Obviously, this could be a followup ticket .. I'll add a comment to #206 though).

@wuan wuan mentioned this pull request Dec 20, 2022
@donpui
Copy link
Contributor

donpui commented Dec 21, 2022

There are currently two well-known public mailbox servers, and one of them uses ws:// only. So if you deny them from editing that, they can't use the software with the default server. If you warn them, what will the warning say that they can meaningfully take action upon?

We can warn the same as browsers does, only change text to be more correct:
image

To my understanding, unencrypted traffic expose at least app_id, nameplate, maybe something more (if someone can elaborate here)

Copy link
Contributor

@donpui donpui 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 and works, fine from my side. Warning notification can be discussed and implemented separately.

@wuan
Copy link
Contributor Author

wuan commented Dec 21, 2022

Looks good and works, fine from my side. Warning notification can be discussed and implemented separately.

Whenever you are ready. There will be some conflicts with the iOS changes, but I can help to resolve them.

@donpui
Copy link
Contributor

donpui commented Dec 21, 2022

Whenever you are ready. There will be some conflicts with the iOS changes, but I can help to resolve them.

Lets merge and I will try update iOS. After iOS PR review, we could release new version.

@wuan wuan merged commit 87bfcf3 into LeastAuthority:main Dec 21, 2022
@wuan
Copy link
Contributor Author

wuan commented Dec 21, 2022

Whenever you are ready. There will be some conflicts with the iOS changes, but I can help to resolve them.

Lets merge and I will try update iOS. After iOS PR review, we could release new version.

Let me know when I can help you.

@meejah
Copy link
Collaborator

meejah commented Dec 21, 2022

We can warn the same as browsers does, only change text to be more correct:

What are we warning about, though? None of that applies here.

I believe the point of Brian running the public server on ws:// is to "prove" that the server doesn't have any advantage attacking clients. That is, you're only revealing (to network listeners) those things you'd reveal to the server anyway -- and that's okay! It's still secure!

I suppose another way to state this is: what attacks are you actually preventing by using wss:// for the mailbox communications?

(I believe the answer is: none, and if the answer isn't "none" then we should probably also look at fixing something in the protocol and/or server).

donpui added a commit that referenced this pull request Dec 22, 2022
* editable config

* do not use inherited preference class

* expand defaults for transit relay

* add cancel button and update dart_wormhole_william

* improve styling

* fix formatting

* combine header with value and edit button.

* Derive themes and use editable fields for desktop

* remove Fimber logging

* fix declaration of dark theme

* cleanup

* Improve prefs editor layout

* Do not update preference when choosing default value

* add widget test

* also test multiple defaults

* formatting

* Fix Version for Targets which do not define versionCode

* format

* add configurable validation

* Allow ws scheme

* add test for Version

* format

* add generated mocks file

* cleanup

* fix preference text overflow

* update dart module

* fix not handling Uri parsing exceptions

* trim input strings

Co-authored-by: Donatas Puidokas <104410044+donpui@users.noreply.github.com>
donpui added a commit that referenced this pull request Jan 19, 2023
* Adding iOS build changes to experment

* Some changes

* Update lock

* Update experments

* Experments

* Progress

* Adjusted to iOS screens

* Fixing Download, Select File functions, adding icon

* Added select media for iOS

* Cleanup on IPA validation

* Cleanup and adding stage

* Small double button fix and documentation

* Small fixes for button and file selector

* Add push capabilities

* Update flutter-intro-slider to newest commit

* Added resource and debug label parameter

* Update submodules and lock files

* Editable config (#209)

* editable config

* do not use inherited preference class

* expand defaults for transit relay

* add cancel button and update dart_wormhole_william

* improve styling

* fix formatting

* combine header with value and edit button.

* Derive themes and use editable fields for desktop

* remove Fimber logging

* fix declaration of dark theme

* cleanup

* Improve prefs editor layout

* Do not update preference when choosing default value

* add widget test

* also test multiple defaults

* formatting

* Fix Version for Targets which do not define versionCode

* format

* add configurable validation

* Allow ws scheme

* add test for Version

* format

* add generated mocks file

* cleanup

* fix preference text overflow

* update dart module

* fix not handling Uri parsing exceptions

* trim input strings

Co-authored-by: Donatas Puidokas <104410044+donpui@users.noreply.github.com>

* Stage_la updated

* Fix small mixtakes after merge

* Added notes regarding ios and some typos fix

* Ios build improvements (#219)

* TEMP iOS build improvements

* Saving working changes for iphonesimulator

* Cleanup

* Cleanup and added documentation

* TEMP

* Plugin update

* Adding simple CI build for iOS

* Adding CI build for iOS

* Update plugin

* fixup! Merge remote-tracking branch 'origin/main' into ios-build-improvements

* fixup! Update plugin

* iOS lockfile updates

* Update plugin

Co-authored-by: El-Hassan Wanas <wanas@leastauthority.com>

* Update submodule

* Revert "Update dart submodule"

This reverts commit 76982bdd7d86bd981c99895b692586cd12198c15.

* Update submodule

* Update version to 1.0.3

* Fix styling issue

* Add wakelock to keep screen on (#215)

* Fix static error after update branch

Co-authored-by: Andreas Würl <andi@tryb.de>
Co-authored-by: El-Hassan Wanas <wanas@leastauthority.com>
@wuan wuan deleted the editable_config branch January 21, 2023 10:18
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.

Make Magic Wormhole Backend configuration editable
4 participants