-
Notifications
You must be signed in to change notification settings - Fork 49
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
Improvements to the namespace Role Mappings #491
Comments
First step with the DB impacts is here in branch Then I'm kind of unsure how to progress with |
I'd just supply the db session as a function argument. Would that work? |
Ok, some parts around the authentication were still blurry but I see clearer now. I have explored the solution you have described. Let me know if I get this correctly, because I have a slightly different approach to suggest:
Let me know what you think :) |
2023 07 06 - Sync with Chris :
|
Great summary @pierrotsmnrd. After checking the code more I think that several methods will require the
The more I think about it the RBACAuthorization class just generally needs a db session object injected into it when being created. . It would be difficult to pass the db instance in all methods. Might be easiest to add after https://github.com/Quansight/conda-store/blob/main/conda-store-server/conda_store_server/server/app.py#L189 the following: self.authentication._authorization.db = conda_store.db Probably do in a more official way 😄 |
This issue is partially implmented @pierrotsmnrd. Now we need to add api methods mentioned in this issue. |
I think this issue is fully resolved now that we have merged the PR #508. Will re-open if issues occur. |
Reopening since we need to make get namespace return the api endpoint data. |
This is an important issue for the role mappings in the database to be done properly but is not a priority for JATIC work. |
@nkaretnikov assigning this to you |
This is a complex issue that I think is worth talking about over a call with @pierrotsmnrd and I. I'll put the current status here. I think we slightly missed how this should be implemented. The entire use case: I We have a database table Also I have doubts on us having So I think that most of the hard work is already done but we need to tweak the REST api and fixup how we represent things in the DB. @pierrotsmnrd do you have thoughts? Also we should schedule a call. |
Working on this. I found the discussion here pretty difficult to follow, so I decided to focus on this bit, which is actionable:
It was tricky to understand how this would even look in the UI, so I started from that direction. I have a UI mockup here: https://github.com/nkaretnikov/conda-store/commits/role-mappings-491 It looks like this: Dropdowns update when you click on them. Delete shows a popup warning before executing. The style is similar to how we manage builds, but I use a table here to keep it aligned. We could add stuff like text field auto-complete later. We could move it somewhere else, but currently:
The main thing I need to figure out is how role assignment will look like, such that I could actually list some users in this table above. Eve is just a hardcoded placeholder at the moment. IIUC, we don't store usernames, it's all token-based (JWT). I'll update other APIs as needed. |
Actually this is the wrong way around we first need to define how the API should look like, followed by the workflow and the UI is the very last thing that needs doing. The UI simply exposed the workflow to the user. |
@trallard There's a huge gap between the discussion above (about entity being a wrong abstraction or the DB attribute idea) and what Chris wrote about being able to share namespace permissions. It wasn't clear to me how those things help (or not) reaching that end goal. I wanted to have a mockup to test real-world UX, to see which handlers are needed for those buttons to work, or what info is needed in the DB to be shown in the UI. Now I understand constraints better, the UI is a bonus. To me, this highlighted a need to have access to usernames, which we cannot get easily IIUC. You need a readable way to distinguish users, in case you want to update their permissions later or remove them. OK, I stop work on this and will schedule a discussion. |
Meeting has been scheduled to discuss this. Blocked until the meeting happens |
@trallard I wont be in the meeting but I think as we think through these roles, we need to identify some usecases or user profiles: As a As an individual As a As an Based on what I currently know about the role mappings, this raises a couple of issues:
disclaimer: there are so many issues on this topic, I apologize if I've posted this in the wrong place. |
Note: you don't need to read this message unless you want to provide early feedback on the model. After talking to people and reading the code, I present my current plan below. I might still make changes to it if I run into implementation issues, but I hope there won't be any. This is motivated by how we want sharing to work, as well as what we want to have in the UI. UI mock up (this shows access to current namespace):
This explains how the above UI can be implemented using routes listed below:
Routes:These allow reading and deleting everything for convenience:
These always return 1 entry or fail.To detect potential API errors, I've opted for the strict CRUD model.
Tableclass NamespaceRoleMapping(Base):
"""Mapping between roles and namespaces"""
__tablename__ = "namespace_role_mapping"
id = Column(Integer, primary_key=True)
# Provides access to this namespace
namespace_id = Column(Integer, ForeignKey("namespace.id"), nullable=False)
namespace = relationship(Namespace, back_populates="role_mappings")
# ... for other namespace
other_namespace_id = Column(Integer, ForeignKey("namespace.id"), nullable=False)
# ... with this role, like 'viewer'
role = Column(Unicode(255), nullable=False)
@validates("role")
def validate_role(self, key, role):
if role not in ["admin", "viewer", "developer"]:
raise ValueError(f"invalid entity={role}")
return role
__table_args__ = (
# Ensures no duplicates can be added with this combination of fields
UniqueConstraint('namespace_id', 'other_namespace_id', 'role', name='_uc'),
) CommentsI've kept 'namespace' here to keep this table related to
Also, the current model is stricter and not as flexible as before, but it is less confusing and allows to implement namespace sharing. In this model, when I have namespace We could do it by environment instead, but it would complicate things in the UI and DB. Because then people would ask to share multiple environments together, too. So I think it's simpler to just do it by namespace from the start, and that's it. Important: this is not compatible with the previous DB model, so my plan is to drop namespace mappings when migrating, which will require users to re-add other namespaces (for sharing, I mean). But I don't think we even use this feature yet, do we? Also, we have versioned API, but I'm just modifying v1. I don't think we've made any claims about stability of this one yet. I'm also not sure if anyone uses these APIs either. |
Status update for PR #607:
|
@nkaretnikov has opened a draft PR and needs my review. |
@smeragoel and I had a call about the design part of this. I also talked to @costrouc after that.
|
Status update:
|
I’ll get a first draft if the designs ready for review by the end of this week! |
This is a first draft of the designs required for the workflow described above. 1. Namespace Page
2. Edit Namespace
3. Creating a new namespace
|
@smeragoel these look great! Thank you! New Namespace button - I think the placement looks good. The label is kind of long for a button, but I agree with you that its necessary to include the word "namespace" so I dont think anything can be done about it. What is the checkbox next to the trashcan for? - Ohhh now I see that line is what it looks like if you change the role. On the New Namespace page, the changing role highlighting and checkmark doesn't make sense though, right? Since everything will be new? One thing we might have to think through is adding Users to the new Namespace.
These questions are mixed backend and frontend and I realize they may initiate more work so I want to say that I'm happen to consider these ideas as enhancements once we get the POC out. |
From meeting today, it'll be intuitive to rename the "developer" role to "editor" |
After making this change locally, I realized it introduces too many changes. It requires updating the default role mappings, the docs, the DB validation functions, and the tests (to support both of these roles). The current role mappings PR is already too big. This will be done separately. New issue: #675. |
From the meeting today:
|
Migrating data from v1 to v2 role mappings will be done in a separate PR because it can cause issues during migration, which might require manual intervention. So a standalone function will be provided for those who need it. #681 |
Status update: asked Chris and Chuck to review PR #607. Also, allocated time for a meeting to answer questions to speed up review process. |
Motivation
For authentication currently the user to role and permissions has been managed in traitlets. Via a few important attributes
Developers were responsible for creating a function
authenticate(...)
which handled the user to role mapping. With this configuration it was possible to conda-store to not know anything about the users.Proposal
I propose adding one new database table to conda-store. We leverage the fact that the username will match a namespace name which is created on the backend.
Then add a method to
RBACAuthorizationBackend
Additionally there should be rest api methods and permissions for CRUD. I am least sure about this part.
- need to add a
schema.Permissions
namespace::update
permission which will allow updating the metadata- namespace update rest api method that uses the namespace update
- create, list, delete for adding namespace role mappings. I think you should only be able to edit namespace role mappings if you have
namespace::update
permissions. If you havenamespace::read
it should be sufficient to view members.@pierrotsmnrd I will be gone until Thursday but I think this should be scoped well enough to start. I will periodically be checking github so I should be able to respond to comments.
The text was updated successfully, but these errors were encountered: