-
Notifications
You must be signed in to change notification settings - Fork 7
Providing user authentication #21
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
Conversation
Added an overload to the Connector::connect() method, which takes a config structure containing address, port, username and password. If the name and password are specified, the user is authorized. Added new structure config. Added tests to check and as an example of how to use the function.
| INCLUDE_DIRECTORIES(./src) | ||
| INCLUDE_DIRECTORIES(./third_party/libev) | ||
| ADD_LIBRARY(ev STATIC third_party/libev/ev.c) | ||
| ADD_LIBRARY(scrmbl STATIC third_party/scramble/scramble.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you set scramble.c as a separate lib?
| INCLUDE_DIRECTORIES(./third_party/libev) | ||
| ADD_LIBRARY(ev STATIC third_party/libev/ev.c) | ||
| ADD_LIBRARY(scrmbl STATIC third_party/scramble/scramble.c) | ||
| ADD_LIBRARY(sha STATIC third_party/sha1/sha1.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question..
| rid_t ping(); | ||
|
|
||
| /** | ||
| * Authorizate user with user_name and password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not an authorization, but rather preparation of authorization request.
| LOG_DEBUG("Connected to ", config.addr, ':', config.port, " has been established"); | ||
| if (config.user_name != std::string() && config.password != std::string()) { | ||
| rid_t auth_f = conn.auth(config.user_name, config.password); | ||
| wait(conn, auth_f, 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use magic constants.
You do forget to check wait return code.
| #include <map> | ||
|
|
||
| #include "IprotoConstants.hpp" | ||
| #include "ResponseReader.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this header in encoder? Seems to be messy
| const std::string addr; | ||
| unsigned port; | ||
| const std::string user_name = std::string(); | ||
| const std::string password = std::string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have to call default constructor explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better make Config to be class with proper constructors
| } | ||
| for (auto const& t : tuples) { | ||
| std::cout << t << std::endl; | ||
| if (response.body.data != std::nullopt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does assertion turn into if?
| authentication_test(Connector<BUFFER, NetProvider> &client) | ||
| { | ||
| TEST_INIT(0); | ||
| TEST_CASE("Authentication within user_name and password"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case name is confusing
| #include "../sha1/sha1.h" | ||
| #include <string.h> | ||
| #include <stdio.h> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better export any extra libs in a separate commit
| client.close(conn); | ||
| } | ||
|
|
||
| TEST_CASE("Authentication with some privileges without errors"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get assertion fault on this test:
Client.test: tntcxx/test/ClientTest.cpp:609: void authentication_test(Connector<BUFFER, NetProvider>&) [with BUFFER = tnt::Buffer<16384>; NetProvider = DefaultNetProvider<tnt::Buffer<16384>, NetworkEngine>]: Assertion `false' failed.
| TEST_CASE("Authentication with user_name and password good"); | ||
| { | ||
| Connection<Buf_t, NetProvider> conn(client); | ||
| Config config = {localhost, port, "Anastas", "123456"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use pre-defined constants:
const char *user = "..."
| std::tuple data = std::make_tuple(666, "111", 1.01); | ||
| rid_t f1 = conn.space[space_id].replace(data); | ||
| data = std::make_tuple(777, "asd", 2.02); | ||
| rid_t f2 = conn.space[space_id].replace(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authentication requires a bit more test cases. You can look at examples from Tarantool repo
Connector::connect()method, which takesa
configstructure containing address, port, username and password.If the name and password are specified, the user is authorized.