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

Add Browser Integration to Group Edit page #4180

Merged

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Jan 13, 2020

Type of change

  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

Adds Browser Integration page to Group Edit page. This allows to set the following options for all entries in the group and its subgroups:

  • Skip Auto-Submit for entries
  • Hide entries from the browser extension
  • Use entries only with HTTP Basic Auth
  • Do not use entries with HTTP Basic Auth

Checkboxes are disabled if group level settings are used.

Fixes #1789.
Fixes #3998.

Screenshots

Screenshot 2021-08-10 at 12 37 21

Screenshot 2021-08-10 at 12 24 45

Testing strategy

Manually.

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@varjolintu varjolintu added this to the v2.6.0 milestone Jan 13, 2020
@varjolintu varjolintu force-pushed the feature/add_bi_page_to_groups branch 5 times, most recently from 4fc5fc8 to 130190b Compare May 15, 2020 12:57
@varjolintu varjolintu force-pushed the feature/add_bi_page_to_groups branch from 130190b to 6215d4f Compare May 24, 2020 07:02
@droidmonkey droidmonkey modified the milestones: v2.6.0, v2.6.1 Jun 4, 2020
@droidmonkey
Copy link
Member

I'm saving this for a future release

@droidmonkey droidmonkey modified the milestones: v2.6.1, v2.7.0 Jul 21, 2020
@droidmonkey
Copy link
Member

@varjolintu do we still want to incorporate this?

@varjolintu
Copy link
Member Author

@droidmonkey Sure, why not. I can make a rebase.

@varjolintu varjolintu force-pushed the feature/add_bi_page_to_groups branch 2 times, most recently from 7253206 to 41df1c1 Compare February 7, 2021 08:32
@droidmonkey
Copy link
Member

The problem with this implementation is that new entries added to the group do not have the group "desired" settings applied to it. Basically the group settings are a one-shot deal and reapplying them to child entries requires editing the group, checking the box, and pressing OK. This is obviously not the desired intention of these settings.

I think it would be better to store these settings at the group level just like auto-type does. The browser plugin would then exclude entries from groups that have these specific settings applied, depending on the context. This also prevents issues when entries are moved into/out of groups and adding entries to a group with these desired settings.

@droidmonkey droidmonkey force-pushed the feature/add_bi_page_to_groups branch from 41df1c1 to 5fd740c Compare August 8, 2021 17:01
@varjolintu
Copy link
Member Author

@droidmonkey Good idea to move the option to group level instead.

@varjolintu
Copy link
Member Author

Entry settings also need to be changed to tristate (I think checkboxes will be enough) because the state can be inherited from the group.

@droidmonkey
Copy link
Member

droidmonkey commented Aug 9, 2021

My opinion is that group settings override all entries. We make things more complicated by allowing exceptions all over the place.

If anything we should be disabling the entry level checkboxes if the group settings are set

You can disable them and have a tool tip that says the setting is controlled at the group level

@varjolintu
Copy link
Member Author

My opinion is that group settings override all entries. We make things more complicated by allowing exceptions all over the place.

If anything we should be disabling the entry level checkboxes if the group settings are set

You can disable them and have a tool tip that says the setting is controlled at the group level

This was the other option I thought about. However, if group settings are inherited but the setting value is default, we should allow the entry level settings. Disabling them only when the value is set or inherited value differs from the default should work.

@droidmonkey
Copy link
Member

Right my assumption is these group settings are meant to be blanket settings across all entries in the group. They should be worded such that they apply to every entry in the group. If the group settings aren't changed then the behavior is as before

@varjolintu varjolintu force-pushed the feature/add_bi_page_to_groups branch from 5fd740c to ccb1456 Compare August 10, 2021 10:36
@varjolintu varjolintu force-pushed the feature/add_bi_page_to_groups branch from 03e92a5 to 22950ab Compare August 11, 2021 05:51
@varjolintu
Copy link
Member Author

You are connecting the signals before initializing the UI. The ui elements you connected are nullptr at time of connection.

Got it. This is good to go.

@droidmonkey
Copy link
Member

I like the current implementation, simple clean easy to understand. OK to merge this now?

@droidmonkey
Copy link
Member

I made some tweaks to wording and positioning of elements. Unfortunately we cannot use the "WriteTriState" into the kdbx, if you do that then those XML elements will be dropped by any client that is no KeePassXC (ie, it is not part of the standard). We need to use the Custom Data of the Group. The benefit to custom data is we can eliminate all the tristate code and polluting the Group class with browser settings.

@droidmonkey droidmonkey force-pushed the feature/add_bi_page_to_groups branch from 22950ab to 3d078b2 Compare August 22, 2021 02:38
@varjolintu
Copy link
Member Author

varjolintu commented Aug 22, 2021

I made some tweaks to wording and positioning of elements. Unfortunately we cannot use the "WriteTriState" into the kdbx, if you do that then those XML elements will be dropped by any client that is no KeePassXC (ie, it is not part of the standard). We need to use the Custom Data of the Group. The benefit to custom data is we can eliminate all the tristate code and polluting the Group class with browser settings.

Ah, ok. Got it. I thought "EnableAutoType" and "EnableSearching" with WriteTriState were also non-standard? This is why I used a similar method.

@varjolintu varjolintu force-pushed the feature/add_bi_page_to_groups branch 2 times, most recently from 4bcb79a to 4842d73 Compare August 23, 2021 08:14
@varjolintu
Copy link
Member Author

@droidmonkey This should be ready now. Hopefully there's no more findings.

@varjolintu varjolintu force-pushed the feature/add_bi_page_to_groups branch from 4842d73 to ace01e9 Compare September 21, 2021 13:27
src/core/Group.cpp Outdated Show resolved Hide resolved
src/core/Group.cpp Outdated Show resolved Hide resolved
src/core/Group.h Outdated Show resolved Hide resolved
src/format/KdbxXmlReader.cpp Outdated Show resolved Hide resolved
src/format/KdbxXmlWriter.cpp Outdated Show resolved Hide resolved
@droidmonkey
Copy link
Member

@varjolintu I made the changes necessary to this PR, do a double check for me please.

@varjolintu
Copy link
Member Author

varjolintu commented Oct 10, 2021

@varjolintu I made the changes necessary to this PR, do a double check for me please.

EDIT: All is working properly. Had some merge errors earlier.

@droidmonkey
Copy link
Member

I need to update the tests

@droidmonkey droidmonkey force-pushed the feature/add_bi_page_to_groups branch from 9a5e34e to 184b099 Compare October 11, 2021 03:26
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Attention: Patch coverage is 55.55556% with 92 lines in your changes missing coverage. Please review.

Project coverage is 63.61%. Comparing base (c7cdce6) to head (fdd6718).
Report is 591 commits behind head on develop.

Files with missing lines Patch % Lines
src/gui/group/EditGroupWidget.cpp 47.56% 43 Missing ⚠️
src/gui/entry/EditEntryWidget.cpp 73.75% 21 Missing ⚠️
src/browser/BrowserService.cpp 25.00% 15 Missing ⚠️
src/core/Group.cpp 35.00% 13 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4180      +/-   ##
===========================================
- Coverage    63.63%   63.61%   -0.02%     
===========================================
  Files          330      330              
  Lines        41646    41807     +161     
===========================================
+ Hits         26499    26595      +96     
- Misses       15147    15212      +65     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@droidmonkey droidmonkey force-pushed the feature/add_bi_page_to_groups branch from 184b099 to fdd6718 Compare October 11, 2021 03:43
@droidmonkey droidmonkey merged commit b6716bd into keepassxreboot:develop Oct 11, 2021
@varjolintu varjolintu deleted the feature/add_bi_page_to_groups branch October 11, 2021 12:55
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Browser pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a browser integration tab in folders Ability to restrict keepassxc-browser to specific sub-tree
4 participants