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

fix: report correct state of feature flags on TUI #6611

Closed
ossdhaval opened this issue Nov 22, 2023 · 15 comments · Fixed by #6769
Closed

fix: report correct state of feature flags on TUI #6611

ossdhaval opened this issue Nov 22, 2023 · 15 comments · Fixed by #6769
Assignees
Labels
area-documentation Documentation needs to change as part of issue or PR comp-jans-auth-server Component affected by issue or PR comp-jans-config-api Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality needs-discussion Issue or PR needs discussion during triage meeting priority-3 Issue or PR is relevant to core functions, but does not impede progress. Important, but not urgent
Milestone

Comments

@ossdhaval
Copy link
Contributor

ossdhaval commented Nov 22, 2023

On a fresh install of Janssen Server(build: 1.0.21-nightly), feature flags are shown as below in TUI.

image

All checkboxes are unchecked. Giving an impression that all flags are disabled. At the same time, on AS all the flags are enabled by default.

We need to fix this inconsistency. TUI should always show which flags are enabled/disabled on AS. There has to be a way of TUI to know which all flags are available on AS and out of those, which ones are enabled at that point in time.

Context: discussion here

@ossdhaval ossdhaval added kind-bug Issue or PR is a bug in existing functionality area-documentation Documentation needs to change as part of issue or PR comp-jans-auth-server Component affected by issue or PR comp-jans-config-api Component affected by issue or PR needs-discussion Issue or PR needs discussion during triage meeting priority-3 Issue or PR is relevant to core functions, but does not impede progress. Important, but not urgent labels Nov 22, 2023
@ossdhaval
Copy link
Contributor Author

Tagging relevant members for discussion: @pujavs @yuriyz @devrimyatar @nynymike

@yuriyz
Copy link
Contributor

yuriyz commented Nov 22, 2023

If "featureFlags" configuration is absent or if it has no values then AS consider it as everything is enabled. To keep it clear to end user I guess we can do following:

  • All features enabled [x]
  • Enable only particular AS features:
    • End Session [ ]
    • other flags below....

What do you guys think ?

@devrimyatar
Copy link
Contributor

I think we are trying to determine which flags should be enabled when jans installed.

@yuriyz
Copy link
Contributor

yuriyz commented Nov 22, 2023

By default we don't specify any specific flag, Means no values for "featureFlags" which for AS means everything is enabled.
So default behavior is everything is enabled. It's under admin responsibility to start swithing on/off features if needed.

We can start maintaining that list and simply put all values there but then each time new feature flags is added we will need to remember to update it. Current approach is better because we don't need to maintain it. However I agree with Dhaval that we can't simply show everything unchecked while in fact everything is enabled. That's why I'm proposing solution described above with "All features enabled [x]" which should solve end-user confusion.

@yurem
Copy link
Contributor

yurem commented Nov 22, 2023

I agree with @yuriyz It's better to enable all by default and disable only specified. This will work fork new components as well

@devrimyatar
Copy link
Contributor

Okay, I am going to check all flags if config-api does not return property featureFlags

@ossdhaval
Copy link
Contributor Author

ossdhaval commented Nov 23, 2023

Does this not take away our capability to disable a particular flag at the start? we can still do it via a code change, correct?

I think it is important to be able to do so. Maybe, with the feature flags that we have currently, we are ok keeping everything enabled. In the future, we may have flags that are disabled by default and we want the admin to enable them explicitly.

We already have a user voicing concern on Gitter about this.

@nynymike can you opine on this?

@yuriyz
Copy link
Contributor

yuriyz commented Nov 23, 2023

Does this not take away our capability to disable a particular flag at the start?

It does not.

we can still do it via a code change, correct?

correct

@nynymike
Copy link
Contributor

nynymike commented Nov 23, 2023

I need to consider this. The actual defaults should be to max privacy and security... not just to all enabled or disabled.

@nynymike
Copy link
Contributor

image
See above. Although @ossdhaval TBH, I don't know what all these features do despite the docs. For example, is the id_generation endpoint still used by any services? For the JANS_CONFIGURATION endpoint, which .well-knowns does it disable? All of them? What's the difference between STAT and METRIC? And STATUS_SESSION v. ACTIVE_SESSION ?

@yuriyz
Copy link
Contributor

yuriyz commented Nov 24, 2023

For the JANS_CONFIGURATION endpoint, which .well-knowns does it disable? All of them?

It disables whole ./well-known endpoint. AS stops returning replies

What's the difference between STAT and METRIC?

  • STAT is for /stat endpoint which returns MAU, tokens usage etc.
  • METRIC - Enable/Disable metric reporter feature. Its original event reporter, e.g. user authorized successfully or failed or client is registered.

STATUS_SESSION v. ACTIVE_SESSION ?

@ossdhaval
Copy link
Contributor Author

Hi @nynymike I'll add more details to feature flag descriptions. I anyway have to update the default values for each flag.

@yuriyz As suggested by @nynymike in the above picture, can you make code changes so these flags are disabled by default?

@yuriyz
Copy link
Contributor

yuriyz commented Nov 27, 2023

Yes

@yuriyz
Copy link
Contributor

yuriyz commented Nov 28, 2023

Feature flags state is set according to list provided above by @nynymike .
TUI will reflect state thus we can close discussion about absent "featureFlags" config.

@moabu moabu added this to the 1.0.21 milestone Nov 30, 2023
@ossdhaval
Copy link
Contributor Author

I was checking the new feature flag screen behavior after we merged and fixed this issue and #6779. What I see on the TUI is same as before. Is there something that we still need to do?

cc: @devrimyatar @yuriyz @pujavs

devrimyatar pushed a commit that referenced this issue Dec 30, 2023
…cussed in #6611 (#6769)

#6611

Signed-off-by: YuriyZ <yzabrovarniy@gmail.com>
Signed-off-by: Mustafa Baser <mbaser@mail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-documentation Documentation needs to change as part of issue or PR comp-jans-auth-server Component affected by issue or PR comp-jans-config-api Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality needs-discussion Issue or PR needs discussion during triage meeting priority-3 Issue or PR is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
7 participants