Skip to content
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

sql: enable changing the current user of a session with SET ROLE #15005

Closed
jordanlewis opened this issue Apr 17, 2017 · 12 comments · Fixed by #68973
Closed

sql: enable changing the current user of a session with SET ROLE #15005

jordanlewis opened this issue Apr 17, 2017 · 12 comments · Fixed by #68973
Assignees
Labels
A-security A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-graphile Issues relating to graphile compatibility C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@jordanlewis
Copy link
Member

jordanlewis commented Apr 17, 2017

Add support for the SET ROLE command and variants from PostgreSQL. This command sets or resets the user for the current session.

This should also support SET LOCAL ROLE, which changes the role just for the current transaction. This is related to, but not dependent on SET LOCAL for session variables: #32562

At least one client, Flyway, requires support for this syntax. Graphile uses this too.

Epic CRDB-7217

@jordanlewis jordanlewis added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Apr 17, 2017
@jordanlewis jordanlewis changed the title sql: support SET ROLE sql: support SET ROLE Apr 17, 2017
@bdarnell
Copy link
Contributor

It looks like flyway only requires RESET ROLE, which is a trivial noop if we don't support SET ROLE.

@dianasaur323 dianasaur323 added this to the 1.1 milestone Apr 18, 2017
@cuongdo cuongdo modified the milestones: Later, 1.1 Aug 22, 2017
@dianasaur323 dianasaur323 modified the milestones: Later, 1.2 Sep 17, 2017
@dianasaur323
Copy link
Contributor

Looks like this is RBAC, which we will work on in 1.2. cc @nstewart

@nstewart
Copy link
Contributor

RBAC is scheduled to be an enterprise feature, so we should definitely try to decouple that effort from the apparently small amount of work required to unblock FlywayDB, which should work for all users. I'll note this consideration in our RBAC record. cc @awoods187

@jordanlewis
Copy link
Member Author

FWIW this isn't a blocker for FlywayDB anymore, since their team created an adapter that works around our limitations.

@cfsnyder
Copy link

@jordanlewis do you have a link for information about the Flyway adapter that you mentioned?

@jordanlewis
Copy link
Member Author

@cfsnyder it'll be part of their next major release. Adapter is probably the wrong word - it'll be part of the main Flyway product.

@vivekmenezes vivekmenezes assigned mberhault and unassigned mberhault Oct 24, 2017
@mberhault
Copy link
Contributor

SET ROLE is not part of the initial RBAC implementation (users cannot specify that they want to run as a specific role. Not entirely needed as privileges are currently always inherited. SET ROLE may be needed once NOINHERIT is possible).

Since this is no longer needed for Flyway, I propose we close it.

@awoods187 awoods187 modified the milestones: 1.2, Later Dec 4, 2017
@awoods187
Copy link
Contributor

Moving to later in case we need it, but agree with Marc that its not needed at the moment cc @vivekmenezes

@knz knz added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label May 9, 2018
@mberhault
Copy link
Contributor

Unassigning. This need to be prioritized. If desired, this would be an easy project for someone on the sql team.

@aodhan-domhnaill
Copy link

Can I get guidance on how to add support for this feature?

@knz knz changed the title sql: support SET ROLE sql: enable changing the current user of a session with SET ROLE May 27, 2021
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) A-security labels May 27, 2021
@rafiss rafiss added the A-tools-graphile Issues relating to graphile compatibility label Jul 21, 2021
@otan
Copy link
Contributor

otan commented Aug 12, 2021

Unfortunately, due to the way we parse SESSION_USER:

cockroach/pkg/sql/parser/sql.y

Lines 11528 to 11531 in 3ea1633

| SESSION_USER
{
$$.val = &tree.FuncExpr{Func: tree.WrapFunction("current_user")}
}
, we may need two releases to push this out and work as expected. Since SESSION_USER and CURRENT_USER is different with the introduction of this PR, changing the builtin it parses to would not work in a mixed version state as any new builtin we introduce needs on release to make it in.

We can also just ship it and say SESSION_USER returns the wrong thing (for one release), use SESSION_USER() for the correct result.

@otan
Copy link
Contributor

otan commented Aug 16, 2021

#68973 has this working, but with the above limitation.

craig bot pushed a commit that referenced this issue Aug 18, 2021
68608: go.mod: update to pgx v4.13 r=otan,knz,stevendanna a=rafiss

fixes #62840

This also required an update to cockroach-go/v2 to get the crdbpgx
transaction retry helper.

The majority of changes here are because pgx now requires the context to
be passed in always.

Other changes:
* The way that pgx prepares statements has changed -- to disable
automatic prepared statements, the PreferSimpleProtocol setting must be
enabled when making the pgxpool.
* The `noprepare` setting was removed from `workload`. This functionality
is not supported by pgx any more. pgx will either use prepared statements and
cache them automatically or use the simple protocol.
* sqlproxy tests had to be modified since pgx now has more
fallback logic to retry connections when using an sslmode of `prefer`
or `allow`. pgx also now tries to resolve `localhost` as the IPv6 address
`:::1`, which isn't well-supported by sqlproxy, so IP resolution was disabled
for sqlproxy tests.
* CHANGEFEED tests had to be changed since `pgx.Conn.Query` now immediately
tries fetching the results, which didn't work with the synchronization that was added
to TestChangefeedDataTTL.
* COPY was fixed to correctly ignore CopyData after encountering an error.

Release note: None

68715: sql: introduce crdb_internal.statement_statistics virtual table r=maryliag,ajwerner a=Azhng

Depends on  #68675
Related to: #64743

This commit introduces crdb_internal.statement_statistics virtual
table that exposes both cluster-wide in-memory statement statistics
as well as persited statement statistics. This new virtual table
will be used to replace crdb_internal.node_statement_statistics
virtual table, which only surface node-local in-memory statement
statsitics.

Release note (sql change): introduced new crdb_internal.statement_statistics
virtual table that surfaces both cluster-wide in-memory statement statistics
as well as persited statement statistics.

68749: builtins: implement a separate session_user() builtin r=richardjcai a=otan

This is needed groundwork to separate current_user and session_user.

Refs: #15005

Release note (sql change): Add a session_user() builtin function, which
currently returns the same thing as current_user() as we do not
implement `SET ROLE`.

68750: parser: SET ROLE syntax preparation r=richardjcai a=otan

See commits for details.

Refs: #15005

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Azhng <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
@craig craig bot closed this as completed in 46cef2c Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-graphile Issues relating to graphile compatibility C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.