Skip to content

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

Closed
Yuqing-cat wants to merge 6 commits intofeathr-ai:mainfrom
Yuqing-cat:rbac-new
Closed

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

Conversation

@Yuqing-cat
Copy link
Copy Markdown
Collaborator

@Yuqing-cat Yuqing-cat commented Jun 16, 2022 •
edited
Loading

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 Yuqing-cat requested review from blrchen and windoze June 16, 2022 11:14
@Yuqing-cat 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
Copy link
Copy Markdown
Collaborator Author

Yuqing-cat commented Jun 16, 2022

Some local test screenshots:
image
image
image

@Yuqing-cat Yuqing-cat requested a review from jainr June 16, 2022 11:18
@Yuqing-cat Yuqing-cat self-assigned this Jun 16, 2022
@Yuqing-cat Yuqing-cat added the feature New feature or request label Jun 16, 2022
@Yuqing-cat 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 Yuqing-cat changed the title Enable RBAC Management API & UI Page Enable Registry Access Control Management API & UI Page Jun 16, 2022
@Yuqing-cat Yuqing-cat changed the title Enable Registry Access Control Management API & UI Page Enable Access Control Management API & UI Page Jun 16, 2022
blrchen
blrchen reviewed Jun 16, 2022
View reviewed changes
blrchen
blrchen reviewed Jun 16, 2022
View reviewed changes
blrchen
blrchen reviewed Jun 16, 2022
View reviewed changes
blrchen
blrchen reviewed Jun 16, 2022
View reviewed changes
blrchen
blrchen reviewed Jun 16, 2022
View reviewed changes
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}`,
Copy link
Copy Markdown
Collaborator

@blrchen blrchen Jun 16, 2022

Choose a reason for hiding this comment

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

${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.

Copy link
Copy Markdown
Collaborator Author

@Yuqing-cat Yuqing-cat Jun 17, 2022

Choose a reason for hiding this comment

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

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
blrchen reviewed Jun 16, 2022
View reviewed changes
@xiaoyongzhu
Copy link
Copy Markdown
Member

xiaoyongzhu commented Jun 19, 2022

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

@Yuqing-cat Yuqing-cat requested a review from blrchen June 20, 2022 03:37
@Yuqing-cat
Copy link
Copy Markdown
Collaborator Author

Yuqing-cat commented Jun 20, 2022

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 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
Copy link
Copy Markdown
Collaborator Author

Yuqing-cat commented Jun 28, 2022

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

@Yuqing-cat Yuqing-cat closed this Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@windoze windoze Awaiting requested review from windoze

@jainr jainr Awaiting requested review from jainr

@hangfei hangfei Awaiting requested review from hangfei

@jaymo001 jaymo001 Awaiting requested review from jaymo001

@xiaoyongzhu xiaoyongzhu Awaiting requested review from xiaoyongzhu

@blrchen blrchen Awaiting requested review from blrchen blrchen is a code owner

Assignees

@Yuqing-cat 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

@Yuqing-cat @xiaoyongzhu @windoze @blrchen