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

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

Open
krassowski opened this issue Feb 21, 2024 · 2 comments
Labels

Comments

@krassowski
Copy link
Collaborator

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 #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 #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.

@minrk
Copy link
Contributor

minrk commented Feb 21, 2024

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

@Wh1isper
Copy link
Contributor

Wh1isper commented Mar 15, 2024

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

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

No branches or pull requests

3 participants