libkeepass / pykeepass

Python library to interact with keepass databases (supports KDBX3 and KDBX4)
https://pypi.org/project/pykeepass/
GNU General Public License v3.0
411 stars 96 forks source link

entry: Do not set protected=True on setters #381

Closed A6GibKm closed 5 months ago

A6GibKm commented 7 months ago

Setting a property would call _set_string_field which by default would set the protected status of the attribute as "True".

In order to verify in tests if a property is protected we add a private method _is_property_protected which shares the code with is_custom_property_protected.

Fixes: https://github.com/libkeepass/pykeepass/issues/376

A6GibKm commented 7 months ago

cc: @julianfairfax

A6GibKm commented 7 months ago

The newly introduced tests fails if one revers the commit.

Evidlo commented 6 months ago

The test description doesn't match what I'm seeing in the test. Shouldn't we set a property and check that the Protected attribute has not changed?

This would imply that the _set_string_field(..., protected=None) leaves Protected as is.

I guess something like this on line 107:

# copy Protected attribute from existing property
protected = self._is_protected_property(field) if protected is None else protected
A6GibKm commented 6 months ago

The test description doesn't match what I'm seeing in the test. Shouldn't we set a property and check that the Protected attribute has not changed?

This would imply that the _set_string_field(..., protected=None) leaves Protected as is.

I guess something like this on line 107:

# copy Protected attribute from existing property
protected = self._is_protected_property(field) if protected is None else protected

Yes, this prob would work best. But I have to say having a Option<bool> (in rust notation) feels weird in python.

Evidlo commented 6 months ago

It boils down to whether the default behavior should be preserving attribute values when setting properties that already exist.

A6GibKm commented 6 months ago

I implemented the tri state for protected, do note that we set the password and OTP to protected=True when the values were initialized by the library.