[Notifications] Check if secret key is internal before deleting it #5886
Conversation
Yacouby
force-pushed
the
ML-6936
branch
3 times, most recently
from
4cf7f99
to
5c40c5c
Compare
server/api/api/utils.py
Outdated
| @@ -429,7 +430,11 @@ def validate_and_mask_notification_list( | |||
| # validate notification schema | |||
| mlrun.common.schemas.Notification(**notification_object.to_dict()) | |||
|
|
|||
| notification_object.validate_notification_params() | |||
| # skip the params validation if the key is "secret" | |||
There was a problem hiding this comment.
Not sure I understand why. Also why not check this in the validation function?
There was a problem hiding this comment.
after a conversation with @quaark we decided that we should skip the validation when "secret" is present because it implies that the parameters are securely handled and should not be validated.
mlrun/model.py
Outdated
| # skip the secret_params validation if it contains the key "secret" since the parameters are securely handled | ||
| # and should not be validated in this case |
There was a problem hiding this comment.
| # skip the secret_params validation if it contains the key "secret" since the parameters are securely handled | |
| # and should not be validated in this case | |
| # if the secret_params are already masked - no need to validate |
mlrun/model.py
Outdated
| params_secret = secret_params.get("secret", "") | ||
| if params_secret: | ||
| return |
There was a problem hiding this comment.
I think the heuristic can be improved. We should also verify that this is the only param in the dict because if there are others than they will be ignored. I think we should be explicit about it. (That is IIUC the behavior @quaark )
There was a problem hiding this comment.
Yes I agree that's a good idea.
The error message should also be indicative and explain this mutual exclusion
resolves:
https://iguazio.atlassian.net/browse/ML-6936
when deleting notifications, only internal project secrets should be deleted.
also, this PR introduces a change to skip the validation of notification parameters when the "secret" key is present in the secret parameters.