fix: when setting isOidcEnabled is false override env variable #4403
Conversation
There was a problem hiding this comment.
Since OIDC is not available in previews I can't fully test this.
The logic for handling the setting & environment property if it is the setting is not present seems fine, but I see one edge case that might be confusing. If OIDC was configured in the past and then removed the OIDC setting remains in the settings database.
I think if MOLGENIS_OIDC_CLIENT_ID is not set we should remove the IS_OIDC_ENABLED setting from the settings database.
I think you are saying that if the oidc settings is clearly incomplete then it should be ignored, logging an error in startup script. That I suppose is a bit of a challenge because when settings change then the oidc should reload too. I pushed an update that provides some useful info on these missing parameters but I am not sure it is clear now. |
There was a problem hiding this comment.
This latest change can leave you in a situation where it is very hard to recover from.
I would do it the other way around. isOidcEnabled only functioning as a override to temporarily disable OIDC without removing the configuration. If there is no OIDC configuration in the environment then the isOidcEnabled flag is removed from the settings database.
But that would lead to weird dissapearing if a user made the setting. Issen't that confusing? |
backend/molgenis-emx2-sql/src/main/java/org/molgenis/emx2/sql/SqlDatabase.java
Outdated
Show resolved
Hide resolved
|
* When OIDC setting is available use it * provide an error state if parameters are missing * Load db settings in security config * Show error message when setting oidc to true when settings are incomplete --------- Co-authored-by: Morris Swertz <m.a.swertz@rug.nl>
Fixes #3619
What are the main changes you did:
how to test:
todo: