Rework Auth Plugins to Support HTTP Auth #194
Conversation
This commit reworks auth plugins slightly to enable support for HTTP authentication. By raising an AuthenticationError, auth plugins can now return HTTP responses to the upgrade request (such as 401). Related to novnc/noVNC#522
DirectXMan12
force-pushed
the
feature/http-auth-plugins
branch
from
c13ad47 to
1e2b5c2
Compare
| except auth.AuthenticationError: | ||
| ex = sys.exc_info()[1] | ||
| self.send_auth_error(ex) | ||
| raise | ||
|
|
There was a problem hiding this comment.
Seems like auth should be before token plugin. Reduces the ability for non-authorized connections to do a denial of service to the system by inducing disk or DB activity due to token authorization.
There was a problem hiding this comment.
The issue is that currently, the authenticate method receives the decode host and port (that way you could say "person X is only authorized to connect to host/port Y"), so changing it would break backwards compatibility. Hmm...
There was a problem hiding this comment.
Yes, you're right. I suppose if it becomes an issue, we could always add an optional early auth call that doesn't have the target resolved yet. And really, it's probably better to do authorization on the token. But having the target is useful for authorization in many cases. And truth be told, if the token->target information is on disk or in a DB, then user auth info probably is too. Anyways, it was really just a thought that came to me, I'm fine with the change as is.
|
One inline comment but otherwise looks fine to me. |
Rework Auth Plugins to Support HTTP Auth
This commit reworks auth plugins slightly to enable
support for HTTP authentication. By raising an
AuthenticationError, auth plugins can now return
HTTP responses to the upgrade request (such as 401).
Related to novnc/noVNC#522