Add AuthAdmin #1318

brfrn169 · 2023-11-23T17:26:10Z

Description

This PR adds the AuthAdmin interface for administrative operations related to ScalarDB Auth (authentication and authorization). As ScalarDB Auth is an enterprise feature, this PR only introduces the interface without any implementations in this repository.

Related issues and/or PRs

N/A

Changes made

Added the AuthAdmin interface for administrative operations for ScalarDB
Modify exceptions to support authentication and authorization errors

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)

Documentation for this will be added in a separate PR.

Release notes

N/A

brfrn169 · 2023-11-23T17:34:44Z

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

+ * An interface for administrative operations for authentication and authorization. This interface
+ * is used for creating and dropping users, and granting and revoking privileges.
+ */
+public interface AuthAdmin {


Added AuthAdmin for administrative operations for ScalarDB Auth.

brfrn169 · 2023-11-23T17:35:19Z

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

@@ -8,7 +8,7 @@
 * An administrative interface for distributed transaction implementations. The user can execute
 * administrative operations with it like createNamespace/createTable/getTableMetadata.
 */
-public interface DistributedTransactionAdmin extends Admin {
+public interface DistributedTransactionAdmin extends Admin, AuthAdmin {


Made DistributedTransactionAdmin extend AuthAdmin.

brfrn169 · 2023-11-23T17:38:41Z

core/src/main/java/com/scalar/db/exception/storage/ExecutionException.java

+  private final boolean authenticationError;
+  private final boolean authorizationError;
+  private final boolean superuserRequired;
+  @Nullable private final AuthAdmin.Privilege requiredPrivilege;


Added flags to the exceptions to indicate authentication and authorization errors. Initially, I considered introducing new exceptions for these errors, but to maintain backward compatibility, I opted for this approach.

brfrn169 · 2023-11-23T17:39:49Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java

+  @Override
+  public void createUser(String username, @Nullable String password, UserOption... userOption) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public void alterUser(String username, @Nullable String password, UserOption... userOption) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public void dropUser(String username) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public void grant(String username, String namespaceName, Privilege... privileges) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public void grant(
+      String username, String namespaceName, String tableName, Privilege... privileges) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public void revoke(String username, String namespaceName, Privilege... privileges) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public void revoke(
+      String username, String namespaceName, String tableName, Privilege... privileges) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public List<User> getUsers() {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public EnumSet<Privilege> getPrivileges(String username, String namespaceName) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }
+
+  @Override
+  public EnumSet<Privilege> getPrivileges(String username, String namespaceName, String tableName) {
+    throw new UnsupportedOperationException("Not supported in the community edition");
+  }


No implementations are added here as it's an enterprise feature.

komamitsu · 2023-11-24T02:51:43Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java

@@ -240,6 +243,58 @@ public void upgrade(Map<String, String> options) throws ExecutionException {
    admin.upgrade(options);
  }

+  @Override
+  public void createUser(String username, @Nullable String password, UserOption... userOption) {
+    throw new UnsupportedOperationException("Not supported in the community edition");


How about moving these default implementations throwing the exception to AuthAdmin as default methods so that all classes which inherit AuthAdmin don't need to implement these ones?

I had the same thought, but I didn't do that because I was not sure for which methods we should add a default implementation. In other words, should we add default implementations for the existing methods (createTable(), getTableMetadata(), etc.) as well? What do you think?

I think only the methods of AuthAdmin should have default methods to throw the exception since those features aren't supported in the community edition. As for the existing methods coming from DistributedTransactionAdmin, we should keep them available in the community edition and don't need default impl for them. But I might be missing something on the background of this auth feature.

Okay, I think that's reasonable. Let me add default implementations to the methods in AuthAdmin. Thanks.

Fixed in ca46a9d. Thanks.

komamitsu · 2023-11-24T02:55:51Z

core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionAdmin.java

+
+  @Override
+  public void createUser(String username, @Nullable String password, UserOption... userOption) {
+    throw new UnsupportedOperationException();


This exception should have the same error message as the exceptions ConsensusCommitAdmin's methods throw?

Initially, I didn't add any message here because we were going to remove the JDBC transaction and ScalarDB Server components. However, recent discussions have raised the possibility of retaining the JDBC transaction. Therefore, I will add a message depending on the outcome of this discussion:
#1318 (comment)

Thanks.

komamitsu · 2023-11-24T03:04:56Z

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

+  }
+
+  /** The privileges for ScalarDB operations. */
+  enum Privilege {


This seems like an union of privileges for user (admin) operations, namespace operations and table operations. For instance, GRANT is for user (admin) operations and TRUNCATE is for table operations. Another option is to separate this set of privileges into each type of operations like UserPrivilege, NamespacePrivilege and TablePrivilege. This option requires additional enum definitions, but it might beneficial in the long run to avoid wrong privilege usage (e.g, TRUNCATE for a user). What do you think?

Let me clarify the specification to ensure we are on the same page.

Currently, we can grant privileges to a user for either a specific namespace or a table. I think we can imagine the SQL GRANT statement as follows:

For table privileges:

GRANT READ, WRITE, CREATE ON <TABLE> TO <USER> WITH GRANT OPTION

In this case, the user has READ, WRITE, CREATE and GRANT privileges for the specified table.

READ: The user can read the table.

WRITE: The user can write the table.

CREATE: The user can create indexes on the table.

GRANT: The user can grant privileges on the table to other users.

For namespace privileges:

GRANT READ, WRITE, CREATE ON NAMESPACE <NAMESPACE> TO <USER> WITH GRANT OPTION

In this case, the user has READ, WRITE, CREATE and GRANT privileges for all tables in the namespace.

READ: The user can read all tables in the namespace.

WRITE: The user can write all tables in the namespace.

CREATE: The user can create a table in the namespace or create indexes on all tables in the namespace.

GRANT: The user can grant privileges on all tables in the namespace to other users.

Given this, I believe the approach is consistent and there's no need for separate privilege types for tables, namespaces, and users. What are your thoughts?

Thanks for clarifying the usage. I understand how to use GRANT.

GRANT privilege seems a bit different type (more metadata-like?) attribute to me from other privileges (e.g. CREATE). It's like between UserOption and Privilege? It might fit PrivilegeOption.

In this case, the user has READ, WRITE, CREATE and GRANT privileges for all tables in the namespace.

Understood. So, the enum is like TablePrivilege. BTW, we don't need to another type of privileges to allow or prohibit operations for namespaces like GRANT CREATE ON DATABASE?

Ah sorry, the explanation of GRANT for namespace privileges was wrong.

The correct explanation is as follows

GRANT: The user can grant/revoke privileges on all tables in the namespace to/from other users. Also the user can grant/revoke privileges on the namespace to/from other users.

So for example, if a superuser issues the following:

GRANT ... ON NAMESPACE ns TO user1 WITH GRANT OPTION

Then user1 can issue GRANT... ON NAMESPACE ns to ... for other users. Also, user1 can issue GRANT on all tables in the ns namespace for other users.

What do you think?

One more thing, if a superuser issues the following GRANT command:

GRANT DROP ON NAMESPACE ns TO user1

Then, user1 can drop the ns namespace as well as all tables in the ns namespace.

That sounds inconsistent to you?

GRANT DROP ON NAMESPACE ns TO user1

As we talked offline, this semantics of dropping a specific namespace seems inconsistent with other GRANT statements that affect tables. Perhaps, we can defer considering how to permit/prohibit manipulation of namespace.

As for Privilege.GRANT, I still think it might belong to a different layer, but I don't have a so strong opinion on it. Let's proceed with the current design.

Thank you for summarizing the discussion. I've fixed the Javadoc slightly and renamed several methods.

I think we can support something like the following grammar in ScalarDB SQL:

GRANT { { SELECT | INSERT | UPDATE | DELETE | CREATE | DROP | TRUNCATE | ALTER } [, ...] | ALL [ PRIVILEGES ] } ON { [ TABLE ] table_name [, ...] | ON NAMESPACE namespace_name [, ...] } TO user_name [, ...] [ WITH GRANT OPTION ]

If a user specified ON [TABLE] table_name in the GRANT statement, we call the AuthAdmin.grant(String username, String namespaceName, String tableName, Privilege... privileges) method. And if a user specified ON NAMESPACE namespace_name in the GRANT statement, we call the AuthAdmin.grant(String username, String namespaceName, Privilege... privileges) method.

Also, if a user specified WITH GRANT OPTION and ON [TABLE] table_name in the GRANT statement, the granted user can issue GRANT statements, and they can grant privileges on the specified table to other users.

And if a user specified WITH GRANT OPTION and ALL TABLES IN NAMESPACE namespace_name in the GRANT statement, they can grant privileges on all tables in the specified namespace to other users.

Note that users can only grant privileges that they have to other users.

Does that make sense?

Yes. The syntax of the GRANT statement and the behavior looks good 👍

As for Privilege.GRANT, I still think it might belong to a different layer, but I don't have a so strong opinion on it. Let's proceed with the current design.

This comment only considers the internal design and implementation since I still think WITH GRANT OPTION is a sort of option not a normal privilege. The above GRANT statement syntax itself also seems to handle GRANT option differently from other privileges (e.g., UPDATE). I checked the PostgreSQL source code, and it doesn't handle GRANT OPTION as well as other privileges https://github.com/postgres/postgres/blob/5ad49322e50459444b201e624d5df706751c9d41/src/include/nodes/parsenodes.h#L74-L93. So, IMO, maybe Privilege.GRANT should be removed from the enum and handled differently, although it might be a matter of taste.

As discussed offline, we have reached a conclusion regarding the behavior mentioned in the following comment:
#1318 (comment)

I've also revised the Javadoc and revered the method names in ad52e62.

@komamitsu Can you please take a quick look at it? Thanks.

Thanks. Looks good!

komamitsu

LGTM! 👍

feeblefakie

Overall, looking good. Thank you!
I left one comment. PTAL!

feeblefakie · 2023-11-28T07:26:49Z

core/src/main/java/com/scalar/db/exception/transaction/AbortException.java

+      String message,
+      Throwable cause,
+      @Nullable String transactionId,
+      boolean authenticationError) {


Instead of adding an argument only for authentication, it might be better to define an error code and add it here or something?
Since we don't define error code yet, I understand it's hard to do so now...

Yeah, at the moment, it's hard to do so since we need to define error codes for all errors for that. Anyway, I think we can add error codes later on.

feeblefakie

LGTM! Thank you!

brfrn169 added the enhancement New feature or request label Nov 23, 2023

brfrn169 self-assigned this Nov 23, 2023

brfrn169 commented Nov 23, 2023

View reviewed changes

Add AuthAdmin

d38d93f

brfrn169 force-pushed the add-auth-admin branch from d0a3149 to d38d93f Compare November 23, 2023 17:59

brfrn169 requested review from komamitsu, feeblefakie and Torch3333 November 23, 2023 17:59

komamitsu reviewed Nov 24, 2023

View reviewed changes

Add default implementations to AuthAdmin

ca46a9d

brfrn169 requested a review from komamitsu November 24, 2023 09:40

Fix

b8c724e

brfrn169 force-pushed the add-auth-admin branch from 6345ceb to b8c724e Compare November 25, 2023 18:26

brfrn169 added 2 commits November 27, 2023 01:50

Refactor exceptions

0599d70

Fix comments for Privilege

1fbde60

komamitsu approved these changes Nov 27, 2023

View reviewed changes

brfrn169 added 2 commits November 28, 2023 09:22

Fix Javadoc

67d8215

Rename several methods

e43fc39

brfrn169 force-pushed the add-auth-admin branch from 3485cd6 to e43fc39 Compare November 28, 2023 00:58

Fix Javadoc and revert method names

ad52e62

feeblefakie reviewed Nov 28, 2023

View reviewed changes

brfrn169 requested a review from feeblefakie November 28, 2023 07:37

feeblefakie approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'master' into add-auth-admin

6c9beb1

brfrn169 merged commit 2de8433 into master Dec 1, 2023
23 checks passed

feeblefakie pushed a commit that referenced this pull request Dec 1, 2023

Add AuthAdmin (#1318)

68d0160

feeblefakie mentioned this pull request Dec 1, 2023

Backport to branch(3) : Add AuthAdmin #1340

Merged

brfrn169 deleted the add-auth-admin branch December 1, 2023 07:58

brfrn169 mentioned this pull request Dec 12, 2023

Add getUser() to AuthAdmin #1380

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AuthAdmin #1318

Add AuthAdmin #1318

brfrn169 commented Nov 23, 2023

brfrn169 Nov 23, 2023

brfrn169 Nov 23, 2023

brfrn169 Nov 23, 2023

brfrn169 Nov 23, 2023

komamitsu Nov 24, 2023

brfrn169 Nov 24, 2023

komamitsu Nov 24, 2023

brfrn169 Nov 24, 2023

brfrn169 Nov 24, 2023

komamitsu Nov 24, 2023

brfrn169 Nov 24, 2023 •

edited

Loading

komamitsu Nov 24, 2023

brfrn169 Nov 24, 2023 •

edited

Loading

komamitsu Nov 24, 2023

brfrn169 Nov 24, 2023 •

edited

Loading

brfrn169 Nov 27, 2023

komamitsu Nov 27, 2023

brfrn169 Nov 28, 2023 •

edited

Loading

komamitsu Nov 28, 2023

brfrn169 Nov 28, 2023 •

edited

Loading

komamitsu Nov 28, 2023

komamitsu left a comment

feeblefakie left a comment

feeblefakie Nov 28, 2023

brfrn169 Nov 28, 2023 •

edited

Loading

feeblefakie left a comment

Add AuthAdmin #1318

Add AuthAdmin #1318

Conversation

brfrn169 commented Nov 23, 2023

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

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 Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Nov 28, 2023 • edited Loading

Choose a reason for hiding this 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

brfrn169 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

brfrn169 Nov 24, 2023 •

edited

Loading

brfrn169 Nov 24, 2023 •

edited

Loading

brfrn169 Nov 24, 2023 •

edited

Loading

brfrn169 Nov 28, 2023 •

edited

Loading

brfrn169 Nov 28, 2023 •

edited

Loading

brfrn169 Nov 28, 2023 •

edited

Loading