FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[WIP] Enable Access Control Management API & UI Page by Yuqing-cat · Pull Request #376 · feathr-ai/feathr · GitHub

/ feathr Public

[WIP] Enable Access Control Management API & UI Page - #376

Closed
Yuqing-cat wants to merge 6 commits into
feathr-ai:mainfrom
Yuqing-cat:rbac-new
ConversationCommits (6)ChecksFiles changed
Closed

[WIP] Enable Access Control Management API & UI Page#376
Yuqing-cat wants to merge 6 commits into
feathr-ai:mainfrom
Yuqing-cat:rbac-new

Conversation

Yuqing-cat commented Jun 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What is covered:

Items Notes
Access Control Management APIs 3 APIs to get, add, delete user role records in SQL database and protected with "admin" role
JWT Auth AAD Token supported
Access Control Management UI The link is still hidden. But you can test with /management & role-management router.
API Spec Update Access Control contents are added
SQL Schema & test data Sample data and Table initialize scripts are included

What is NOT covered:

Items Reasons
Access Control Initialized for new projects Init_rbac() Function and TODO are added. However, current API server & client still have gaps
protect all registry APIs with RBAC logic Protecting all APIs will Access Control configs may add burden to engineer test effort as the API is still under development. Will add it when API layer is stable. Current PR is the fundamental for Registry API Access Control in the future.
Access Control for other Registry backend, e.g., Purview Purview Registry APIs are under development
Other OAuth Token Currently, only AAD is supported by UI
Security Group Info Will be included in future enhancement.

Yuqing-cat requested review from blrchen and windoze June 16, 2022 11:14
Yuqing-cat linked an issue Jun 16, 2022 that may be closed by this pull request
Implement Basic RBAC Roles (Admin, Producer, Consumer) #146
Closed

Yuqing-cat commented Jun 16, 2022

Copy link
Copy Markdown
Collaborator Author

Some local test screenshots:

Yuqing-cat requested a review from jainr June 16, 2022 11:18
Yuqing-cat self-assigned this Jun 16, 2022
Yuqing-cat added the feature New feature or request label Jun 16, 2022
Yuqing-cat linked an issue Jun 16, 2022 that may be closed by this pull request
Add azure ad authentication to Fast API #295
Closed
Yuqing-cat changed the title Enable RBAC Management API & UI Page Enable Registry Access Control Management API & UI Page Jun 16, 2022
Yuqing-cat changed the title Enable Registry Access Control Management API & UI Page Enable Access Control Management API & UI Page Jun 16, 2022
blrchen reviewed Jun 16, 2022
View reviewed changes
Comment thread registry/sql-registry/api-spec.md Outdated
blrchen reviewed Jun 16, 2022
View reviewed changes
Comment thread registry/sql-registry/api-spec.md Outdated
blrchen reviewed Jun 16, 2022
View reviewed changes
Comment thread registry/sql-registry/api-spec.md Outdated
blrchen reviewed Jun 16, 2022
View reviewed changes
Comment thread registry/sql-registry/api-spec.md
blrchen reviewed Jun 16, 2022
View reviewed changes
Comment thread ui/src/api/api.tsx Outdated
}).then((response) => {
const reason = "Delete from management UI.";
return await Axios.post(
`${API_ENDPOINT}/users/${userrole.userName}/userroles/delete?code=${token}`,

blrchen Jun 16, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

${API_ENDPOINT}/users/${userrole.userName}/userroles/delete?code=${token},

accessToken should be put in header, not url query string.

Please fix same issue in other api calls.

It's suggeted to implement an interceptor which fires in every request.

Yuqing-cat Jun 17, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Move all tokens of access control management APIs into header with a common axios.create() function.
Please check if it's an acceptable way. If so, I will implement the practice to all APIs.

blrchen reviewed Jun 16, 2022
View reviewed changes
Comment thread registry/sql-registry/rbac/db_rbac.py Outdated

xiaoyongzhu commented Jun 19, 2022

Copy link
Copy Markdown
Member

Would this only work for SQL based registry? Or for both purview & SQL based reigstry?

Yuqing-cat requested a review from blrchen June 20, 2022 03:37

Yuqing-cat commented Jun 20, 2022

Copy link
Copy Markdown
Collaborator Author

Would this only work for SQL based registry? Or for both purview & SQL based reigstry?

In current implementation, this only work for SQL based Access Control with API layer.
It should also be able to work with purview-based registry API with a SQL server to store access control records.
To clarify, this PR only includes the fundamental infrastructure of SQL based Access Control management.

Also, I've tried to store access control records into Purview to simplify required resources. However, the concurrency control is still a big problem. If that's fixed, we can implement access control management into purview as well.

Yuqing-cat changed the title Enable Access Control Management API & UI Page [WIP] Enable Access Control Management API & UI Page Jun 22, 2022

Yuqing-cat commented Jun 28, 2022

Copy link
Copy Markdown
Collaborator Author

Create a new PR for access control plugin experience: #409
Close this one.

Yuqing-cat closed this Jun 28, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

windoze Awaiting requested review from windoze
jainr Awaiting requested review from jainr
hangfei Awaiting requested review from hangfei
jaymo001 Awaiting requested review from jaymo001
xiaoyongzhu Awaiting requested review from xiaoyongzhu
blrchen Awaiting requested review from blrchen blrchen is a code owner

Assignees

Yuqing-cat

Labels

feature New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add azure ad authentication to Fast API Implement Basic RBAC Roles (Admin, Producer, Consumer)

4 participants

Back | FazBrowse Home | New Git URL