feat: create user if not exists on JWT authenticate #4924

junnplus · 2022-04-18T15:42:11Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Create user if not exists on JWT authenticate.

Specify the create_user struct in custom claims to create a user.

#[derive(Default, Deserialize, Serialize)]
pub struct CreateUser {
    pub tenant_id: Option<String>,
    pub roles: Vec<String>,
}

#[derive(Default, Deserialize, Serialize)]
pub struct CustomClaims {
    pub create_user: Option<CreateUser>,
}

Changelog

New Feature

Related Issues

Fixes #4875

vercel · 2022-04-18T15:42:16Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
databend	⬜️ Ignored (Inspect)		Apr 19, 2022 at 7:25AM (UTC)

mergify · 2022-04-18T15:42:44Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

youngsofun · 2022-04-19T00:48:20Z

query/src/users/auth/auth_mgr.rs

                    None => return Err(ErrorCode::AuthenticateFailure("jwt auth not configured.")),
                };
-                self.users
+                let user_name = jwt.subject.unwrap();
+                if let Some(create_user) = jwt.custom.create_user {


add_user return error if user exists. does the cloud platform when it should carry the create_user field?

for an existed user, what if the existed user info diff with info in create_user?

add_user return error if user exists. does the cloud platform when it should carry the create_user field?

User will be created only if user does not exist. is_not_exists is true for add_user .

for an existed user, what if the existed user info diff with info in create_user?

create_user just used to create users, not update.

User will be created only if user does not exist

do you mean cloud platform do this check to determine whether or not to addcreate_user in jwt? cc @flaneur2020

add_user may ErrorCode::UserAlreadyExists

do you mean cloud platform do this check to determine whether or not to addcreate_user in jwt?

i guess the cloud platform could ensure user exists by an jwt with create_user enabled when the user successfully logged in, and then pass normal jwt for the normal operations to interact with databend-query.

a jwt is used to authenticate an user, if the jwt verified successfully and the user already existed, it can pass the authentication happily, it might a bit weird to get a userAlreadyExists in this case. 🤔

btw, how about making the create_user claim into a bool?

the infomation about tenant and role may already existed some another claims 🤔

btw, how about making the create_user claim into a bool?

like this?

{ "create_user": true, "tenant_id": "", "roles": [] }

I'm not sure if the same fields would be in custom claims, it's better together.

@junnplus sorry, what I saw is UserApi::add_user.

i guess the cloud platform could ensure user exists by an jwt with create_user enabled when the user successfully logged in, and then pass normal jwt for the normal operations to interact with databend-query.

maybe we can call it ensure_user? I saw it a lot in puppet. or "create_user_if_not_exist".

btw, how about making the create_user claim into a bool?

seems currently this create_user provides only a basic template for a newly-seen user, so the value in it take effective only when user dose not exist. and it will not override behavior specified by an existing userInfo.

So, I prefer to separate it from other fields，which while be regard，naturally， being able to override the behavior of the session

@flaneur2020

ensure_user SGTM. cc @flaneur2020

cool, ensure_user is much better than create_user_if_not_exist.

I guess jwt in some manner favors shorter names, as it'd like to avoids getting too big in the transport, likewise "aud" over "audience", "sub" over "subject" etc.

query/src/users/auth/auth_mgr.rs

youngsofun · 2022-04-19T00:55:54Z

query/src/users/auth/auth_mgr.rs

+                    }
+                    self.user_mgr.add_user(&tenant, user_info, true).await?;
+                }
+                self.user_mgr


if add_user does return ok, no need to get_user here?

if add_user does return ok, no need to get_user here?

If the user already exists, we still need to get the user.

why not return the UserInfo just created?

why not return the UserInfo just created?

User will not be created if user already exists, so we need to get_user.

so, put it together, Jwt only create a user if not exist.
the user may be altered later as normal.

lgtm

query/src/users/auth/jwt/authenticator.rs

Signed-off-by: Ye Sijun <junnplus@gmail.com>

junnplus requested a review from BohuTANG as a code owner April 18, 2022 15:42

databend-bot added the need-review label Apr 18, 2022

mergify bot added the pr-feature this PR introduces a new feature to the codebase label Apr 18, 2022

junnplus force-pushed the jwt-user branch from 4096d35 to 27cdfb5 Compare April 18, 2022 15:47

junnplus requested review from youngsofun, flaneur2020 and ZhiHanZ April 18, 2022 15:49

junnplus force-pushed the jwt-user branch from 27cdfb5 to 214c99d Compare April 18, 2022 15:55

youngsofun reviewed Apr 19, 2022

View reviewed changes

query/src/users/auth/auth_mgr.rs Show resolved Hide resolved

youngsofun reviewed Apr 19, 2022

View reviewed changes

flaneur2020 reviewed Apr 19, 2022

View reviewed changes

query/src/users/auth/jwt/authenticator.rs Outdated Show resolved Hide resolved

junnplus added 2 commits April 19, 2022 11:31

create user on JWT authenticate

f54511b

refactor new user/role info

159874a

junnplus force-pushed the jwt-user branch from 214c99d to 21cc65f Compare April 19, 2022 03:31

add jwt test for auth_mgr

86674a3

Signed-off-by: Ye Sijun <junnplus@gmail.com>

junnplus force-pushed the jwt-user branch from 21cc65f to 86674a3 Compare April 19, 2022 07:25

youngsofun approved these changes Apr 19, 2022

View reviewed changes

BohuTANG merged commit d411978 into databendlabs:main Apr 19, 2022

junnplus mentioned this pull request Apr 19, 2022

Rename CreateUser to EnsureUser on JWT CustomClaims #4935

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create user if not exists on JWT authenticate #4924

feat: create user if not exists on JWT authenticate #4924

junnplus commented Apr 18, 2022 •

edited

Loading

vercel bot commented Apr 18, 2022 •

edited

Loading

mergify bot commented Apr 18, 2022

youngsofun Apr 19, 2022

junnplus Apr 19, 2022

youngsofun Apr 19, 2022

flaneur2020 Apr 19, 2022

flaneur2020 Apr 19, 2022 •

edited

Loading

junnplus Apr 19, 2022

youngsofun Apr 19, 2022

youngsofun Apr 19, 2022 •

edited

Loading

junnplus Apr 19, 2022 •

edited

Loading

flaneur2020 Apr 19, 2022

youngsofun Apr 19, 2022

junnplus Apr 19, 2022 •

edited

Loading

youngsofun Apr 19, 2022

junnplus Apr 19, 2022

youngsofun Apr 19, 2022

feat: create user if not exists on JWT authenticate #4924

feat: create user if not exists on JWT authenticate #4924

Conversation

junnplus commented Apr 18, 2022 • edited Loading

Summary

Changelog

Related Issues

vercel bot commented Apr 18, 2022 • edited Loading

mergify bot commented Apr 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

flaneur2020 Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngsofun Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

junnplus Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junnplus Apr 19, 2022 • edited Loading

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

junnplus commented Apr 18, 2022 •

edited

Loading

vercel bot commented Apr 18, 2022 •

edited

Loading

flaneur2020 Apr 19, 2022 •

edited

Loading

youngsofun Apr 19, 2022 •

edited

Loading

junnplus Apr 19, 2022 •

edited

Loading

junnplus Apr 19, 2022 •

edited

Loading