-
Notifications
You must be signed in to change notification settings - Fork 179
Enable update_user_role property for Docker Image #350
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
base: master
Are you sure you want to change the base?
Conversation
| \"authentication_method\": \"${OIDC_AUTH_METHOD}\", | ||
| \"redirect_uri\": \"${OIDC_REDIRECT_URI}\" | ||
| \"redirect_uri\": \"${OIDC_REDIRECT_URI}\", | ||
| + \"update_user_role\": ${OIDC_UPDATE_USER_ROLE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when this is empty? Is this valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referenced in:
https://github.com/MISP/MISP/blob/2.5/app/Plugin/OidcAuth/Lib/Oidc.php#L109
https://github.com/MISP/MISP/blob/2.5/app/Plugin/OidcAuth/Lib/Oidc.php#L240
The bit that matters for this is:
$this->getConfig('update_user_role', true)This uses the getConfig function from:
https://github.com/MISP/MISP/blob/2.5/app/Plugin/OidcAuth/Lib/Oidc.php#L541
If the value is not in the configuration file at all, then getConfig call below would default the value to true (which is what the container does today).
In the case where the env variable is empty coming from the container runtime, you are 100% right; this would cause the JSON being written to be invalid. We should probably use ${OIDC_UPDATE_USER_ROLE:=default_role_value} to account for this case, so this isn't a breaking change for existing configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, but does /var/www/MISP/tests/modify_config.php modify succeed when that is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does, if I'm reading the modify_config.php file correctly; otherwise the upstream project would have issues with the upgrades after the new property was added.
Using:
${OIDC_UPDATE_USER_ROLE:=true}
Should allow any potential inputs to pass as they currently do. There is a failure case in the original submission for an invalid json file if no value was set.
I'll give the following a test when I get back to a computer:
- input value set to true/false
- no input value supplied for OIDC_UPDATE_USER_ROLE
- no input value specified, but a value already set in an existing container (e.g. via a configmap in k8s)
Are there any other scenarios this needs to be tested under?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are sure that the default value is true, we can cut to the chase by creating defaults in the main entrypoint file (entrypoint.sh) so we are sure that the var is always initialized to a default value.
Also, we should update docker-compose.yml and template.env to document it.
This change lets users of the image define one new envars:
OIDC_UPDATE_USER_ROLE = true | false
This sets the update_user_role value in the MISP config.php that is used by newer versions to allow the management of user-roles to occur internal of the application vs through role mapping.
Why would you want this?
This is to bring the image params in-line with the allowed configuration of the parent MISP application.