search

Luzifer / ots

One-Time-Secret sharing platform with a symmetric 256bit AES encryption in the browser
https://ots.fyi
Apache License 2.0
448 stars 69 forks source link

Feature/default expiry #161

Closed tchbla closed 7 months ago

tchbla commented 9 months ago

Hi @Luzifer

I found some time and prepared some changes regarding #153

Below my thoughts about your comments:

The default value is set through the SECRET_EXPIRY environment variable and is used by the API when there is no value for expiry set which is exactly what the frontend choice "Default Expiry" does: It does not specify any value and therefore leaves the choice to the server.

Yes, that is correct. The is able to pick shorter expiry values (if not disabled y cust.DisableExpiryOverride) but never longer periods.

Having two "defaults" is kinda confusing. On the one hand configuring the default expiry and then also configuring the default-frontend-expiry which is not the value of "Default Expiry" and also must match one of the software-defined choices not deriving by one second.

I renamed SecretExpiry to MaxSecretExpiry and added DefaultSecretExpiry to make this less confusing. In my opinion that is even more clear than only one SecretExpiry variable.

That's not that easy: How would you display SECRET_EXPIRY=263543? In Go notation that's 73h12m23s or assuming 86400s as a day "3 Days, 1 Hour, 12 Minutes, 23 Seconds". Sure, that's a constructed case but something to be taken into account. The full field text would then be "Default Expiry (3 Days, 1 Hour, 12 Minutes, 23 Seconds)" which is "a little" too long…

This problem does also occure with every other default value that can be set in the frontend.
You already included a tie breaker method that breaks this into discrete day / hour / minute or second values depending how big they are. I added a "never" part to reflect unconfigured or "0" values.

In this case also the default value cannot be selected as it simply does not exist in the dropdown.

I added the defaultSecretExpiry as the preselected default.

The default value is null, which leaves the choice to the server (which can be infinite storage of the secret). Which also is difficult to display ("Default Expiry (Infinity)"? - too confusing for users)…

I called it "never" and find it very less confusing than "Default Expiry" without explanation.
On my server users already asked and raises ticket to ask about the "Default Expiry" value. My boss also told me that I have to add some notice... this is why I first came up with this quick but ugly overwriting the index.html and added custom javascript file to add this value to the frontend. But now the updated code in this PR does a better job (at least for my eyes 😄).

Luzifer commented 9 months ago

I renamed SecretExpiry to MaxSecretExpiry and added DefaultSecretExpiry to make this less confusing.

That's a breaking change.

I called it "never" and find it very less confusing than "Default Expiry" without explanation.

That's just plain wrong. Using the server default is not never.

but never longer periods.

That's exactly the intended behavior. You shall not be able to pick a longer value than the server allows.

This problem does also occure with every other default value that can be set in the frontend.

You cannot set arbitrary values in the frontend: Those are defined in the frontend source-code.

On my server users already asked and raises ticket to ask about the "Default Expiry" value.

What exactly do they ask?

tchbla commented 9 months ago

That's a breaking change.

yes

That's just plain wrong. Using the server default is not never.

okay in case there is no configured value it is never. otherwise if there is no default expiry set but a max expiry this value will become the default expiry.

That's exactly the intended behavior. You shall not be able to pick a longer value than the server allows.

yes, I didn't want to discredit this. This is the intended good behavior.

You cannot set arbitrary values in the frontend: Those are defined in the frontend source-code.

the customizte.yaml does offer this option image

What exactly do they ask?

Just "what is the default expiry time / why is it not displayed".

Luzifer commented 9 months ago

What exactly do they ask?

Just "what is the default expiry time / why is it not displayed".

Okay so to solve the issue you're having, simply modifying one translation string is sufficient. That wouldn't need a breaking change and would not throw off the limit in every installation.

tchbla commented 9 months ago

Okay so to solve the issue you're having, simply modifying one translation string is sufficient. That wouldn't need a breaking change and would not throw off the limit in every installation.

okay I got the point why it is bad to introduce this braking change that will cause installations that do not update this value to have no limit. could add some quirks to support also just SECRET_EXPIRY as before...

Just to change the translation will fix the unknown default expiry value but I had a nother request in mind. It should be possible to define a default value that is lower than the max value. Users tend to keep the default settings and so the most secrets will have the max secret expiry. With a default value it is possible to define for example a day as a default but allow for some usecases to set higher.

Luzifer commented 9 months ago

Users tend to keep the default settings and so the most secrets will have the max secret expiry. With a default value it is possible to define for example a day as a default but allow for some usecases to set higher.

What exactly does this solve?

So what's the benefit of pre-selecting another expiry just in the frontend (which is only one consumer to the API)?

You're presenting a feature-change / solution, not the issue you want to solve. So: What do you want to solve by splitting up max-retention and default-retention?

Luzifer commented 7 months ago

Closing this PR:

For me this feels like my questions and objections as a project owner are completely ignored. I still don't see the need for the bigger, (breaking) change and questions to understand the need were ignored.

Feel free to open a new PR containing only the frontend changes to display the default expiry time and leave out the other unrelated changes from that PR.