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: UserHasAdminRole should return true for the admin role itself #71219

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

tukeJonny
Copy link
Contributor

@tukeJonny tukeJonny commented Oct 6, 2021

This PR makes planner.UserHasAdminRole returns true on the admin role for improving introspection.

This PR had checked by running make testbaselogic .

Fixes #70779

Release note: None

@blathers-crl
Copy link

blathers-crl bot commented Oct 6, 2021

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

  • A person you worked with closely on this PR.
  • The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
  • Join our community slack channel and ask on #contributors.
  • Try find someone else from here.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Oct 6, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tukeJonny tukeJonny force-pushed the sql-admin-user-has-admin-role branch from c8ed0dc to e21d3a9 Compare October 6, 2021 16:35
@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Oct 6, 2021

CLA assistant check
All committers have signed the CLA.

@blathers-crl
Copy link

blathers-crl bot commented Oct 6, 2021

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the X-blathers-triaged blathers was able to find an owner label Oct 6, 2021
@blathers-crl blathers-crl bot requested a review from rafiss October 6, 2021 16:35
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i thought we already had tests in place for this, but we don;t

could you add admin to the test case here:

WHERE usename IN ('super_user', 'regular_user', 'root')

@tukeJonny tukeJonny force-pushed the sql-admin-user-has-admin-role branch from e21d3a9 to db7cb62 Compare October 7, 2021 01:29
@blathers-crl
Copy link

blathers-crl bot commented Oct 7, 2021

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@tukeJonny tukeJonny force-pushed the sql-admin-user-has-admin-role branch 2 times, most recently from 680ed02 to ef27e19 Compare October 7, 2021 02:25
@tukeJonny
Copy link
Contributor Author

tukeJonny commented Oct 7, 2021

@rafiss

I've added admin for testcase of pg_catalog.pg_user .
But, there were no records for admin in pg_catalog.pg_user , then tests had failed.

`make tesebaselogic FILES='pg_catalog'`
SELECT usename, usesuper
FROM pg_user
WHERE usename IN ('super_user', 'regular_user', 'root', 'admin')
ORDER BY usename;
        -- FAIL
    logic.go:2323: 
         
        testdata/logic_test/pg_catalog:5357: SELECT usename, usesuper
        FROM pg_user
        WHERE usename IN ('super_user', 'regular_user', 'root', 'admin')
        ORDER BY usename
        expected:
            usename       usesuper
            admin         true 
            regular_user  false
            root          true
            super_user    true
            
        but found (query options: "colnames") :
            usename       usesuper
            regular_user  false
            root          true
            super_user    true
        -- FAIL
    logic.go:1867: 
         testdata/logic_test/pg_catalog:5369: too many errors encountered, skipping the rest of the input
--- done: testdata/logic_test/pg_catalog with config local: 280 tests, 2 failures

From what i've been able to find out with make testbaselogic FILES='pg_catalog' , though admin role/user are exists, but can't see from pg_catalog.pg_user .

As following link, vtable iterates roles, and output simply. Therefore, I still can't find out cause of this problem.

https://github.com/cockroachdb/cockroach/blob/6ea317ddf277c6005589326d7ee68ea5003ceba2/pkg/sql/pg_catalog.go#L3006,L3036

Though i will do some more research on my end, please give me some advice if you have any ideas.

Thanks!

@tukeJonny tukeJonny force-pushed the sql-admin-user-has-admin-role branch from ef27e19 to 24be1a7 Compare October 7, 2021 07:16
@tukeJonny
Copy link
Contributor Author

tukeJonny commented Oct 7, 2021

When populating vtable.PGCatalogUser , isRole=true about admin user.
So, following if-statement passes and returns nil.

if isRole {

From 20171220_sql_role_based_access_control.md

`isRole` denotes whether the corresponding name is a user or a role. Roles cannot log in or have passwords.

This commit makes `UserHasAdminRole` returns true on the `admin` role
for improving introspection.

Release note: None
@tukeJonny tukeJonny force-pushed the sql-admin-user-has-admin-role branch from 24be1a7 to a530f6f Compare October 7, 2021 12:32
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, thanks for that investigation! in that case, this PR looks good! (there are other logictests that look up "admin" in other pg_catalog tables, and those already are correct)

bors r+

@tukeJonny
Copy link
Contributor Author

Thanks for the great opportunity!

This Issue was just right and a good opportunity to look around and learn more!

@craig
Copy link
Contributor

craig bot commented Oct 7, 2021

Build succeeded:

@craig craig bot merged commit 5b1bd56 into cockroachdb:master Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community X-blathers-triaged blathers was able to find an owner X-blathers-untriaged blathers was unable to find an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: UserHasAdminRole should return true for the admin role itself
3 participants