Always create new identity for view owner #15111

kokosing · 2022-11-18T21:04:26Z

No description provided.

kokosing · 2022-11-18T21:04:58Z

kokosing · 2022-11-21T08:35:10Z

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

                    identity = Identity.from(owner.get())
                            .withGroups(groupProvider.getGroups(owner.get().getUser()))
                            .build();
-                    viewAccessControl = new ViewAccessControl(accessControl, session.getIdentity());
+                    if (owner.get().getUser().equals(session.getIdentity().getUser())) {


thanks to this if view owner does not require GRANT OPTION to access base tables. Do you think I should update the comment?

Yeah, "don't restrict owner's access" is kinda vague... And this rule that view owner does not require grant option is kinda obscure. Probably a good idea to clarify.

I agree that "Always create new identity for view owner" conveys no meaning. It's not terrible -- a person reading commit log may understand that the title doesn't hint and the change meaning and will proably read the full message. but we could do better.

I improved commit message too.

core/trino-main/src/test/java/io/trino/server/TestHttpRequestSessionContextFactory.java

core/trino-main/src/main/java/io/trino/server/testing/TestingSystemSecurityMetadata.java

findepi · 2022-11-28T09:00:16Z

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

                    identity = Identity.from(owner.get())
                            .withGroups(groupProvider.getGroups(owner.get().getUser()))
                            .build();
-                    viewAccessControl = new ViewAccessControl(accessControl, session.getIdentity());
+                    if (owner.get().getUser().equals(session.getIdentity().getUser())) {


I agree that "Always create new identity for view owner" conveys no meaning. It's not terrible -- a person reading commit log may understand that the title doesn't hint and the change meaning and will proably read the full message. but we could do better.

findepi · 2022-11-28T09:01:17Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+                .setAdditionalModule(binder -> {
+                    newOptionalBinder(binder, SystemSecurityMetadata.class)
+                            .setBinding()
+                            .to(TestingSystemSecurityMetadata.class)


Does this change meaning of all pre-existing tests in this class?

No. Other tests rely on TestingAccessControlManager which controls testing privileges.

Also notice that by default we were using DisabledSystemSecurityMetadata which was returning empty for getViewRunAsIdentity. This works the same way with TestingSystemSecurityMetadata as nobody calls setViewOwner directly.

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

findepi · 2022-11-28T09:02:58Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+                // TestingAccessControlManager works by denying accesses, so:
+                // * viewOwnerSession does not have deniedrole enabled
+                // * the owner has a role grant to deniedrole
+                // we deny access to deniedrole, so:
+                // * even though viewOwnerSession would be allowed,
+                // * the session created for view owner will be denied


Are we testing TestingAccessControlManager? I..e. testing the testing code?

No really. But we need mocks to work as real implementation. I will improve a comment. It is a bit scary and moves focus towards testing code.

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

findepi · 2022-11-28T09:04:30Z

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

@@ -4522,7 +4522,7 @@ private void analyzeRowFilter(String currentIdentity, Table table, QualifiedObje
            ExpressionAnalysis expressionAnalysis;
            try {
                expressionAnalysis = ExpressionAnalyzer.analyzeExpression(
-                        createViewSession(filter.getCatalog(), filter.getSchema(), Identity.forUser(filter.getIdentity()).build(), session.getPath()), // TODO: path should be included in row filter
+                        createViewSession(filter.getCatalog(), filter.getSchema(), Identity.ofUser(filter.getIdentity()), session.getPath()), // TODO: path should be included in row filter


👍
Let's maybe move it to separate PR?

core/trino-main/src/test/java/io/trino/server/TestHttpRequestSessionContextFactory.java

core/trino-spi/src/main/java/io/trino/spi/security/Identity.java

core/trino-main/src/main/java/io/trino/security/AccessControlUtil.java

core/trino-spi/src/test/java/io/trino/spi/security/TestIdentity.java

kokosing

Addressed comments. Please take a look.

core/trino-main/src/main/java/io/trino/server/testing/TestingSystemSecurityMetadata.java

kokosing · 2022-12-01T13:08:53Z

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

                    identity = Identity.from(owner.get())
                            .withGroups(groupProvider.getGroups(owner.get().getUser()))
                            .build();
-                    viewAccessControl = new ViewAccessControl(accessControl, session.getIdentity());
+                    if (owner.get().getUser().equals(session.getIdentity().getUser())) {


I improved commit message too.

kokosing · 2022-12-01T13:18:07Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+                .setAdditionalModule(binder -> {
+                    newOptionalBinder(binder, SystemSecurityMetadata.class)
+                            .setBinding()
+                            .to(TestingSystemSecurityMetadata.class)


No. Other tests rely on TestingAccessControlManager which controls testing privileges.

Also notice that by default we were using DisabledSystemSecurityMetadata which was returning empty for getViewRunAsIdentity. This works the same way with TestingSystemSecurityMetadata as nobody calls setViewOwner directly.

kokosing · 2022-12-02T11:41:03Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

+                // TestingAccessControlManager works by denying accesses, so:
+                // * viewOwnerSession does not have deniedrole enabled
+                // * the owner has a role grant to deniedrole
+                // we deny access to deniedrole, so:
+                // * even though viewOwnerSession would be allowed,
+                // * the session created for view owner will be denied


No really. But we need mocks to work as real implementation. I will improve a comment. It is a bit scary and moves focus towards testing code.

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

core/trino-main/src/test/java/io/trino/server/TestHttpRequestSessionContextFactory.java

core/trino-spi/src/main/java/io/trino/spi/security/Identity.java

core/trino-main/src/main/java/io/trino/security/AccessControlUtil.java

testing/trino-tests/src/test/java/io/trino/server/testing/TestSystemSecurityMetadata.java

The rationale is that, in addition to the user name, all of these can affect access control decisions for an `Identity`, so they should be considered distinct if any of those attributes differ. The `StatementAnalyzer`, during the analysis stage, collects all table references along with their corresponding access controls and identities. At the end of the analysis, this is used to do the access checks. They are collected in a `Map`, where the key is the pair of access control and `Identity`, so that we deduplicate the necessary checks. But for this deduplication to work properly, `Identity` instances that result in different accesses should be considered distinct. Remove obsolete AccessControlUtil.FullIdentityEquality. Now `Identity` itself has full identity.

The test is single-threaded so using `executeExclusively` is not much beneficial. Removing usage of that method makes a test way simplified.

Session user may have not always all roles enabled. Reusing session when session user equals view owner may end up with different access. Notice, that we can still reuse access control and not use dedicated view access control. This is because view owner does not need to grant access to itself, having access is just enough. I mean view owner does not requires privileges with GRANT OPTION to access objects used in the view. Co-authored-by: Grzegorz Kokosiński <grzegorz@starburstdata.com>

lukasz-walkiewicz · 2022-12-09T12:15:12Z

core/trino-main/src/main/java/io/trino/security/AccessControlUtil.java

        return queries.stream()
                .filter(queryInfo -> {
                    Identity queryIdentity = queryInfo.getSession().toIdentity();
-                    return queryIdentity.getUser().equals(identity.getUser()) || allowedOwners.contains(new FullIdentityEquality(queryIdentity));
+                    return queryIdentity.getUser().equals(identity.getUser()) || allowedOwners.contains(queryIdentity);


nit: previously it was a set whereas now it's just a collection so contains might be a bit slower (assuming the returned collection is not actually a set ofc)

colebow · 2022-12-14T20:29:26Z

Does this need a release note? @kokosing

kokosing · 2022-12-14T20:30:48Z

I don't think so.

cla-bot bot added the cla-signed label Nov 18, 2022

kokosing changed the title ~~Always create new view owner's identity~~ Always create new identity for view owner Nov 18, 2022

kokosing force-pushed the origin/master/159_view branch from 5426e6a to 2d57b0f Compare November 18, 2022 21:07

This was referenced Nov 18, 2022

Add most properties to Identity's hashCode and equals #14130

Closed

Use the view owner's identity even if the current user is the owner #13991

Closed

kokosing force-pushed the origin/master/159_view branch from 2d57b0f to 4cd7187 Compare November 19, 2022 20:54

kokosing commented Nov 21, 2022

View reviewed changes

kokosing force-pushed the origin/master/159_view branch from 4cd7187 to bee9b03 Compare November 21, 2022 11:42

kokosing requested review from martint and dain November 21, 2022 11:45

kokosing commented Nov 21, 2022

View reviewed changes

core/trino-main/src/test/java/io/trino/server/TestHttpRequestSessionContextFactory.java Show resolved Hide resolved

kokosing requested a review from findepi November 24, 2022 10:33

findepi reviewed Nov 28, 2022

View reviewed changes

kokosing mentioned this pull request Nov 29, 2022

Call access control once per check #15131

Closed

electrum reviewed Nov 30, 2022

View reviewed changes

core/trino-spi/src/test/java/io/trino/spi/security/TestIdentity.java Outdated Show resolved Hide resolved

kokosing force-pushed the origin/master/159_view branch from bee9b03 to 11984c7 Compare December 2, 2022 14:38

kokosing commented Dec 2, 2022

View reviewed changes

kokosing force-pushed the origin/master/159_view branch 2 times, most recently from 3743ed5 to f39a776 Compare December 7, 2022 12:57

findepi reviewed Dec 8, 2022

View reviewed changes

testing/trino-tests/src/test/java/io/trino/server/testing/TestSystemSecurityMetadata.java Outdated Show resolved Hide resolved

findepi approved these changes Dec 8, 2022

View reviewed changes

ksobolew and others added 3 commits December 8, 2022 23:47

Leverage that TestAccessControl is single-threaded

ca668eb

The test is single-threaded so using `executeExclusively` is not much beneficial. Removing usage of that method makes a test way simplified.

kokosing force-pushed the origin/master/159_view branch from f39a776 to 36934db Compare December 8, 2022 22:47

lukasz-walkiewicz approved these changes Dec 9, 2022

View reviewed changes

kokosing merged commit ebbb2fb into trinodb:master Dec 9, 2022

kokosing deleted the origin/master/159_view branch December 9, 2022 14:07

github-actions bot added this to the 404 milestone Dec 9, 2022

colebow mentioned this pull request Dec 14, 2022

Add Trino 405 release notes #15139

Merged

kokosing added the no-release-notes This pull request does not require release notes entry label Dec 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always create new identity for view owner #15111

Always create new identity for view owner #15111

kokosing commented Nov 18, 2022

kokosing commented Nov 18, 2022

kokosing Nov 21, 2022

ksobolew Nov 21, 2022

findepi Nov 28, 2022

kokosing Dec 1, 2022

findepi Nov 28, 2022

findepi Nov 28, 2022

kokosing Dec 1, 2022

findepi Nov 28, 2022

kokosing Dec 2, 2022

findepi Nov 28, 2022

kokosing left a comment

kokosing Dec 1, 2022

kokosing Dec 1, 2022

kokosing Dec 2, 2022

lukasz-walkiewicz Dec 9, 2022

colebow commented Dec 14, 2022

kokosing commented Dec 14, 2022

Always create new identity for view owner #15111

Always create new identity for view owner #15111

Conversation

kokosing commented Nov 18, 2022

kokosing commented Nov 18, 2022

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

kokosing 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

colebow commented Dec 14, 2022

kokosing commented Dec 14, 2022