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

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

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

A6GibKm
Copy link
Contributor

@A6GibKm A6GibKm commented Mar 6, 2024

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: #376

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Mar 6, 2024

cc: @julianfairfax

@A6GibKm
Copy link
Contributor Author

A6GibKm commented Mar 6, 2024

The newly introduced tests fails if one revers the commit.

@Evidlo
Copy link
Member

Evidlo commented Mar 11, 2024

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
Copy link
Contributor Author

A6GibKm commented Mar 11, 2024

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
Copy link
Member

Evidlo commented Mar 12, 2024

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

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: libkeepass#376
@A6GibKm A6GibKm force-pushed the fix-protected-true-all branch from ed311e7 to 498bfcd Compare March 19, 2024 21:40
@A6GibKm
Copy link
Contributor Author

A6GibKm commented Mar 19, 2024

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.

@Evidlo Evidlo merged commit 66bc409 into libkeepass:master Apr 9, 2024
5 checks passed
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.

All fields of items created in GNOME Secrets are hidden
2 participants