Add admin interface and operation attributes things for Attribute-Based Access Control #2405

brfrn169 · 2024-12-13T09:39:33Z

Description

This PR adds the admin interface and operation attributes things for Attribute-Based Access Control (ABAC).

Related issues and/or PRs

N/A

Changes made

Added AbacAdmin and made DistributedTransactionAdmin extend it.
Added AbacOperationAttributes to manipulate the operation attributes for ABAC.
Support the operation attributes for ABAC in the operation builders.

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

…ed Access Control

brfrn169 · 2024-12-13T09:46:03Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

+  ABAC_NOT_ENABLED(
+      Category.USER_ERROR,
+      "0151",
+      "The Attribute-Based Access Control feature is not enabled. To use this feature, you must enable it. Note that this feature is supported only in the ScalarDB Enterprise edition",
+      "",
+      ""),


@josh-wong Could you please take a look at this message? 🙇

Similar to many of our other features (group commit, encryption, etc.), should we lowercase the naming for this feature as follows?:

Suggested change

ABAC_NOT_ENABLED(

Category.USER_ERROR,

"0151",

"The Attribute-Based Access Control feature is not enabled. To use this feature, you must enable it. Note that this feature is supported only in the ScalarDB Enterprise edition",

"",

""),

ABAC_NOT_ENABLED(

Category.USER_ERROR,

"0151",

"The attribute-based access control feature is not enabled. To use this feature, you must enable it. Note that this feature is supported only in the ScalarDB Enterprise edition",

"",

""),

Fixed in 11f38db. Thanks!

josh-wong

I've added one comment. PTAL!

josh-wong · 2024-12-13T09:50:00Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

+  ABAC_NOT_ENABLED(
+      Category.USER_ERROR,
+      "0151",
+      "The Attribute-Based Access Control feature is not enabled. To use this feature, you must enable it. Note that this feature is supported only in the ScalarDB Enterprise edition",
+      "",
+      ""),


Similar to many of our other features (group commit, encryption, etc.), should we lowercase the naming for this feature as follows?:

Suggested change

ABAC_NOT_ENABLED(

Category.USER_ERROR,

"0151",

"The Attribute-Based Access Control feature is not enabled. To use this feature, you must enable it. Note that this feature is supported only in the ScalarDB Enterprise edition",

"",

""),

ABAC_NOT_ENABLED(

Category.USER_ERROR,

"0151",

"The attribute-based access control feature is not enabled. To use this feature, you must enable it. Note that this feature is supported only in the ScalarDB Enterprise edition",

"",

""),

komamitsu · 2024-12-17T02:16:37Z

core/src/main/java/com/scalar/db/api/AbacAdmin.java

+   * @param policyName the policy name
+   * @param username the username
+   * @param levelShortName the level short name
+   * @param defaultLevelShortName the default level short name. If null, the default is the level


If null, the default is the level

What does "the level" exactly mean?

"the level" refers to the level specified by the argument levelShortName. The sentence might be confusing. 😓 Do you have any ideas for it?

This parameter is optional actually, and levelShortName is used instead like as a default value if defaultLevelShortName is null. So, I think the prefix of this parameter default is confusing...

If you can't change the parameter name, how about "If null, levelShortName is used instead" ?

Fixed in 661d076. Thanks.

komamitsu · 2024-12-17T02:17:19Z

core/src/main/java/com/scalar/db/api/AbacAdmin.java

+   * @param username the username
+   * @param levelShortName the level short name
+   * @param defaultLevelShortName the default level short name. If null, the default is the level
+   * @param rowLevelShortName the row level short name. If null, the default is the default level


If null, the default is the default level

The first "the default" is different from the second "the default"?

Correct.

The first "the default" means the default value of the rowLevelShortName. And the second means the default level specified by the argument defaultLevelShortName.

I think it feels confusing. How about using the actual parameter names like "If null, defaultLevelShortName is used instead" ?

And the second means the default level specified by the argument defaultLevelShortName.

defaultLevelShortName can be null. If it's null, levelShortName is used?

Fixed in 661d076. Thanks.

komamitsu · 2024-12-17T07:17:32Z

core/src/main/java/com/scalar/db/api/AbacAdmin.java

+       *
+       * @return the level short name
+       */
+      String getLevelShortName();


I guess User Tag should have MaxLevel and MinLevel. What do you think?

In ABAC in ScalarDB, we don’t have MinLevel. We only have MaxLevel, which is referred to simply as level.

core/src/main/java/com/scalar/db/api/AbacOperationAttributes.java

…tributes.java Co-authored-by: Mitsunori Komatsu <[email protected]>

josh-wong

I left just a minor suggestion regarding the capitalization of attribute-based access control in code comments. Other than that, LGTM. Thank you!🙇🏻‍♂️

core/src/main/java/com/scalar/db/api/AbacAdmin.java

komamitsu

LGTM! 👍

feeblefakie

Thank you!
Left some comments and suggestions.
The suggestions are mainly for the Javadoc comments.
PTAL!

core/src/main/java/com/scalar/db/api/AbacAdmin.java

feeblefakie · 2024-12-19T02:20:41Z

core/src/main/java/com/scalar/db/api/UpsertBuilder.java

+    }
+
+    @Override
+    public Buildable writeTag(String policyName, String writeTag) {


Just a confirmation. The reason for not having writeTag in InsertBuilder is that insertion uses the default tag?

Actually, the writeTag is not the rowTag. The writeTag is used for access control on existing records. Since insert operations involve adding new records, the writeTag is not applicable to them. Therefore, the InsertBuilder does not include the writeTag. Thanks.

Co-authored-by: Hiroyuki Yamada <[email protected]>

brfrn169 · 2024-12-23T04:50:50Z

core/src/main/java/com/scalar/db/api/AbacAdmin.java

+   * @param tableName the table name
+   * @throws ExecutionException if the operation fails
+   */
+  default void applyPolicyToTable(String policyName, String namespaceName, String tableName)


Renamed this method in 2c387a7.

brfrn169 · 2024-12-23T04:51:09Z

core/src/main/java/com/scalar/db/api/AbacAdmin.java

+   * @param namespaceName the namespace name
+   * @throws ExecutionException if the operation fails
+   */
+  default void applyPolicyToNamespace(String policyName, String namespaceName)


Renamed this method in 2c387a7.

feeblefakie

LGTM! Thank you!

Torch3333

I had comments regarding the Javadoc, besides this LGTM.

core/src/main/java/com/scalar/db/api/AbacAdmin.java

Co-authored-by: Vincent Guilpain <[email protected]>

Torch3333

LGTM, thank you!

…ed Access Control (#2405)

Add admin interface and operation attributes things for Attribute-Bas…

67721c6

…ed Access Control

brfrn169 added the enhancement New feature or request label Dec 13, 2024

brfrn169 self-assigned this Dec 13, 2024

brfrn169 requested review from komamitsu, feeblefakie, Torch3333 and josh-wong December 13, 2024 09:45

brfrn169 commented Dec 13, 2024

View reviewed changes

josh-wong reviewed Dec 13, 2024

View reviewed changes

komamitsu reviewed Dec 17, 2024

View reviewed changes

brfrn169 added 2 commits December 18, 2024 13:38

Merge branch 'master' into add-abac

b5f94b3

Fix based on feedback

11f38db

brfrn169 requested a review from josh-wong December 18, 2024 04:40

[skip ci] Update core/src/main/java/com/scalar/db/api/AbacOperationAt…

dd38c2a

…tributes.java Co-authored-by: Mitsunori Komatsu <[email protected]>

brfrn169 requested a review from komamitsu December 18, 2024 04:50

josh-wong approved these changes Dec 18, 2024

View reviewed changes

core/src/main/java/com/scalar/db/api/AbacAdmin.java Outdated Show resolved Hide resolved

brfrn169 added 2 commits December 18, 2024 16:00

Fix based on feedback

967d7bd

Fix Javadoc of AbacAdmin

661d076

komamitsu approved these changes Dec 18, 2024

View reviewed changes

feeblefakie reviewed Dec 19, 2024

View reviewed changes

brfrn169 and others added 3 commits December 20, 2024 14:59

[skip ci] Apply suggestions from code review

07b5e09

Co-authored-by: Hiroyuki Yamada <[email protected]>

Fix

da47a33

Rename method names

2c387a7

brfrn169 commented Dec 23, 2024

View reviewed changes

brfrn169 requested a review from feeblefakie December 23, 2024 05:13

brfrn169 added 2 commits December 23, 2024 16:28

Add methods to AbacAdmin

d0fa05f

Fix

960cf2b

feeblefakie approved these changes Dec 23, 2024

View reviewed changes

Torch3333 reviewed Dec 24, 2024

View reviewed changes

core/src/main/java/com/scalar/db/api/AbacAdmin.java Outdated Show resolved Hide resolved

core/src/main/java/com/scalar/db/api/AbacAdmin.java Outdated Show resolved Hide resolved

[skip ci] Apply suggestions from code review

c4e523a

Co-authored-by: Vincent Guilpain <[email protected]>

brfrn169 requested a review from Torch3333 December 24, 2024 08:56

Merge branch 'master' into add-abac

e489ce1

Torch3333 approved these changes Dec 24, 2024

View reviewed changes

brfrn169 merged commit e982d74 into master Dec 24, 2024
48 checks passed

brfrn169 deleted the add-abac branch December 24, 2024 09:51

feeblefakie pushed a commit that referenced this pull request Dec 24, 2024

Add admin interface and operation attributes things for Attribute-Bas…

e34571f

…ed Access Control (#2405)

feeblefakie mentioned this pull request Dec 24, 2024

Backport to branch(3) : Add admin interface and operation attributes things for Attribute-Based Access Control #2441

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add admin interface and operation attributes things for Attribute-Based Access Control #2405

Add admin interface and operation attributes things for Attribute-Based Access Control #2405

brfrn169 commented Dec 13, 2024

brfrn169 Dec 13, 2024

josh-wong Dec 13, 2024

brfrn169 Dec 18, 2024

josh-wong left a comment •

edited

Loading

josh-wong Dec 13, 2024

komamitsu Dec 17, 2024

brfrn169 Dec 18, 2024

komamitsu Dec 18, 2024

brfrn169 Dec 18, 2024

komamitsu Dec 17, 2024

brfrn169 Dec 18, 2024

komamitsu Dec 18, 2024

komamitsu Dec 18, 2024

brfrn169 Dec 18, 2024 •

edited

Loading

komamitsu Dec 17, 2024

brfrn169 Dec 18, 2024

josh-wong left a comment

komamitsu left a comment

feeblefakie left a comment

feeblefakie Dec 19, 2024

brfrn169 Dec 23, 2024

brfrn169 Dec 23, 2024

brfrn169 Dec 23, 2024

feeblefakie left a comment

Torch3333 left a comment

Torch3333 left a comment

Add admin interface and operation attributes things for Attribute-Based Access Control #2405

Add admin interface and operation attributes things for Attribute-Based Access Control #2405

Conversation

brfrn169 commented Dec 13, 2024

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-wong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-wong left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

josh-wong left a comment •

edited

Loading

brfrn169 Dec 18, 2024 •

edited

Loading