Allow to disable os.ExpandEnv for storage + connector configs by env variable DEX_EXPAND_ENV = false#1902
Conversation
| // True by default, to avoid a breaking change. | ||
| // False if the env variable "DEX_EXPAND_ENV" is set to "0". | ||
| func isExpandEnvEnabled() bool { | ||
| return os.Getenv("DEX_EXPAND_ENV") != "0" |
There was a problem hiding this comment.
How about using ParseBool instead? Then a whole bunch of other boolean-like values can be accepted. false/true comes to mind as better alternatives to 0/1 and I'd also use those in the documentation instead of 0/1.
Obviously this means that you have to look up the env first.
Also, thinking about the variable name: since this is true by default, I wonder if it would make sense to use DEX_NO_EXPAND_ENV instead.
There was a problem hiding this comment.
How about using ParseBool instead?
Sounds good, will do that.
I wonder if it would make sense to use DEX_NO_EXPAND_ENV instead.
I'm not a big fan of this, for the following reasons:
- Consider
DEX_NO_EXPAND_ENV=false, which is a double negation. I wanted to avoid that case. - In case that we later change the default to "don't expand", we would have to change back the variable name
DEX_NO_EXPAND_ENV->DEX_EXPAND_ENV.
E.g. the OpenLDAP Docker image also uses LDAP_...=false instead of LDAP_NO_...=true:
LDAP_READONLY_USER: Add a read only user. Defaults to false
LDAP_TLS: Add openldap TLS capabilities. [...] Defaults to true
Definitely you will also find many software projects that use your suggestion ..._NO_..., just wanted to show one example for my suggestion.
But if you insist, I can still change it.
heidemn-faro
changed the title
a383803 to
c6d8035
Compare
|
@sagikazarmark please take another look, and let me know if further changes are required. |
| // Default, for downwards-compatibility: DEX_EXPAND_ENV = true | ||
| return true, nil | ||
| } | ||
| return strconv.ParseBool(dexExpandEnv) |
There was a problem hiding this comment.
The reason I thought a negated variable name might be better is because variable expansion should be true by default, even if the content of the variable is considered invalid.
I agree it sounds bad, but I think we can keep DEX_EXPAND_ENV: I think we should handle the error returned by ParseBool: if there is an error, it should return true, otherwise return the parsed value. I think it's okay to silently fall back to true when the value of the variable is empty for example or some gibberish.
There was a problem hiding this comment.
Oh no, then I spent too much effort on the tests etc.... But you're right, silently using the default seems fine.
c6d8035 to
8481871
Compare
…variable DEX_EXPAND_ENV = false Signed-off-by: Martin Heide <[email protected]>
8481871 to
4cb5577
Compare
|
@sagikazarmark please take a 3rd look :-) Simplified as requested. |
|
Thanks for reviewing @sagikazarmark , and happy new year! |
The official docker release for this release can be pulled from ``` ghcr.io/dexidp/dex:v2.28.0 ``` **Features:** - Add c_hash to id_token, issued on /auth endpoint, when in hybrid flow (dexidp#1773, @HEllRZA) - Allow configuration of returned auth proxy header (dexidp#1839, @seuf) - Allow to disable os.ExpandEnv for storage + connector configs by env variable DEX_EXPAND_ENV = false (dexidp#1902, @heidemn-faro) - Added the possibility to activate lowercase for UPN-Strings (dexidp#1888, @VF-mbrauer) - Add "Cache-control: no-store" and "Pragma: no-cache" headers to token responses (dexidp#1948, @nabokihms) - Add gomplate to the docker image (dexidp#1893, @nabokihms) - Graceful shutdown (dexidp#1963, @nabokihms) - Allow public clients created with API to have no client_secret (dexidp#1871, @spohner) **Bugfixes:** - Fix the etcd PKCE AuthCode deserialization (dexidp#1908, @bnu0) - Fix garbage collection logging of device codes and device request (dexidp#1918, @nabokihms) - Discovery endpoint contains updated claims and auth methods (dexidp#1951, @nabokihms) - Return invalid_grant error if auth code is invalid or expired (dexidp#1952, @nabokihms) - Return an error to auth requests with the "request" parameter (dexidp#1956, @nabokihms) **Minor changes:** - Change default themes to light/dark (dexidp#1858, @nabokihms) - Various developer experience improvements - Dependency upgrades - Tons of small fixes and changes
3 participants
Overview
Closes #1876
What this PR does / why we need it
See issue + release note below.
Special notes for your reviewer
@sagikazarmark I guess this one is for you, since you commented on the issue.
Does this PR introduce a user-facing change?