-
Notifications
You must be signed in to change notification settings - Fork 18
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
Configurable Authorization #204
Conversation
428c8bc
to
3541eca
Compare
Codecov Report
@@ Coverage Diff @@
## master #204 +/- ##
==========================================
+ Coverage 75.00% 77.72% +2.72%
==========================================
Files 11 12 +1
Lines 748 1001 +253
Branches 127 173 +46
==========================================
+ Hits 561 778 +217
- Misses 161 192 +31
- Partials 26 31 +5
Continue to review full report at Codecov.
|
755967e
to
06bd627
Compare
a04e8d8
to
c0f1991
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a super quick review done reading in the GitHub UI @datamel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@datamel checked out the branch today and remembered I hadn't used the IDE static analyzer yet. Found a few typos and unused imports. Marked them in this last review 👍
No chance of nested here, we've designed the mutations as flat:
So there's nowhere to nest.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@datamel I finished reading the proposal, and reading the code today 🎉
Still need to play more with the UIS and authorization schema.
But one thing I couldn't find in the docs, I wonder if you could help me… I saw that we have a site and a user config. So I imagined myself as a system admin looking after a machine with several (100s?) users.
All users having ability to start their own hub (something like module load cylc
or similar). Now, maybe I don't want users to start Cylc because of GraphQL queries consuming resources, or some other concern.
Maybe I could limit the module loading, but users could still use Conda to install perhaps. That got me thinking in trying the site configuration to lock my own account. i.e. set a default to !ALL
or something like that.
I couldn't get that to work, and I couldn't find anything in the proposal. Is the authorization supposed to work only controlling authorization to other users?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a lazy-way to review it 😄
Here's my ~/.cylc/hub/config.py
:
c.UIServer.ui_build_dir = '~/Development/python/workspace/cylc-ui/dist' # path to build
c.JupyterHub.authenticator_class = 'jupyterhub.auth.DummyAuthenticator'
import pwd
def mocked_fn(any_value):
class Obj(object):
def __init__(self):
self.pw_uid = 1000
self.pw_gid = 1000
self.pw_dir = '/home/kinow/'
self.pw_shell = '/bin/bash'
self.pw_gecos = any_value
self.pw_name = any_value
self.pw_passwd = 'x'
def __repr__(self):
return str(self.__dict__)
return Obj()
pwd.getpwnam = mocked_fn
c.Authenticator.delete_invalid_users = True
c.Authenticator.admin_users = {
'kinow'
}
c.JupyterHub.api_tokens = {
'knockknock': 'kinow'
}
c.Application.log_level = 'DEBUG'
c.UIServer.user_authorization = {
"cylc": ["READ", "pause", "trigger", "message"],
"group:group1": ["ALL"],
"user2": ["READ", "CONTROL", "!trigger", "!edit"],
"user3": ["READ", "stop", "play"],
"user4": ["!ALL"]
}
JupyterHub uses the pwd
module to fetch the current user (we use getpass
, but I think in the end both should access the same system resources and both hopefully work with LDAP/etc). Monkey-patching it and using the DummyAuthenticator
with no password, that means I can log in as anything.
All the workflows are loaded from the same folder. One strange behaviour is that I can see all workflows, but upon start, the workflow disappears from the list of other users, and only my own user shows the workflow (we are probably filtering it somewhere in UIS), but that doesn't interfere with my testing here I think.
So my kinow
user has:
And the cylc
user (does not exist in my OS) has:
Trying to access my kinow|five
from this cylc
user, but no luck.
So at least I know the new authorization code is working. Now trying to get the hang of the schema and enable/disable permissions.
This comment has been minimized.
This comment has been minimized.
Hi @kinow,
With the example above, the Once this is fixed (apologies for the mix-up with spelling, I will post on element now I am back to get confirmation of spellings), the config you have created will still not work to allow
In order for user
Fingers crossed this will be good to go now. If these suggestions fail, I can give you a call on element this evening/your morning to see if I can get you up and running with this. |
- Fix current user for subscriptions - Add tests
4d27136
to
de5dd1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great 🎉 No major issues found. A few minor code suggestions, and some questions (next) for possible follow-up.
) | ||
# If not explicit permissions for access user in owner conf then revert | ||
# to site defaults | ||
if len(user_conf_permitted_ops) == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd better support implicit as well as explicit negation, because it's likely to be assumed by users, and we do for partial cases, right?:
'x' : ['op1', 'op2'] # allow only 'op1' and 'op2'
'y': ['op1'] # allow only 'op1'
# therefore:
'z': [ ] # allow no access
A couple of more general issues, possibly for follow-up:
|
(Yikes, lost control of the keyboard and closed the issue!) |
(I should probably spend more time looking at this yet, but it's taken a while for me to get up to ~speed again on the UIS, figure out how to diagnose what it's doing etc.). |
Not really.
Jupyter Server loads the config so there isn't much we can do, however, if you look at the UIS log you should see a warning for each mistyped config (providing you got the
The UIS logs the permissions of the authenticated user when they first connect. There's kinda an interface for viewing permissions vie the user-profile. On Mel's UI branch this is used to disable mutation buttons. |
Thanks so much for the review @hjoliver, I will implement the changes today.
The reason we are keeping them separate is due to negations. If you did
Yes, typos could potentially be a bit frustrating for users. To add to what Oliver has said, I have also opened an issue to add more detailed validating of the config. Ideally this would flag typos in operation name. |
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
@datamel note I commited a small syntax fix to your branch to make the tests pass (I had ommited a comma in a code change suggestion of mine that you commited, I think). |
Grand thanks! I'm just about to push a change. I spotted a problem with the permissions being passed between uiserver and ui, the format of some of the mutations were not matching up, hard to spot, since I hadn't really tested things like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, this can go in now 🎉 I'll post another issue for minor follow-up questions.
These changes close #10
Addresses #171
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.Authorization
Implementation includes...
Configuration
Site config for authorization
Sets site policy for
Example site config...
More examples in the proposal
Reminder for reviewing, this is set to
/etc/cylc/hub/config.py
but can be overridden using the env variableCYLC_SITE_CONF_PATH
User config
Sets user applied permissions, supports both usernames and system groups.
This resides here:
~/.cylc/hub/config.py
.Example user config...
Config Details.
READ
,CONTROL
andALL
access groups implemented. See access group table mappings.Middleware
Authorization has been implemented at GraphQL middleware level.
As of current implementation...We do not check auth for return variables, in order for this not to compromise security need confirmation that GraphQL mutations won't be nested (e.g. Can't perform a kill within a play) [Confirmed, by @dwsutherland below].
User Permissions
In line with the diagram, users and permissions are cached, reducing processing and system calls.
Note to reviewers
groups
in a terminal will reveal these). You can then configure the config to test this fully.