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

feat: support user authentication in frontend #3074

Merged
merged 8 commits into from Jun 9, 2022

Conversation

yezizp2012
Copy link
Contributor

@yezizp2012 yezizp2012 commented Jun 8, 2022

What's changed and what's your intention?

As title, until now we can create a new user to login, the frontend will do the authentication check if password provided. Currently we using risingwave root as default super user and all e2e test is running under risingwave root.
image

Note that:
psql use -U, --username=USERNAME as user option, sqllogictest uses -u/--user.

Current user is already recorded in each session, this is helpful for #2814. Cc @cnissnzg

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

@yezizp2012 yezizp2012 added the user-facing-changes Contains changes that are visible to users label Jun 8, 2022
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #3074 (d535d56) into main (f90b137) will decrease coverage by 0.06%.
The diff coverage is 56.30%.

@@            Coverage Diff             @@
##             main    #3074      +/-   ##
==========================================
- Coverage   73.47%   73.41%   -0.07%     
==========================================
  Files         734      734              
  Lines      100209   100346     +137     
==========================================
+ Hits        73631    73666      +35     
- Misses      26578    26680     +102     
Flag Coverage Δ
rust 73.41% <56.30%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/common/src/catalog/mod.rs 58.59% <ø> (ø)
src/meta/src/manager/user.rs 87.82% <ø> (-0.13%) ⬇️
src/risedevtool/src/bin/risedev-compose.rs 0.43% <0.00%> (-0.01%) ⬇️
src/utils/pgwire/src/pg_message.rs 74.63% <0.00%> (-3.90%) ⬇️
src/frontend/src/session.rs 45.68% <17.74%> (-5.82%) ⬇️
src/frontend/src/test_utils.rs 92.30% <28.57%> (-1.36%) ⬇️
src/utils/pgwire/src/pg_server.rs 88.23% <33.33%> (-3.77%) ⬇️
src/utils/pgwire/src/pg_protocol.rs 76.23% <56.09%> (-4.36%) ⬇️
src/frontend/src/handler/create_user.rs 87.30% <100.00%> (ø)
src/frontend/src/user/user_authentication.rs 100.00% <100.00%> (ø)
... and 1 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@yezizp2012 yezizp2012 marked this pull request as ready for review June 8, 2022 10:21
Copy link
Contributor

@BowenXiao1999 BowenXiao1999 left a comment

Choose a reason for hiding this comment

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

  1. Maybe we should unify the -u and -U later ..
  2. So for now, we can log in as risingwave without password. But we need password for any other users?
  3. Do we set different permissions for different users? For example, superuser and other default user.

Rest LGTM

@yezizp2012
Copy link
Contributor Author

yezizp2012 commented Jun 8, 2022

  1. Maybe we should unify the -u and -U later ..

  2. So for now, we can log in as risingwave without password. But we need password for any other users?

  3. Do we set different permissions for different users? For example, superuser and other default user.

Rest LGTM

NTFS for 1. root is the default initialized super user in our cluster. We can create any user with different options like can_login/superuser etc, and also with password or not. If provided, the new user will be required to enter password when log in. You can find more details in #2643. The permission check for privilege is not implemented yet.

Copy link
Contributor

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

LGTM. Please reconsider the name of root user :lark-cry:

src/frontend/src/session.rs Outdated Show resolved Hide resolved
src/frontend/src/session.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@hengm3467
Copy link
Contributor

@yezizp2012 Is it accurate to say that RisingWave now supports "CREATE USER"? Is the syntax same with PG's? Thanks!

@yezizp2012
Copy link
Contributor Author

@yezizp2012 Is it accurate to say that RisingWave now supports "CREATE USER"? Is the syntax same with PG's? Thanks!

Yes, RisingWave supports "CREATE/DROP USER" now. The syntax is quit the same with PG, but only support some options as follows (some options in PG are not supported currently):

CREATE USER name [ [ WITH ] option [ ... ] ]

where option can be:

      SUPERUSER | NOSUPERUSER
    | CREATEDB | NOCREATEDB
    | LOGIN | NOLOGIN
    | [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL

DROP USER [ IF EXISTS ] name [, ...]

ALTER USER is gonna be supported in the future.

@CharlieSYH
Copy link
Contributor

CharlieSYH commented Aug 3, 2022

@yezizp2012
I tested this statement and it retuned an error "ERROR: sql parser error: Expected end of statement, found: PASSWORD":
CREATE USER u1 PASSWORD 's1234';
But this works: CREATE USER u1 WITH PASSWORD 's1234';

Seems that 'with' is not optional. Can you confirm on that?

@yezizp2012
Copy link
Contributor Author

yezizp2012 commented Aug 3, 2022

@yezizp2012

I tested this statement and it retuned an error "ERROR: sql parser error: Expected end of statement, found: PASSWORD":

CREATE USER u1 PASSWORD 's1234';

But this works: CREATE USER u1 WITH PASSWORD 's1234';

Seems that 'with' is not optional. Can you confirm on that?

Yes, currently 'with' is required in risingwave. I will check the behavior in PostgreSQL and fix this if it's optional.

@yezizp2012
Copy link
Contributor Author

Will be fix in #4414 . @CharlieSYH

@CharlieSYH
Copy link
Contributor

Can you point out which one in each option pair is the default if not specified? @yezizp2012

  SUPERUSER | NOSUPERUSER
  CREATEDB | NOCREATEDB
  LOGIN | NOLOGIN
  [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL

@yezizp2012
Copy link
Contributor Author

Can you point out which one in each option pair is the default if not specified? @yezizp2012

  SUPERUSER | NOSUPERUSER
  CREATEDB | NOCREATEDB
  LOGIN | NOLOGIN
  [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL

The default options for all are: NOSUPERUSER, NOCREATEDB, LOGIN, PASSWORD NULL.

@CharlieSYH
Copy link
Contributor

Can you point out which one in each option pair is the default if not specified? @yezizp2012

  SUPERUSER | NOSUPERUSER
  CREATEDB | NOCREATEDB
  LOGIN | NOLOGIN
  [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL

The default options for all are: NOSUPERUSER, NOCREATEDB, LOGIN, PASSWORD NULL.

That's awkward. Then I can create a database with a user role that is not supposed to have CREATEDB previledge:

dev=> create user u8;
CREATE_USER
dev=> \q
shenyuhan@shenyuhans-MacBook-Pro risingwave % psql -h localhost -p 4566 -d dev -U u8;
psql (14.4, server 9.5.0)
Type "help" for help.

dev=> create database db1;
CREATE_DATABASE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants