User manager for graph layer #1842

bright-starry-sky · 2020-03-01T08:00:12Z

User manager for graph layer.

src/graph/ShowExecutor.cpp

src/parser/parser.yy

src/parser/UserSentences.h

src/graph/UserExecutor.cpp

src/graph/ShowExecutor.cpp

src/graph/UserExecutor.cpp

ansarizafar · 2020-03-02T11:18:48Z

Its better to use term Super Admin/User than GOD.

src/meta/MetaServiceUtils.cpp

src/interface/meta.thrift

liuyu85cn

LGTM

src/graph/UserExecutor.cpp

panda-sheep · 2020-03-06T07:39:00Z

Great job, LGTM
Please resolve the conflict first

panda-sheep

Great job， 👍

dangleptr · 2020-03-09T04:02:43Z

src/parser/scanner.lex

@@ -157,6 +153,15 @@ SNAPSHOT                    ([Ss][Nn][Aa][Pp][Ss][Hh][Oo][Tt])
 SNAPSHOTS                   ([Ss][Nn][Aa][Pp][Ss][Hh][Oo][Tt][Ss])
 FORCE                       ([Ff][Oo][Rr][Cc][Ee])
 BIDIRECT                    ([Bb][Ii][Dd][Ii][Rr][Ee][Cc][Tt])
+MAX_QUERIES_PER_HOUR        ([Mm][Aa][Xx][_][Qq][Uu][Ee][Rr][Ii][Ee][Ss][_][Pp][Ee][Rr][_][Hh][Oo][Uu][Rr])


Actually, we don't need them anymore.

dangleptr · 2020-03-09T04:02:53Z

src/parser/test/ScannerTest.cpp

@@ -429,6 +421,26 @@ TEST(Scanner, Basic) {
        CHECK_SEMANTIC_TYPE("SNAPSHOTS", TokenType::KW_SNAPSHOTS),
        CHECK_SEMANTIC_TYPE("Snapshots", TokenType::KW_SNAPSHOTS),
        CHECK_SEMANTIC_TYPE("snapshots", TokenType::KW_SNAPSHOTS),
+        CHECK_SEMANTIC_TYPE("MAX_QUERIES_PER_HOUR", TokenType::KW_MAX_QUERIES_PER_HOUR),


dangleptr · 2020-03-09T04:04:22Z

src/interface/common.thrift

+    // encoded password
+    4: optional string        encoded_pwd,
+    // The number of queries an account can issue per hour
+    5: optional i32           max_queries_per_hour,


We don't need them any more before we don't implement user's isolation.

We don't need them any more before we don't implement user's isolation.

Good suggest !
Yes , They are useless here if we don't implement user's isolation.
I also hesitated to add these options at that time. Now, let me delete them.

dangleptr · 2020-03-09T05:00:48Z

src/interface/common.thrift

+}
+
+struct UserItem {
+    1: required string        account;


Do we have id for user?

Do we have id for user?

I've removed user id . because we can using user name to keep the uniqueness for users.

dangleptr · 2020-03-09T05:00:56Z

src/interface/common.thrift

+
+struct RoleItem {
+    1: string        user,
+    2: string        space,


Use spaceId

ditto

fixed.

dangleptr · 2020-03-09T05:08:25Z

src/meta/processors/usersMan/AuthenticationProcessor.cpp

+        return;
+    }
+    if (roleItem.get_role_type() == nebula::cpp2::RoleType::GOD) {
+        spaceId = kDefaultSpaceId;


Do we support to grant a user to be GOD?

Do we support to grant a user to be GOD?

Only supported to grant a user to GOD in initialization phase.
We need to initialize a root user before the authority flag turn on.

src/interface/common.thrift

dangleptr · 2020-03-09T05:16:19Z

src/meta/processors/usersMan/AuthenticationProcessor.cpp

    if (!ret.ok()) {
-        if (req.get_missing_ok()) {
+        if (req.get_if_exists()) {
            handleErrorCode(cpp2::ErrorCode::SUCCEEDED);


Please recheck "handleErrorCode", IIRC "onFinished" has been called inside?

Please recheck "handleErrorCode", IIRC "onFinished" has been called inside?

emm... seems It's works well now, "onFinished" have not call "handleErrorCode" inside.

dangleptr · 2020-03-09T05:18:40Z

src/graph/UserExecutor.cpp

+    return Status::OK();
+}
+
+void ChangePasswordExecutor::execute() {


Do we need "change password" Or just "resetPassword" ?

dangleptr · 2020-03-09T05:21:46Z

src/meta/MetaServiceUtils.cpp

+std::string MetaServiceUtils::replaceUserVal(const nebula::cpp2::UserItem& user,
+                                             folly::StringPiece val) {
+    nebula::cpp2::UserItem oldUser;
+    apache::thrift::CompactSerializer::deserialize(val, oldUser);


Why not serialize the newUser and overwrite the old one directly

bright-starry-sky · 2020-03-10T11:52:56Z

Refactored root user initialize logic. Now we have only one super user naming "root".
metad will check the root user when meta service start. and then create the user if dose not exists .

src/meta/RootUserMan.h

dangleptr · 2020-03-11T02:52:38Z

src/meta/processors/partsMan/DropSpaceProcessor.cpp

@@ -43,7 +43,19 @@ void DropSpaceProcessor::process(const cpp2::DropSpaceReq& req) {
    deleteKeys.emplace_back(MetaServiceUtils::spaceKey(spaceId));

    // delete related role data.
-    // TODO(boshengchen) delete related role data under the space
+    auto rolePrefix = MetaServiceUtils::roleSpacePrefix(spaceId);


Add some LOGs when deleting the users information.

Add some LOGs when deleting the users information.

fixed.

dangleptr · 2020-03-11T03:18:06Z

src/interface/meta.thrift

@@ -145,26 +129,6 @@ struct HostItem {
    4: map<string, list<common.PartitionID>> (cpp.template = "std::unordered_map") all_parts,
 }

-struct UserItem {


We don't need it any more?

I don't think we need it any more, currently we only have one item of password, we can storage it direct.
If there are new elements in the future, we can add this struct at that time.

dangleptr

Totally LGTM

dangleptr · 2020-03-11T06:39:40Z

src/daemons/MetaDaemon.cpp

+        /**
+         *  Only leader part needed.
+         */
+        auto ret = kvstore->partLeader(nebula::meta::kDefaultSpaceId,


Please check the logic on real cluster

Please check the logic on real cluster

Yes, I've simple tested two meta nodes on virtual machines.
I will do complete integration test at later.

panda-sheep

Excellent！

Amber1990Zhang · 2020-03-13T03:50:46Z

src/meta/processors/usersMan/AuthenticationProcessor.cpp

-            LOG(ERROR) << "Create User Failed :" << user.get_account() << " have existed";
-            handleErrorCode(cpp2::ErrorCode::E_EXISTED);
+            LOG(ERROR) << "Create User Failed : User " << account
+                       << " have existed!";


already existed.

* push filter down traverse rule * fix conflicts * fix tck * fix exists func Co-authored-by: jie.wang <38901892+jievince@users.noreply.github.com>

bright-starry-sky added the ready-for-testing PR: ready for the CI test label Mar 1, 2020

bright-starry-sky requested review from monadbobo, dutor, yixinglu, darionyaphet, sherman-the-tank, CPWstatic, critical27, dangleptr, laura-ding, liuyu85cn and panda-sheep March 1, 2020 08:00

bright-starry-sky added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Mar 1, 2020

panda-sheep reviewed Mar 2, 2020

View reviewed changes

critical27 reviewed Mar 2, 2020

View reviewed changes

src/graph/ShowExecutor.cpp Outdated Show resolved Hide resolved

src/graph/UserExecutor.cpp Show resolved Hide resolved

jude-zhu added this to the R201910_RC4 milestone Mar 3, 2020

bright-starry-sky mentioned this pull request Mar 5, 2020

User permission manager #1873

Merged

liuyu85cn reviewed Mar 6, 2020

View reviewed changes

src/meta/MetaServiceUtils.cpp Outdated Show resolved Hide resolved

liuyu85cn reviewed Mar 6, 2020

View reviewed changes

src/interface/meta.thrift Show resolved Hide resolved

liuyu85cn previously approved these changes Mar 6, 2020

View reviewed changes

panda-sheep reviewed Mar 6, 2020

View reviewed changes

src/graph/UserExecutor.cpp Outdated Show resolved Hide resolved

src/graph/UserExecutor.cpp Outdated Show resolved Hide resolved

bright-starry-sky dismissed liuyu85cn’s stale review via db571b9 March 6, 2020 05:31

panda-sheep previously approved these changes Mar 6, 2020

View reviewed changes

dangleptr reviewed Mar 9, 2020

View reviewed changes

jude-zhu requested review from dangleptr, critical27 and liuyu85cn March 9, 2020 07:44

bright-starry-sky dismissed panda-sheep’s stale review via f4d9ea5 March 9, 2020 17:01

bright-starry-sky added 4 commits March 10, 2020 20:44

user graph

97a877f

Addressed comments

e90760d

Remove DCHECK

2812c27

Addressed comments

0ea90ac

dangleptr reviewed Mar 11, 2020

View reviewed changes

bright-starry-sky and others added 2 commits March 11, 2020 14:17

1,Fixed error of root init on leader part, 2, Addressed comments

cf00d49

Merge branch 'master' into user_graph

f5ef911

dangleptr approved these changes Mar 11, 2020

View reviewed changes

panda-sheep approved these changes Mar 11, 2020

View reviewed changes

dangleptr merged commit 70f14b2 into vesoft-inc:master Mar 11, 2020

bright-starry-sky deleted the user_graph branch March 11, 2020 07:13

Amber1990Zhang reviewed Mar 13, 2020

View reviewed changes

Amber1990Zhang mentioned this pull request Mar 13, 2020

[doc]User management #1917

Merged

tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021

User manager for graph layer (vesoft-inc#1842)

321aea9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User manager for graph layer #1842

User manager for graph layer #1842

bright-starry-sky commented Mar 1, 2020

ansarizafar commented Mar 2, 2020

liuyu85cn left a comment

panda-sheep commented Mar 6, 2020

panda-sheep left a comment

dangleptr Mar 9, 2020

dangleptr Mar 9, 2020

dangleptr Mar 9, 2020

bright-starry-sky Mar 9, 2020

dangleptr Mar 9, 2020

bright-starry-sky Mar 9, 2020

dangleptr Mar 9, 2020

dangleptr Mar 11, 2020

bright-starry-sky Mar 11, 2020

dangleptr Mar 9, 2020

bright-starry-sky Mar 9, 2020

dangleptr Mar 9, 2020

bright-starry-sky Mar 9, 2020

dangleptr Mar 9, 2020

dangleptr Mar 9, 2020

bright-starry-sky commented Mar 10, 2020

dangleptr Mar 11, 2020

bright-starry-sky Mar 11, 2020

dangleptr Mar 11, 2020

bright-starry-sky Mar 11, 2020

dangleptr left a comment

dangleptr Mar 11, 2020

bright-starry-sky Mar 11, 2020

panda-sheep left a comment

Amber1990Zhang Mar 13, 2020

User manager for graph layer #1842

User manager for graph layer #1842

Conversation

bright-starry-sky commented Mar 1, 2020

ansarizafar commented Mar 2, 2020

liuyu85cn left a comment

Choose a reason for hiding this comment

panda-sheep commented Mar 6, 2020

panda-sheep left a comment

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

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

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

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

bright-starry-sky commented Mar 10, 2020

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

dangleptr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panda-sheep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment