Affix webserver access_denied warning to be configurable#33022
Conversation
|
|
||
|
|
||
| def get_access_denied_message(): | ||
| return conf.get("webserver", "access_denied_message") |
There was a problem hiding this comment.
This should either use get_mandatory or provide a fallback.
There was a problem hiding this comment.
Not really. The get_mandatory is needed only when we want to have a value but the value has no default (for example when we want to get a URL that has no default but we really need it to be configured).
The way it works when it is put in config.yml is that "default" is automatically used as fallback if there is no fallback (It's been working like that already for quite some time via defaul_config.cfg - so my recent change has not changed it - jut made it works without the default_config.cfg
There was a problem hiding this comment.
I don't think it is needed as @potiuk mentions here. The older default text is passed as the default in the new property in config.yml, which is basically the fallback.
pankajkoti
left a comment
pankajkoti
left a comment
There was a problem hiding this comment.
Looks good to me, minor cosmetic suggestions if you'd like to consider :)
| options: | ||
| access_denied_message: | ||
| description: | | ||
| The message displayed when an user tries to perform operations outside their rights. |
There was a problem hiding this comment.
| The message displayed when an user tries to perform operations outside their rights. | |
| The notification that appears when a user attempts to execute actions beyond their authorised privileges. |
There was a problem hiding this comment.
I would like to keep it as message because this is not a notification. Maybe I can term it to warning instead
There was a problem hiding this comment.
okay so
"The message displayed when a user attempts to execute actions beyond their authorised privileges."
?
There was a problem hiding this comment.
If not, the minimum suggestion is to correct an user to a user 🙂
There was a problem hiding this comment.
Yeah why not, thanks, making the changes
| version_added: 2.7.0 | ||
| type: string | ||
| example: ~ | ||
| default: "Access is Denied" |
There was a problem hiding this comment.
| default: "Access is Denied" | |
| default: "Unauthorized Access: Denied" |
There was a problem hiding this comment.
I dont think we can do that. The current message is "Access is Denied" so in order to keep the current experience, we got to have this default value
There was a problem hiding this comment.
Agreed. We do not want to break the backward compatibility experience. If we want to make this change, we should have a different task for it instead
| version_added: 2.7.0 | ||
| type: string | ||
| example: ~ | ||
| default: "Access is Denied" |
There was a problem hiding this comment.
I dont think we can do that. The current message is "Access is Denied" so in order to keep the current experience, we got to have this default value
| version_added: 2.7.0 | ||
| type: string | ||
| example: ~ | ||
| default: "Access is Denied" |
|
Waiting for a second look from @uranusjr before merge |
eladkal
left a comment
eladkal
left a comment
There was a problem hiding this comment.
Lets merge so it won't miss 2.7.0
we can modify in followup PR if needed
* Affix webserver access_denied warning to be configurable (cherry picked from commit 12a760f)
9 participants
Add a possibility to make the airflow webserver's access denied message - which occurs when an user tries to perform some operation outside their user rights. Allow this to be configurable through config so that every webserver installation can have a custom message without changing the airflow code.
Note: This message is not per call to the
has_accessfunction, but per webserver installation which is a more realistic case.Took suggestions from community and approached the problem accordingly.
get_access_denied_messageso that the "unit" is testable.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.