refactor sso provider selection #525
Conversation
avoid using boolean values in envvars as yaml, ansible, .env evaluate them differently
There was a problem hiding this comment.
The code changes look ok to me. However, I recommend adding an explanation to Tunnistus vs Tunnistamo to the README. Those words are so similar that they are easily confused, and even when not, it is not clear what the difference is and what they are referring to. Will this documentation be added now or in a later PR?
| @@ -31,6 +33,9 @@ const loginProviderTunnistusProperties: LoginProviderProps = { | |||
| sessionPollerSettings: { pollIntervalInMs: 300000 } // 300000ms = 5min | |||
| }; | |||
|
|
|||
| export const useTunnistamoOpenIdConnect = import.meta.env.VITE_USE_TUNNISTAMO_OPENID_CONNECT === 'true' || import.meta.env.VITE_USE_TUNNISTAMO_OPENID_CONNECT === true; | |||
| export const oidcProviderName: OidcProviderName = import.meta.env.VITE_OIDC_PROVIDER || 'tunnistus'; | |||
There was a problem hiding this comment.
'tunnistamo' string is assigned to an env var, but 'tunnistus' is just a plain string without a reference to a variable. Looks a bit odd, but is there a reason for it? Could it be a bit cleaner if 'tunnistus' could also be defined in variable?
There was a problem hiding this comment.
It is defined in VITE_OIDC_PROVIDER, || 'tunnistus' is a fallback default value.
How do you suggest to change it?
There was a problem hiding this comment.
It could be defined as DEFAULT_PROVIDER_NAME. But I'm not sure, doesn't feel necessary, because when I think about it more, the string 'tunnistus' is used only here. Maybe defining it in a variable becomes necessary only if the exact 'tunnistus' string is used in several places, to avoid the pitfall of mistyping. So maybe there is no need to define it in a variable in these changes.
I don't intend to document it. My reasoning is that it should be pretty clear from the |
| @@ -31,6 +33,9 @@ const loginProviderTunnistusProperties: LoginProviderProps = { | |||
| sessionPollerSettings: { pollIntervalInMs: 300000 } // 300000ms = 5min | |||
| }; | |||
|
|
|||
| export const useTunnistamoOpenIdConnect = import.meta.env.VITE_USE_TUNNISTAMO_OPENID_CONNECT === 'true' || import.meta.env.VITE_USE_TUNNISTAMO_OPENID_CONNECT === true; | |||
| export const oidcProviderName: OidcProviderName = import.meta.env.VITE_OIDC_PROVIDER || 'tunnistus'; | |||
There was a problem hiding this comment.
It could be defined as DEFAULT_PROVIDER_NAME. But I'm not sure, doesn't feel necessary, because when I think about it more, the string 'tunnistus' is used only here. Maybe defining it in a variable becomes necessary only if the exact 'tunnistus' string is used in several places, to avoid the pitfall of mistyping. So maybe there is no need to define it in a variable in these changes.
|
There is a type that has both options, it should check for mistyping. Ofc it wont do that in envvars or dotenv files. |
avoid using boolean values in envvars as yaml, ansible, .env evaluate them differently