search

jupyterhub / jupyter-rsession-proxy

Jupyter extensions for running an RStudio rsession proxy
BSD 3-Clause "New" or "Revised" License
118 stars 87 forks source link

Made the www-frame-origin for rserver environment-configurable #148

Closed rickmcgeer closed 6 months ago

rickmcgeer commented 6 months ago

This is the proposed solution to: https://github.com/jupyterhub/jupyter-rsession-proxy/issues/147. Configuration by traitlets, as proposed in the issue, is inappropriate since the command to set up the rserver and rsession is invoked when the package is loaded, so any configuration has to be present in the environment when the package is loaded. Adding configuration by environment variables is consistent with the existing package architecture, and in fact it was done (hardcoded) in setup_rserver._get_timeout and setup_rsession._get_timeout.
Changes made:

  1. New top-level method get_env_value(env_var_name) to get the value for the given environment variable. Defaults are provided for configurable environment variables.
  2. Added RSERVER_FRAME_ORIGIN environment variable to make www-frame-origin environment-configurable. The default value is the same as the current hard-coded value.
  3. Added RSERVER_TIMEOUT environment variable and replaced setup_rserver._get_timeout() with get_env_value('RSERVER_TIMEOUT'). Default is the same as the current hardcoded default (15)
  4. Added RSESSION_TIMEOUT environment variable and replaced setup_rsession._get_timeout() with get_env_value('RSESSION_TIMEOUT'). Default is the same as the current hardcoded default (15)
  5. Replaced user = os.environ.get('NB_USER', getpass.getuser()) with get_env_value('NB_USER'). Default for NB_USER is getpass.get_user()
  6. Updated README.md with information on using configuration variables.
welcome[bot] commented 6 months ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

rickmcgeer commented 6 months ago

@ryanlovett The only reason I would do this is to learn how, but I think I already know and you doing it saves us a loop. Sold, with (as always) thanks!

  1. The (minor) case for get_env_value is that adding a new environment variable is a little easier than a dedicated function (just add an entry to a dictionary). But what's much more important is to maintain consistency with the current code base, and you have a much better handle on that than I do. Also, as you point out, using a dedicated function abstracts away how these values are getting set, and so traitlets in the future will be more seamless. And the it's-a-little-easier argument is very weak; after all, we won't have all that many environment variables, so the effect on the code by using dedicated functions is very minor. So, please, use your best judgment here -- I am happy either way. Can I propose _get_rserver_frame_origin() or similar for the dedicated function?
  2. JUPYTER_RSESSION_PROXY_FOO is much better than FOO. Sorry, I should have thought of it...please make that change.
  3. I'm sorry about the egg-info. What happened was (I'm explaining so you can tell me how not to screw this up in future!) is that I needed to clone and install the package and hardcode the www-frame-origin=any flag, and to make this seamless with pip git+https I cloned the egg-info. I then branched off that fork. I'm sure I could have done that better.

Please go ahead and make all those changes.

Thanks again for everything Rick

ryanlovett commented 6 months ago

@rickmcgeer Is the setting to allow edits from maintainers enabled in your fork's PR? I don't think I can edit the PR otherwise. (I've tried and failed, but I could be doing something wrong.) Alternatively, I can make a new PR and pull in your changes.

rickmcgeer commented 6 months ago

@ryanlovett my apologies; I don't know. The doc you referenced says "On user-owned forks, if you want to allow anyone with push access to the upstream repository to make changes to your pull request, select Allow edits from maintainers.". I've been unable to find the screen with the field "Allow edits from maintainers" (as with many infrastructures, Github has a security/permissions system designed by Franz Kafka and administered by the DMV and the City of Dis as told by Niven and Pournelle in Inferno). Anyway, I made you an admin of our repo, so I hope that's enough. Sorry for the trouble,...

ryanlovett commented 6 months ago

@rickmcgeer :) "It was on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying 'Beware of the Leopard.'"

Its no trouble, thanks! I think this should be sufficient.

ryanlovett commented 6 months ago

With this PR, I've confirmed that setting JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN=any causes --www-frame-origin=any to be passed to rserver.

I didn't change the other env vars as part of this PR to avoid breaking things for others. That can be done separately in an announced change.

welcome[bot] commented 6 months ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart:

ryanlovett commented 6 months ago

Thanks @rickmcgeer !

rickmcgeer commented 6 months ago

Thanks @ryanlovett ! It was a real pleasure to work with you and I hope to do so again soon.

ryanlovett commented 6 months ago

Absolutely @rickmcgeer ! We'd be happy to receive more code contributions. :)

rickmcgeer commented 6 months ago

@ryanlovett Happy to contribute! On that note, should we enhance the Shiny proxy the same way? And the next time we do this (for the Shiny proxy or another piece of code), I'd like to seek your guidance on the right way to fork the repo so we don't have the permissions issues that we had to deal with this time. If I'd done it right that wouldn't have happened.