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

getAppAuthMode does not evaluate configuration values #86

Closed
pbolduc opened this issue Nov 23, 2022 · 2 comments · Fixed by #245
Closed

getAppAuthMode does not evaluate configuration values #86

pbolduc opened this issue Nov 23, 2022 · 2 comments · Fixed by #245
Labels
bug Something isn't working

Comments

@pbolduc
Copy link
Contributor

pbolduc commented Nov 23, 2022

Describe the bug

The getAppAuthMode function does not evaluate the configuration. It only checks if the configuration is defined. This causes wrong evaluation if values are set but set to false.

getAppAuthMode() {
const basicAuth = config.has('basicAuth.enabled');
const oidcAuth = config.has('keycloak.enabled');
if (!basicAuth && !oidcAuth) return AuthMode.NOAUTH;
else if (basicAuth && !oidcAuth) return AuthMode.BASICAUTH;
else if (!basicAuth && oidcAuth) return AuthMode.OIDCAUTH;
else return AuthMode.FULLAUTH; // basicAuth && oidcAuth
},

In the above function, the code tests if a config value exists instead of getting config.

So if I define KC_ENABLED=false, the service starts up with OIDC configuration enabled. Since I do not have OIDC configuration values, the pod dies saying Configuration property \"keycloak.clientId\" is not defined

To Reproduce

Steps to reproduce the behavior:

  1. Deploy service with KC_ENABLED=false set but do not set any other KC vars
  2. See error at startup saying keycloak.clientId is not defined.

Expected behavior

If I configured KC_ENABLED=false, it should not expect keycloak configuration to be defined.

Screenshots

msedge_1m1fZtonRb

msedge_PFwJ0LSkYU

Desktop (please complete the following information):

  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context

@TimCsaky
Copy link
Contributor

You make a good point.. and it's a usability issue we want to resolve.
currently, the config values are parsed as strings. so "false" = true`
it appears recent versions of the node-config library allow us to parse booleans.
We're going to look into this. but I expect it will be a v.0.5 change.

@jujaga jujaga added the bug Something isn't working label Mar 16, 2023
@pbolduc
Copy link
Contributor Author

pbolduc commented Mar 17, 2023

config.has('basicAuth.enabled') only checks if there is a value in basicAuth.enabled, but does not check the value. You could change it to:

const basicAuth = config.has('basicAuth.enabled') && config.basicAuth.enabled === true

It is like having a configuration value format.hard-disk and I have it set to false, but boom, my disk gets formatted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants