uis: authorise endpoint for multi-user use #202
Conversation
|
Mocked authorisation for tests. |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| ME = getpass.getuser() |
There was a problem hiding this comment.
Not sure if this would work for every JupyterHub authenticator? Maybe for some users the user database is stored in a database, external system, etc. So maybe getpass.getuser() would lock us to linux & pam only?
There was a problem hiding this comment.
In order for the UIS to run the user has to exist on the system so it should be good for most things but in more esoteric systems where the authenticated user does not match any system user it would break yes (although I'm not sure how the hub could spawn UI Servers in this case).
We will add the ability to configure authorisation (i.e. grant access to different accounts) that should help us to handle this sort of thing.
For example we could do something along the line of this:
[authorisation]
[[alice]] # authorised user name
read = True
[[bob]] # authorised user name
read = True
write = True
[[ME]] # my authorised user name if it differs
read = True
write = True
execute = TrueThere was a problem hiding this comment.
Maybe we could use Tornado's current_user then?
When the hub is configured to use PAM, it will match what user you have logged in. Then we can compare that against the request path.
So if I logged in with a database backed authenticator and have the user kinow, but I'm accessing the URL /#/workflows/userx|one, then our code to authorise would just match current_user (kinow from the database in this case, but would work with PAM, etc) and fail if I don't have authorization I think?
Also, if we run Cylc UIS with root or another superuser, what would getpass.getuser() return? Wouldn't it return root instead of the logged-in user?
There was a problem hiding this comment.
At the moment this should fail all authenticated users except for the user account under which the UIS is running.
Also, if we run Cylc UIS with root or another superuser
The UIS should never be run as root. It is only run under a regular system user.
The Hub could be run under an privileged account (e.g. root) so that it could spawn UI Servers under regular user accounts, (although, until we have proper authentication there is little advantage to doing this).
There was a problem hiding this comment.
I think current_user is the result of get_current_user which returns the authenticated user who performed the request.
We need to know the user that the UIS is running under.
There was a problem hiding this comment.
Ah yeah, you are correct, the UIS should be with my own user, my bad!
There was a problem hiding this comment.
in more esoteric systems where the authenticated user does not match any system user it would break yes (although I'm not sure how the hub could spawn UI Servers in this case).
Not sure that's true. The authenticator doesn't have to know about users on the target system where the UIS and Schedulers run. So long as our authorization system doesn't require the authenticated user to be valid in its domain, we should be able to authorize "alien" users to access the workflows, no?
There was a problem hiding this comment.
(We do of course have to trust that the authentication system has been configured safely).
There was a problem hiding this comment.
we should be able to authorize "alien" users to access the workflows, no?
Yes, however, those users won't be able to start UI Servers as they would have no account to spawn the UIS under.
(This thread was about the use of getpass.getuser() in the UIS for determining the name of UIS owner.)
There was a problem hiding this comment.
OK, you're talking about the current PR, not where we're heading (any authenticated user can spawn a UIS on other users' accounts).
cylc/uiserver/tests/test_handlers.py
Outdated
| # TODO: can't get this test to run due to handler setup? | ||
| # @pytest.mark.usefixtures("mock_authentication_none") | ||
| # def test_unauthenticated(self) -> None: | ||
| # """Test 403 HTTP response (unauthenticated).""" | ||
| # response = self.fetch('/') | ||
| # assert response.code == 403 | ||
| # # assert response.reason == 'Forbidden' |
There was a problem hiding this comment.
I get 500 errors for these tests as the UIS hits an internal error before authentication has the change to fail.
Should find a way to configure things so these tests run in the future, in the mean time can test manually (behaviour unchanged).
There was a problem hiding this comment.
There was a problem hiding this comment.
Does the job as expected with regards to the need to secure the UI Server now so I'm happy to get this merged in for the beta 1. I have manually attempted to break into another user uiserver and can see this branch fixes the issue.
Have a couple of queries which don't affect approval...
- Are you sure the
nonlocalis needed with wrapped function arguments? - Probably don't need to override
runand may also be more secure to overridepostinstead? Both of these comments are tested on my POC PR: Configurable Authorization #204.
Thanks for getting this fixed @oliver-sanders!
| lambda x: ret | ||
| ) | ||
|
|
||
| _mock_authentication() |
There was a problem hiding this comment.
Why does it need to call the inner function itself?
Needed 100% of the time, no. I use
Probably right, I followed the "better safe than sorry" on that one. I did try overriding |
|
Tested with the Tested with One tab has The other tab has Hub is running as
(above the incognito tab has
|
|
Tested on Firefox, got the same behavior. Then, since I had two tabs, one with
Then pressing Authorize I got:
So looks like the authorization code in this PR worked! Just trying to understand why my UI is not working for |
|
BTW, stopping the Hub with 2 UIS's running with the Sudo Spawner, it tidily stopped the processes and logged everything (cc @jarich).
|
There was a problem hiding this comment.
On master I also cannot use the second user... or at least it doesn't show anything in the UI. So for a separate issue.
Only thing that's happening in this PR is that when I open cylc1, there are no warnings in the console. But when I open kinow UIS, even without trying to access cylc1's UIS, I get:
2021-04-14 14:00:23,157 cylc.uiserver.websockets DEBUG Unauthenticated WebSocket request!
Every few seconds a new line like this. I only have one tab open, with the user logged in.
Maybe I need to install something for cylc1? Maybe it needs to have at least 1 workflow? I tried creating ~/cylc-run, but it still failed when I opened the UI 😕
Solved! In the virtual machine, I had a browser minimized with a tab with my kinow user, but logged out. Then websocket connection was still open, causing the warnings! 👍
Co-authored-by: Bruno P. Kinoshita <kinow@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
==========================================
+ Coverage 72.42% 73.09% +0.66%
==========================================
Files 10 10
Lines 729 762 +33
Branches 117 120 +3
==========================================
+ Hits 528 557 +29
- Misses 180 184 +4
Partials 21 21
Continue to review full report at Codecov.
|
|
Good to know, I had spotted that warning at some point in the past. |
There was a problem hiding this comment.
I think we are checking authorization multiple times for some requests, but would have to debug it to confirm.
@oliver-sanders for the TODO tests, there's a few things that we can change so that they pass
- Add a cookie secret for the application (otherwise authentication is not even handled properly (see the application in the code snippet)
- Add a handler for the login URL (unauthenticated requests are not 403'd, instead they are redirected to the login URL I think? at least that's the way UIS works at the moment)
- Confirm that we were redirected (or maybe force Tornado to throw 403, even though that's not how UIS works)
Here's how I got both TODO's removed, and the tests passing locally
diff --git a/cylc/uiserver/tests/test_handlers.py b/cylc/uiserver/tests/test_handlers.py
index 0cc911f..a0266ce 100644
--- a/cylc/uiserver/tests/test_handlers.py
+++ b/cylc/uiserver/tests/test_handlers.py
@@ -24,7 +24,7 @@ from graphql_ws.constants import GRAPHQL_WS
from tornado.httpclient import HTTPResponse
from tornado.httputil import HTTPServerRequest
from tornado.testing import AsyncHTTPTestCase, get_async_test_timeout
-from tornado.web import Application, HTTPError
+from tornado.web import Application, RequestHandler
from cylc.uiserver.main import (
MainHandler,
@@ -36,6 +36,12 @@ from cylc.uiserver.main import (
import pytest
+class NoOpHandler(RequestHandler):
+
+ def get(self):
+ pass
+
+
class MainHandlerTest(AsyncHTTPTestCase):
"""Test for the Main handler"""
@@ -43,8 +49,10 @@ class MainHandlerTest(AsyncHTTPTestCase):
self.tempdir = tempfile.mkdtemp(suffix='mainhandlertest')
return MyApplication(
handlers=[
- ('/', MainHandler, {"path": self.tempdir})
- ]
+ ('/', MainHandler, {"path": self.tempdir}),
+ ('/hub/api/oauth2/authorize.*', NoOpHandler)
+ ],
+ cookie_secret='MainHandlerTest'
)
@pytest.mark.usefixtures("mock_authentication")
@@ -62,13 +70,12 @@ class MainHandlerTest(AsyncHTTPTestCase):
response = self.fetch('/')
assert response.code == 500
- # TODO: can't get this test to run due to handler setup?
- # @pytest.mark.usefixtures("mock_authentication_none")
- # def test_unauthenticated(self) -> None:
- # """Test 403 HTTP response (unauthenticated)."""
- # response = self.fetch('/')
- # assert response.code == 403
- # # assert response.reason == 'Forbidden'
+ @pytest.mark.usefixtures("mock_authentication_none")
+ def test_unauthenticated(self) -> None:
+ """Test login URL redirect response (unauthenticated)."""
+ response = self.fetch('/')
+ assert response.code == 200
+ assert response.effective_url.index('/hub/api/oauth2/authorize') > 0
@pytest.mark.usefixtures("mock_authentication_yossarian")
def test_unauthorised(self):
@@ -88,8 +95,10 @@ class UserProfileHandlerTest(AsyncHTTPTestCase):
def get_app(self) -> Application:
return MyApplication(
handlers=[
- ('/userprofile', UserProfileHandler)
- ]
+ ('/userprofile', UserProfileHandler),
+ ('/hub/api/oauth2/authorize.*', NoOpHandler)
+ ],
+ cookie_secret='MainHandlerTest'
)
@pytest.mark.usefixtures("mock_authentication")
@@ -107,13 +116,12 @@ class UserProfileHandlerTest(AsyncHTTPTestCase):
response = self.fetch('/userprofile')
assert response.code == 200
- # TODO: can't get this test to run due to handler setup?
- # @pytest.mark.usefixtures("mock_authentication_none")
- # def test_unauthenticated(self) -> None:
- # """Test 403 HTTP response (unauthenticated)."""
- # response = self.fetch('/userprofile')
- # assert response.code == 403
- # # assert response.reason == 'Forbidden'
+ @pytest.mark.usefixtures("mock_authentication_none")
+ def test_unauthenticated(self) -> None:
+ """Test login URL redirect (unauthenticated)."""
+ response = self.fetch('/userprofile')
+ assert response.code == 200
+ assert response.effective_url.index('/hub/api/oauth2/authorize') > 0
@pytest.mark.usefixtures("mock_authentication_yossarian")
def test_unauthorised(self):
|
(We can change the test in a follow-up, only posted here because I had it in GitHub notifications and decided to look why these tests were failing 😅 ) |
|
@oliver-sanders - leaving it to you to merge now or deal with @kinow's comments first. |
* apply patch from cylc#202 (review) * thanks @kinow
|
Merging with four approvals. |
This adds a trivial authorisation scheme to the UIS which rejects anyone but the UIS user.
addresses: #10, #171
opens and closes #203
This will be followed by a configurable authorisation scheme in due course.
For testing, change the value of
MEor try with a second user account. For now manual testing only, can implement automated next release.Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.