jupyterhub / ltiauthenticator

A JupyterHub authenticator for LTI
https://ltiauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
67 stars 54 forks source link

Do we really need to depend on oauthenticator? #155

Closed martinclaus closed 1 year ago

martinclaus commented 1 year ago

When analysing the dependency to the oauthenticator package, I find most all logic to be related to XSRF state handling, which is only a small portion of the logic implemented in the classes from which we derive.

More precisely, the following functions, classes and their attributes and methods are used:

If we decide to manage the handling of XSRF state (which is an opaque value and solely for security) ourselves, we can drop the dependency to oauthenticator.

Ping @consideRatio, @minrk

minrk commented 1 year ago

It's not strictly necessary, if we want to reimplement the relevant things. Indeed, OAuthenticator implements the Authorization Code flow, whereas LTI uses implicit flow, which means OAuthenticator is less useful as a base class and there's less code to re-use. If it would be simpler to be fully self contained, that's AOK for me.

which is an opaque value and solely for security

The oauth state is not strictly for security, at least in OAuthenticator - it preserves the requested URL path to ensure you arrive on the requested page, otherwise the landing page will not be preserved and the user will land back at a default URL regardless of where they started. We should make sure that visiting a page (e.g. /hub/token) results in landing on that page after completing login, however we choose to implement it.

martinclaus commented 1 year ago

Thank you for your thoughts. Indeed, the state parameter is used for the same purpose also in the ltiauthenticator.

IMO, keeping the number of dependencies low will improve the maintainability. In particular, since we depend on oauthenticator's private API, which might cause ltiauthenticator to break after non-breaking changes to oauthenticator.

minrk commented 1 year ago

I think it's sensible to drop the dependency, since the flow really is quite different here. I'd just make sure to preserve things like the post-auth destination, however seems appropriate.