-
Notifications
You must be signed in to change notification settings - Fork 3k
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
config(rest-api): enable authentication and api authorization by default #11484
config(rest-api): enable authentication and api authorization by default #11484
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
800cd67
to
de17c10
Compare
* breaking change (previously disabled by default) * upgraded pytests for running with authentication enabled
de17c10
to
179a0d7
Compare
@david-leifker First, I wanted to include this in my PR for the Helm chart...however I think here it is probably better... I have already noticed it sometime ago - we have enabled the REST API authorizations for our deployment for some time - and I also have tried to fix it, however the whole policy logic is a bit hard to follow (i.e. the PolicyConfig.java is just too big and is confusing if you try to understand it...there are plain privileges, then they are grouped somehow in READ, WRITE, etc. and then there is CRUD-like structure which is used "somehow"): Currently some of the privileges which are affected by the REST_API_AUTHORIZATION_ENABLED environment variable cannot be assigned to custom policies using the UI because the privileges are missing in the corresponding list in PoliciesConfig.java (see also this comment here). These privileges are only granted to the DataHub Root User and the Admin role, but cannot be included in custom policies. Not sure, but I think this is something which must be checked if the REST API authorizations are enabled by default now, at least the privileges which have nothing to do with the GraphQL endpoints (as the comment linked above suggests, although I do not fully understand the logic why something should or should not be included in the list) should be available as privileges in the UI: RESTORE_INDICES_PRIVILEGE edit: Maybe all platform privileges could be included in the PLATFORM_PRIVILEGES list without having an impact on the GraphQL endpoints? |
@Masterchen09 - There is some history with how the policy privileges have evolved over time. Initially, APIs like OpenAPI and Rest.li were authenticated only with no authorization checks. The initial implementation of OpenAPI/Rest.li was built similar to graphql privileges where they were higher level and usually simple allow or deny. This simplicity didn't satisfy more complex access control rules which needed CRUD semantics for authorization. This was added later to all APIs (where possible for graphql) in order to be able to have a somewhat consistent authorization policy across all APIs. The privileges listed are privileges that were created for either OpenAPI or Rest.li and since these APIs were considered separate from graphql /UI they were never added to the UI to prevent confusion because there is no way to exercise them in the UI or via graphql. While those privileges are not part of the UI, you can create policies via APIs to include them. As you pointed out the typical users for those privileges are admin or Admin role only and are not exactly useful for anyone but a user of the OpenAPI or Rest.li APIs (this does include the cli users). Hopefully that explains how we got to this point. As to whether we should include these in the UI, even though they are not part of the UI experience, I'd want to get input from product @chriscollins3456 @jjoyce0510 |
I'd have to review some of those privileges, however it might be less confusing to add like |
See #11549 |
Checklist