Skip to content
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

Default allow_privilege_escalation to False #545

Merged
merged 2 commits into from
Nov 15, 2021
Merged

Conversation

yuvipanda
Copy link
Collaborator

Allows it to be set to None as well, to not set the property.

This is a breaking change for hubs where admins were granting
sudo rights to users. That already required some extra work,
so this would be an additional propety to set for that. The
added security benefit from this much more secure default is
well worth the breakage IMO.

Fixes #544

Allows it to be set to None as well, to not set the property.

This is a breaking change for hubs where admins were granting
sudo rights to users. That already required some extra work,
so this would be an additional propety to set for that. The
added security benefit from this much more secure default is
well worth the breakage IMO.

Fixes jupyterhub#544
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks @yuvipanda for getting this done!!! ❤️ 🎉

It would be good to have another reviewer agree we get this right as its a breaking change. @minrk @manics, perhaps you could have a look?

Note that this is both a bugfix, and a change of default.

  1. Bugfix: because our "True" value in the past acted as "None" rather than an explicit True.
  2. A change of default: because now we instead of having "None" as a default, we have "False" as an explicit default.

@consideRatio consideRatio requested review from minrk and manics November 15, 2021 11:35
@minrk minrk merged commit f1e1abc into jupyterhub:main Nov 15, 2021
@minrk
Copy link
Member

minrk commented Nov 15, 2021

Makes sense!

@yuvipanda
Copy link
Collaborator Author

Ty, @minrk and @consideRatio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The default of allow_privilege_escalation should be False
3 participants