jupyter_server icon indicating copy to clipboard operation
jupyter_server copied to clipboard

Should `current_user` and `prepare` guards be set in `AuthenticatedHandler` rather than `JupyterHandler`?

Open krassowski opened this issue 1 year ago • 2 comments

Description

Currently current_user is getting set in JupyterHandler which inherits from AuthenticatedHandler. Some other guards against unauthenticated access (e.g. prepare, check_host, check_referer, etc) are also implemented in JupyterHandler rather than on AuthenticatedHandler level. PR https://github.com/jupyter-server/jupyter_server/pull/1392 adds more guards in the same place as where the existing ones are (i.e. JupyterHandler.prepare).

Some downstreams incorrectly assumed that AuthenticatedHandler does in fact all which is needed for authentication and because of that had insufficient access control. It appears not the best practice to call something Authenticated when in fact it is more of a mixin which requires JupyterHandler to get the full protection.

Expected behavior

  • if AuthenticatedHandler is to stay as-is, extension authors should not need to read the code to know which handler to inherit from and this should be documented with big red warning that AuthenticatedHandler is not to be used unless by advanced developers who know what they are doing
  • alternatively, logic required for authentication should all live in AuthenticatedHandler rather than be spread between it and JupyterHandler

Context

I had asked this question in https://github.com/jupyter-server/jupyter_server/pull/1392 but did not get an answer. It is not necessary to address it in that PR so I am opening a new issue to track the dicsussion.

krassowski avatar Feb 21 '24 08:02 krassowski

I think it probably does make sense to do that, yes.

minrk avatar Feb 21 '24 10:02 minrk

Some downstreams incorrectly assumed that AuthenticatedHandler does in fact all which is needed for authentication and because of that had insufficient access control.

Yes, that's what I used to think until I carefully read the code. Maybe we could also rename AuthenticatedHandle to Mixin? Which may require more work though.

BTW, I also support:

alternatively, logic required for authentication should all live in AuthenticatedHandler rather than be spread between it and JupyterHandler

Wh1isper avatar Mar 15 '24 03:03 Wh1isper