Feature/default expiry #161

tchbla · 2023-12-15T11:37:25Z

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 · 2023-12-15T11:51:41Z

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?

Luzifer

I still don't see why we need to increase complexity to the expiry configuration. Having the server select the maximum and also default and let the user specify smaller retention values is totally fine IMHO…

Luzifer · 2023-12-15T11:53:40Z

src/components/create.vue

@@ -153,22 +155,18 @@ export default {
    },

    expiryChoices() {
-      const choices = [{ text: this.$t('expire-default'), value: null }]
+      const choices = [{
+        text: this.$t('expire-default') + " (" + this.getExpiryLabel(defaultSecretExpire) + ")",


This needs to be properly adjusted in the translation, not just appended.

Also the label does not solve the issue when values other than 86400, 3600, 60 are set. 129600 will be 2 Days which is wrong.

Luzifer · 2023-12-15T11:55:56Z

src/components/create.vue

+      if (duration >= 86400) {
+          text = this.$tc('expire-n-days', Math.round(duration / 86400))
+        } else if (duration >= 3600) {
+          text = this.$tc('expire-n-hours', Math.round(duration / 3600))
+        } else if (duration >= 60) {
+          text = this.$tc('expire-n-minutes', Math.round(duration / 60))
+        } else if (duration > 0) {
+          text = this.$tc('expire-n-seconds', duration)
+        } else {
+          text = this.$t('never')
+        }


Fix formatting

Luzifer · 2023-12-15T11:56:26Z

src/components/create.vue

-        } else {
-          option.text = this.$tc('expire-n-seconds', choice)
-        }
+        option.text = this.getExpiryLabel(choice)


Can be joined with line above

Luzifer · 2023-12-15T11:57:48Z

i18n.yaml

@@ -17,6 +17,7 @@ reference:
    expire-n-hours: '{n} hour | {n} hours'
    expire-n-minutes: '{n} minute | {n} minutes'
    expire-n-seconds: '{n} second | {n} seconds'
+    never: 'never'


Rename: expire-never (though as this is wrong on a functionality level, we don't need that)

tchbla · 2023-12-15T12:14:12Z

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

What exactly do they ask?

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

Luzifer · 2023-12-15T12:17:19Z

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 · 2023-12-15T12:25:57Z

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 · 2023-12-15T12:31:06Z

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?

The secret is encrypted
The server admin set a (hopefully) sane default to keep the secret in the storage
The behavior matches creating a secret via API without setting an expiry

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?

…= max expiry

Luzifer · 2024-02-14T09:31:16Z

Closing this PR:

questions are not getting answered, no progress for two months
while one feature is a small frontend improvement which makes sense the other change in this PR feels forced upon the project with no real need and even though asked multiple times in the related issue and this PR there was no answer provided what issue this change solves

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.

tchbla added 4 commits December 15, 2023 10:33

renamed SecretExpiry to MaxSecretExpiry

a3554d3

add cfg option DefaultSecretExpire

4042cd0

add translations for never

a7acb27

moved option.text describer to a function and added never

140fa57

Luzifer requested changes Dec 15, 2023

View reviewed changes

Luzifer linked an issue Dec 15, 2023 that may be closed by this pull request

add a value to "Default" expiry #153

Open

tchbla added 4 commits December 18, 2023 11:59

if default expiry is not defined but max expiry, make default expiry …

a514b0b

…= max expiry

fix translations, add duration field into translation

3ba9a95

clean code: merge option.text into option definition

bbf5b70

fix intendation

34057de

Luzifer closed this Feb 14, 2024

Luzifer mentioned this pull request Apr 11, 2024

add a value to "Default" expiry #153

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/default expiry #161

Feature/default expiry #161

tchbla commented Dec 15, 2023

Luzifer commented Dec 15, 2023

Luzifer left a comment

Luzifer Dec 15, 2023

Luzifer Dec 15, 2023 •

edited

Loading

Luzifer Dec 15, 2023

Luzifer Dec 15, 2023

Luzifer Dec 15, 2023

tchbla commented Dec 15, 2023

Luzifer commented Dec 15, 2023

tchbla commented Dec 15, 2023

Luzifer commented Dec 15, 2023

Luzifer commented Feb 14, 2024

Feature/default expiry #161

Feature/default expiry #161

Conversation

tchbla commented Dec 15, 2023

Luzifer commented Dec 15, 2023

Luzifer left a comment

Choose a reason for hiding this comment

Luzifer Dec 15, 2023

Choose a reason for hiding this comment

Luzifer Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Luzifer Dec 15, 2023

Choose a reason for hiding this comment

Luzifer Dec 15, 2023

Choose a reason for hiding this comment

Luzifer Dec 15, 2023

Choose a reason for hiding this comment

tchbla commented Dec 15, 2023

Luzifer commented Dec 15, 2023

tchbla commented Dec 15, 2023

Luzifer commented Dec 15, 2023

Luzifer commented Feb 14, 2024

Luzifer Dec 15, 2023 •

edited

Loading