Support for setting multiple roles at once with connection impersonation

[s-huk]

Describe the bug

We experience this bug when using Impersonation in Metabase Enterprise (see below how to reproduce more exactly):

class clojure.lang.PersistentVector cannot be cast to class java.lang.CharSequence (clojure.lang.PersistentVector is in unnamed module of loader 'app'; java.lang.CharSequence is in module java.base of loader 'bootstrap')
...
metabase_enterprise.advanced_permissions.driver.impersonation$connection_impersonation_role.invokeStatic(impersonation.clj:73)

This means that this line of Metabase Enterprise code crashes.

I delivered a solution proposal in PR #47246

To Reproduce

1. Set up impersonation - more or less as described here
2. Obtain the database role user attribute from SSO/Keycloak/JWT JSONs attributes like {"groups": ["myFirstGroup", "mySecondGroup"]}
3. See error

   class clojure.lang.PersistentVector cannot be cast to class java.lang.CharSequence (clojure.lang.PersistentVector is in unnamed module of loader 'app'; java.lang.CharSequence is in module java.base of loader 'bootstrap')
   ...
   metabase_enterprise.advanced_permissions.driver.impersonation$connection_impersonation_role.invokeStatic(impersonation.clj:73)

   This means that this line of Metabase Enterprise code crashes.

Expected behavior

Expected result would be at least a reasonable error, which data(type) is expected and at which point exactly input data is considered to be bad.

An even nicer and customer friendly solution would be to check the type of the role variable, and convert it as necessary.

Logs

class clojure.lang.PersistentVector cannot be cast to class java.lang.CharSequence (clojure.lang.PersistentVector is in unnamed module of loader 'app'; java.lang.CharSequence is in module java.base of loader 'bootstrap')
...
metabase_enterprise.advanced_permissions.driver.impersonation$connection_impersonation_role.invokeStatic(impersonation.clj:73)

Information about your Metabase installation

- Metabase version v1.50.21
- SSO via JWT/Keycloak/OIDC

Rather less relevant information:
- Metabase Hosting-Environment: OpenShift
- Metabase internal Database: MySQL
- Tested with metabase-clickhouse-driver

Severity

blocking

Additional context

No response

[paoliniluis]

Hi there, we should be reviewing and shipping the fix soon, but I wanted to check the use case here as I'm not fully getting why you would like to pass 2 roles to a database. What's the use case here?

[s-huk]

Hi, we use impersonation as described in this article (esp. "Impersonation sets permissions for questions written in both the SQL editor and the query builder").

Our Tables/Databases are partitioned into various domains or project groups. More precisely, this setup resembles a hypergraph where each hyperedge represents a role or project group. Depending on a user's participation in a project group, they are assigned the corresponding role, and users can hold multiple roles simultaneously. This approach helps us avoid the combinatorial explosion of database connections, while facilitating data joins across multiple project groups with proper authorization - regardless of whether users are utilizing the Metabase query builder or query editor.

[paoliniluis]

@s-huk you're using clickhouse as the DW is that correct?

[s-huk]

@paoliniluis yes, and we have already taken a look at the metabase-clickhouse-driver

[noahmoss]

Relabeled as feature request after discussion with product.

[s-huk]

Once again, I suggested a sound and very easy solution in PR #47246 (then you wouldn't have to create a separate issue for the bug and solve that one).

So, why don't we just merge this PR, and add this description to your role impersonation documentation:

If the database role attribute is not a string but a collection of strings (like a JSON array), then it will automatically be reduced to a comma-separated String. For exmaple, collections like ["a", "b", "c"] will automatically be reduced to "a,b,c".

This would make all of us happy, because...

• You wouldn't have to fix the bug anymore.
• Databases that can activate only one role
  • ...could work with text (as currently) or singleton collections as well
  • ...should return the most reasonable error message in case of multiple comma-separated values
• Databases which can activate more than one role...
  • ...could work with text (as currently) or comma-separated input as well

And existing drivers wouldn't have to be modified nor bothered, because reducing a collection of roles to a comma-separated role string aligns with the SQL syntax of every possible driver's SQL dialect.

Under the bottom line: Roughly nothing would have to be done, and you wouldn't have to impair the quality of your software. Or am I missing something?

[noahmoss]

@s-huk As I noted previously, concatenation of strings using commas is a driver implementation detail. Even if every driver does this in the same way, they may have different rules around quoting individual roles, or other syntax differences. For instance, our Postgres driver already will quote a role if it includes non-alphanumeric characters.

It's plausible that a future driver could have both requirements—multiple roles and special quoting rules—in which case your proposed implementation would not work. Or a future might have a different syntax for multiple roles altogether.

I'm open to the idea of updating driver/set-role! and driver/set-role-statement! to take a list of strings in place of a single string. If you'd like to submit a new PR with this approach (or another approach which doesn't have the issue above) I'm happy to review it.

[s-huk]

I will consult with my team to determine whether to participate here, comparing it to other possible solutions.

It sounds like you want to change the type, i.e., first overload the set-role-statement method somehow and then gradually phase out the non-set method from every possible driver.

[s-huk]

@noahmoss Before I contribute vainly to unnecessarily impairing the simplicity and clarity of the driver interface, I need to ask you again: Are you really sure you want to change the type of the role variable from a string to a collection of strings?

To reconsider this more clearly, let's focus on the following role input scenarios which can occur:

1. The role attribute is a string consisting of exactly one role.
2. The role attribute is a string consisting of multiple roles, comma-separated by the user.
3. The role attribute is a collection.

Instead of changing/extending the type of your interface's role variable, I would suggest simplifying scenario 3 into scenario 2.

As you point out, Metabase can't validate or transform input from scenario 2. Only the drivers can transform such strings according to their underlying database syntax, as you already mentioned. And why wouldn’t the drivers be able to handle this with, e.g., regular expressions instead of collections?

Therefore, I still recommend merging PR #47246 (which already maps scenario 3 into 2). Upon closer inspection, it still seems to be the most straightforward and less complicated solution. It works with all possible drivers as long as there is no more than one role involved (role input scenarios 1-3 would all be supported). It also already supports multiple roles in drivers like Clickhouse. As for Postgres, this system doesn't seem to support multiple roles anyway.

If there is a database with special quoting rules that supports multiple roles, we would simply need to adapt its driver to handle comma-separated strings - so what? (because this would have to be done anyways, to be able to handle input scenario 2)

[dpsutton]

Hi @s-huk

I've setup a user with the following login_attributes {:email "sandbox@metabase.com", :id 2, :login_attributes {"id" "1"}} and setup sandboxing on that attribute, i see the following

If I update the login attributes to mirror the proposed allowable shape here, {:email "sandbox@metabase.com", :id 2, :login_attributes {"id" ["1" "2"]}} the sandboxing breaks with an invalid shape.

We have logs that complain about the invalid shape

2024-09-06 11:21:43,499 WARN lib.normalize :: Error normalizing pMBQL:
...
2024-09-06 11:21:43,507 WARN lib.convert :: Clean: Removing bad clause in [:filters 0 2 3] due to error {:filters [[nil nil [nil nil nil ["should be nil" "should be a boolean" "should be a string" "should be an integer" "should be a double" "instance of java.lang.Float" "instance of java.math.BigDecimal" "instance of java.time.LocalDate" "date string literal" "local time string literal" "offset time string literal" "instance of java.time.LocalTime" "instance of java.time.OffsetTime" "local date time string literal" "offset date time string literal" "instance of java.time.LocalDateTime" "instance of java.time.OffsetDateTime" "instance of java.time.ZonedDateTime"]]]]}:
{:filters [[nil nil [nil nil nil :1]]]}

2024-09-06 11:21:43,508 WARN lib.convert :: Clean: Removing bad clause in [:filters 0 2 3] due to error :malli.core/end-of-input:
{:filters
 [[nil
   nil
   [nil {:lib/uuid "2c859dba-33cb-4e97-9a23-92eb8868b4b0"} [nil {:lib/uuid "1ff83655-552a-45bd-958e-0ee67e6764aa"}]]
   [:=
    {:lib/uuid "acfbd23b-c4b6-472e-aea4-15c09b5df9f2"}
    [:field {:base-type :type/BigInteger, :lib/uuid "c5c77b10-c3fb-44ed-91ae-7dbabea7a36f"} 62]
    2]]]}

2024-09-06 11:21:43,508 WARN lib.convert :: Clean: Removing bad clause in [:filters 0 3] due to error :malli.core/end-of-input:
{:filters
 [[:or
   {:lib/uuid "9fe2e75c-3b6f-4d8d-8596-722f5b68d79f"}
   [:=
    {:lib/uuid "acfbd23b-c4b6-472e-aea4-15c09b5df9f2"}
    [:field {:base-type :type/BigInteger, :lib/uuid "c5c77b10-c3fb-44ed-91ae-7dbabea7a36f"} 62]
    2]]]}

2024-09-06 11:21:43,508 WARN lib.convert :: Clean: Removing bad clause in [:filters] due to error :malli.core/limits:
{:filters []}

And I think we have to decline the mechanism you are proposing. It is changing a data shape that flows throughout our system and you are only thinking of the consequences of it in one 3rd party driver. As per your statement "Do you really insist on spaghettifying type insecurity into all possible drivers?", I think we are carefully considering the changes we allow to the product and a change letting a string randomly be a collection would actually spagettify type insecurity into a lot of code paths.

Again, I appreciate you engaging with a PR and we will consider how to land this feature. But we will not do so at the expense of silently breaking sandboxing.

[s-huk]

@dpsutton Hi, I didnt suggest "letting a string randomly be a collection" (just @noahmoss seems to think in this direction). PR #47246 solely proposes to homogenize it to a string if it accidentally turns out to be a collection. So sticking with your example, I would rather suggest to transform something like {:email "sandbox@metabase.com", :id 2, :login_attributes {"id" ["1" "2"]}} to {:email "sandbox@metabase.com", :id 2, :login_attributes {"id" "1,2"}}.

[Gerrit-K]

Chiming in from a different company, but with a very similar setup as @s-huk: we use Metabase Enterprise behind Keycloak to query data from ClickHouse. This bug (/ missing feature, if you like) is currently blocking our rollout of Connection Impersonation, which was one of the key features for us to buy the Enterprise license in the first place.

It's a pity that the existence of multiple roles was not considered for the first concept of this feature, because I think it's a rather common case. Users often have not just one distinct but many different, potentially even overlapping roles set via their IdP and all the DBMS I've come across support users with multiple roles, so IMO Metabase should support that, too.

Regarding the history of this ticket and the associated discussions, I can understand the arguments on both sides. I agree that joining the roles by a comma should be the responsibility of the specific driver instead of Metabase itself, but I also don't see that this (temporary?) bugfix would significantly impair functionality or pose a risk. The method was designed with one input role and one output string in mind, so it was never expected to work with a vector of roles anyways. Thus, I would not expect that changing the (unintended) vector output back to a string output breaks anything. Of course, this should be properly documented and tracked with an according cleanup task once the design plan is revised for multi-role scenarios.

But for now, we would really appreciate a short-term fix to unblock our transition to Metabase Enterprise w/ Connection Impersonation. Regarding the workaround as recommended on this PR comment:

Until we support this use case officially, you may be able to work around this by storing the concatenated list of roles as a string directly in the user attributes.

To my knowledge, this is not easily doable in Keycloak. It would require us to implement a custom User Attribute Mapper or a Script Mapper because Keycloak does not provide this functionality out of the box with its standard mappers.

[paoliniluis]

@Gerrit-K please contact our helpdesk

[s-huk]

@Gerrit-K Thank you for pointing this out (exactly my thoughts too):

The method was designed with one input role and one output string in mind, so it was never expected to work with a vector of roles anyways. Thus, I would not expect that changing the (unintended) vector output back to a string output breaks anything. Of course, this should be properly documented and tracked with an according cleanup task once the design plan is revised for multi-role scenarios.

@dpsutton @noahmoss Sounds like a good plan to proceed, doesnt it?

And, yes, this is exactly what we have to deal with too:

To my knowledge, this is not easily doable in Keycloak. It would require us to implement a custom User Attribute Mapper or a Script Mapper because Keycloak does not provide this functionality out of the box with its standard mappers.