#1488 closed enhancement (fixed)
new auth module: sqlsqlite
Reported by: | Denis01 | Owned by: | Denis01 |
---|---|---|---|
Priority: | major | Milestone: | 2.1 |
Component: | server | Version: | trunk |
Keywords: | Cc: |
Description
Create possibility to keep the users records (like multifile records structure) in mySQL table
Attachments (8)
Change History (53)
comment:1 Changed 4 years ago by
Component: | android → server |
---|---|
Milestone: | → 2.2 |
Status: | new → assigned |
comment:2 Changed 4 years ago by
It would make sense to eventually move the common SQL code to a utility superclass, so this auth module should be called "sqliteauth".
Then we can also have a "mysqlauth" and "postgresqlauth", etc..
All the connectors should follow the PEP 249 : Python Database API Specification v2.0
The "sqliteauth" module can take a single specific parameter: the path to the database file, whereas the other SQL auth modules will require extra parameters: host, port, username, password, database name, etc
All the SQL auth modules should use the "password_query" and "sessions_query" (this one can be made optional) as per the stub patch, moved to the superclass.
comment:3 Changed 4 years ago by
Hello,
1)In order to make the proper call and init of module i tried to get familiar with the content of "program_context"
from xpra.platform import program_context
But didn't find the code source. Could you please help to find it.
2) it seems that Sqlite doesn't support sql schemes. So 2 ways are possible:
- create a DB in ":memory:"
- create a DB as a second db file
Which one to choose?
Thank you.
comment:4 Changed 4 years ago by
Owner: | changed from Antoine Martin to Denis01 |
---|---|
Status: | assigned → new |
program_context
: don't worry about this, you don't need to understand it
- sqlite:
it seems that Sqlite doesn't support sql schemes. So 2 ways are possible:
create a DB in ":memory:"
create a DB as a second db file
No idea what you mean here.
All SQL databases support creating a schema for sure: https://sqlite.org/lang_createtable.html.
comment:5 Changed 4 years ago by
1) program_context: just to avoid asking "stupid" questions.
Tried to pass the DB name thru command string but failed. What is a right way to do?
For example. Is that a right command?
xpra proxy --bind-tcp=XX.XX.XX.XX:443 --auth=sql:create=/root/xpra.sdb
2) SQLite.
As don't know what you would like to do after with that .... my question was just simply inspired by the following discussion:
http://stackoverflow.com/questions/30897377/python-sqlite3-create-a-schema-without-having-to-use-a-second-database
but ok, will start with db.connect("temp DB.file"). And then you will check if it is OK or not
comment:6 Changed 4 years ago by
You seem to be confused about what "creating the schema" means, see https://en.wikipedia.org/wiki/Database_schema. In this instance, it means creating the table(s) required for using the auth module.
comment:7 Changed 4 years ago by
Ok. Just I started thinking that talking about mysqlauth, postgreessql, superclass, create schema ... you meant to have a kind of abstraction for sql module. :-) Now i think that the idea of main() with "create" inside is just to create a new empty DB with the predefined
structure for new installation of xpra.
If it is a right thinking i'll do "create" as a first step and then "update" (for the future releases sql changes) as the second :-)
Changed 4 years ago by
Attachment: | sql_auth.py added |
---|
comment:8 Changed 4 years ago by
Hello,
could you please check the script (basically SQL replacement for Multifile for now). If OK - I will add the function for dynamic sessions initialization and balancing.
To create DB:
python /../.../sql_auth.py create [DB path]
DB path (for example) - /root/xpra.sdb
In SQL(table users) the following fields should be fulfilled:
1) user_ipn - login name
2) pass - password
3) displays (the same format as in Multifile - tcp:XX.XX.XX.XX:port)
4) env_options (optional, the same format as in Multifile)
5) session_options (optional, the same format as in Multifile)
To start xpra
xpra --bind-tcp=XX.XX.XX.XX:port --auth=sql:filename=/.../xpra.sdb
or xpra proxy...
comment:9 Changed 4 years ago by
You've just taken the stub sql auth script I had posted, and added load balancing stuff to get_sessions
- which does not belong there.
You have not addressed any of the issues I had listed.
comment:10 Changed 4 years ago by
Hello!
I apologize! was very tired and posted another file!
Hope that new one is better
Changed 4 years ago by
Attachment: | sql_auth_NEW.py added |
---|
comment:12 Changed 4 years ago by
That's better.
Please address the following points:
- the
init
function is not needed, sqlauth does not use the legacy password file option - remove this comment:
temp. probably to be modified for sql file location reading. Copied from Multifile
- there is nothing to modify here, the filename is configurable, as it should be - remove unnecessary newlines, add the missing spaces in
self.db_name=filename
->self.db_name = filename
- "db_name" may not be the best variable name here, "filename" would be better
- change "user_ipn" to just "username", change "pass" to "password"
- what is "user_conn_str" and what is it doing here?
log.error("SQL_auth. No database: '%s'", self.db_name)
, please use the format:log.error("Error: sqlauth cannot open the database file '%s'", self.db_name)
- also, this codepath should return "None" explicitly.- too many unnecessary brackets here:
cursor.execute(self.password_query, ([self.username]))
- don't use
self.sessions
, use a "sessions" local variable instead - the case where "#dynamic port/session initialisation" will trigger an error as the sessions list is not defined
- it's likely that there can be errors when calling
sqlite3.connect
(ie: if the file is invalid), this should be handled
Now for the "create" command code:
- this belongs in a separate function (call it "create"), call this from main
- there was already a stub for it (see line 136) - why place it somewhere else?
- don't use
print
- don't use
sys.argv[x]
repeatedly, assign it to a descriptive variable name - remove the file extension check, people can use what they want
- the sql scripts should be an array, not a multi-line string
- remove the tables that have nothing to do with this sqlauth module ("open_sessions", "busy_ports", "hosts")
- use this error format:
log.error("Error: failed to create database schema: %s", e)
- replace "print" with "log.info"
comment:13 Changed 4 years ago by
OK. will try to modify ASAP.
Field "user_conn_str" in DB stands for "user_connection_string" and should point to dynamic session initialization and has a format like "--start-child=/bin/gedit --exit-with-children" to start process (will be used in balancing module).
So
if the field "displays" is not empty - Static session
if displays is empty and the field "user_conn_str" is not empty - Dynamic session.
If both are empty - error
comment:14 Changed 4 years ago by
New update.
Everything should be taken in account except:
1) "the init function is not needed, sqlauth does not use the legacy password file option"
have not figured out how to do it without
init(opts)
as then
password_file = opts.password_file
says that object not found
2) "function (call it "create"), call this from main" and "was already a stub for it (see line 136) why place it somewhere else?"
put "create" inside
with program_context("SQL Auth", "SQL Auth"):
as before for test reasons it was out of that branch.
Or it is needed to create a specific dedicated
def create() ....
function?
Changed 4 years ago by
Attachment: | sql_auth_new_up1.py added |
---|
comment:15 Changed 4 years ago by
user_conn_str
is not used - remove it, this doesn't look like the right place for it anywaysql_file_name
should befilename
(makes it consistent options argument name, we know it is sql already anyway)cursor = conn.cursor()
should be moved in the try-except block as this may trigger exceptions- can
conn.commit()
trigger exceptions? Is it even needed when doing reads? entry=str(data[0])
is missing spaces, don't bother creating a temporary "entry" variable, just return the value (the same could be done for sys.exit + main)self.uid=...
etc, all the way toself.session_options
: missing spaces, don't useself.
- indentation is wrong in
main
- probably tabs vs spaces - please move all the
if cmd=="create":
code to a function, and move all the database access code within the try-except block
Other things left to be done:
- unit tests
- man page
- wiki edits
comment:16 Changed 4 years ago by
Done
Comments:
1) "user_conn_str" is used (or will be used: lines 86-90) and logically it is user's data like "Display" and other settings so might be in the same table.
And as a result of lines 86-90 the sessions info is delivered (the same for static session info).
But if should be deleted where it has to be moved?
2) I test each time when code is modified
For wiki/man - ok, sure
Changed 4 years ago by
Attachment: | sql_auth_new_up2py added |
---|
comment:17 Changed 4 years ago by
1) it isn't used yet and as I said multiple times - this is probably not the right place for it anyway
2) we have unit tests for all auth modules:
browser/xpra/trunk/src/unittests/unit/server/auth_test.py, this one will be no exception
comment:18 Changed 4 years ago by
2) ok, will try to figure out how it works
1) being ready to continuer to test and etc. please give a hint where is a good place for it :-)
comment:19 Changed 4 years ago by
Still many things to fix:
- remove
#import xpra_lb.py
- remove
init
function - remove two instances of this useless code:
else: pass
user_conn_str
still needs removing, and the empty if statements that go with itconn.close
- doesn't call the function- return statement at the end of the function doesn't do anything
- indentation is still very very wrong, everywhere - use spaces not tabs
- "pass" does nothing near the end
- remove newline before sys.exit
As for the right place for creating sessions, this belongs in the proxy code where it already has the ability to create new sessions - it just needs to be enhanced to be able to start remote sessions. (and I have already pointed out the function that provides this feature too)
comment:20 Changed 4 years ago by
Ok, sure.
And other point (if you don't mind) will add a call for "create()" inside "...main...." (and keep the same call from "context" branch) as basically it doesn't work from command line for now
Concerning Sessions creation:
1) so to make the session creation function independent from the type of auth module (file, sqlite, mysql, etc) you want to include that in
start_proxy(): try: sessions = client_proto.authenticator.get_sessions()
comment:21 Changed 4 years ago by
ops (press the enter)...
right?
2) "already pointed out the function that provides this feature too"
if it is
xpra start ssh/username:password@HOST:SSHPORT/DISPLAY --start=xterm --attach=no"
function. As far as I understand there is no PORT identification on which client application should start at target server. Only SSHPORT to connect.
comment:22 Changed 4 years ago by
And other point (if you don't mind) will add a call for "create()" inside "main" (and keep the same call from "context" branch) as basically it doesn't work from command line
No. The context code doesn't change sys.argv
, you must be doing something wrong.
1) so to make the session creation function independent from the type of auth module (file, sqlite, mysql, etc) you want to include that in
No. Like I said, where it already includes code to start a new session.
if it is
No it isn't.
As far as I understand there is no PORT identification on which client application should start at target server. Only SSHPORT to connect.
This is not a function, this is a command line.
And anyway, it does support specifying the port: add bind-tcp=
to this command.
In any case, this doesn't really belong in this sqlauth ticket.
comment:23 Changed 4 years ago by
Create()
just to be sure that i understand correctly.
In the
sql_auth_new_up2.py
the create function moved to
with program_context("SQL Auth", "SQL Auth"): if cmd=="create": create(db_file_name)
and basically is not called from this block from the command line.
So moving create(db_file_name) out of this block will allow to call it directly from command line.
If you don't mind - will do that
comment:24 Changed 4 years ago by
If you don't mind - will do that
No. Don't do that.
Figure out why it isn't called from where it should be called, and fix that.
comment:25 Changed 4 years ago by
Here's a hint: address ALL my earlier comments and it will probably be called.
Indentation matters.
comment:26 Changed 4 years ago by
:-) seems to be resolved
Another hint is wanted :-)
Concerning
init(opts)
what is "opts"? tried to find it in "sys_auth_base" but there is the same definition there
comment:27 Changed 4 years ago by
Just without it can't remove "init" function.
Filename is not defined
comment:28 Changed 4 years ago by
Just remove the init
function completely and either replace password_file
by a sensible default value (ie: "auth.sdb") or throw an exception if the "filename" attribute is not specified.
This function is only used for password file authentication and legacy command line options - it will be removed soon.
comment:29 Changed 4 years ago by
ok. got it
So that will be removed and module won't be fully (with the possibility to point DB file) operational for now. Waiting for future evolutions
comment:30 Changed 4 years ago by
So that will be removed and module won't be fully (with the possibility to point DB file) operational for now. Waiting for future evolutions
Just leave the function in for now, but empty.
You specify the database file through the filename attribute, it should be fully operational as it is.
Changed 4 years ago by
Attachment: | sql_auth_new_up3.py added |
---|
comment:32 Changed 4 years ago by
Looks good, only found two remaining problems:
- in
get_sessions
, the "sessions" variable may not be defined when reaching the end of that function - it checks for
data[3]
to decide to load the data or not. It would probably be clearer to assign those to meaningful variable names instead.
Not sure why we have "Error: sqlauth. No session type is identified" in the second branch. If we don't have displays, what's the problem? The proxy may still create or find one for us. (this will be refined with the ability to start sessions).
comment:33 Changed 4 years ago by
:-)
and data[3] is wrong
. should be data[2]
. just have finished testing and it didn't work.
data[2]=displays
"Session type is not defined". And "sessions definition..."
Now having looked at proxy_server.py and r15693 it seems that "displays" can be empty....
And return only "uid" and "guid" (mandatory fields) as sessions from sql_auth...
Changed 4 years ago by
Attachment: | sql_auth_4.py added |
---|
comment:35 Changed 4 years ago by
And 5th version.
DB schema adjusted - "username", "uid", "guid" have to be UNIQUE and NOT NULL
Changed 4 years ago by
Attachment: | sql_auth_5.py added |
---|
comment:36 Changed 4 years ago by
No,uid and guid should not be UNIQUE.
Users can login to the same environement
Will be corrected in the code
comment:37 Changed 4 years ago by
Merged with the following changes in r15752 (+ r15756 fixup for MS Windows):
- renamed to sqlite_auth (as we may add mysql_auth, etc)
- removed unused import
- removed end of lines whitespace
- added missing whitespace around some assignments
- consistently use uppercase for SQL keywords
- removed empty else statement
- improved error / log messages. ie: log full stacktraces at debug level
- access row data by name rather than indexes (the SQL field names we lookup could be made configurable if needed)
- parse displays as CSV value.. maybe this should be a separate table.. oh well
- the create() function now returns an exit code to indicate success or failure
- remove irrelevant fields from
CREATE TABLE
- split long SQL
CREATE TABLE
line into multiple lines - changes to server core to load the module, changed from original stub patch: this module is optional (as some binary builds may not have the sqlite module)
- added "list" "add" and "remove" subcommands to manipulate the database
- man page update
- packaging wrappers for MS Windows and Mac OS, with icons
- unit tests: creating the database, testing auth, etc..
- added to wiki: wiki/Authentication
Tested by hand using:
python ./xpra/server/auth/sqlite_auth.py auth.sdb create python ./xpra/server/auth/sqlite_auth.py auth.sdb add foo bar xpra start --bind-tcp=0.0.0.0: --tcp-auth=sqlite:filename=`pwd`/auth.sdb xpra attach tcp/foo:bar@localhost/
@Denis01: please close if this works for you.
comment:38 Changed 4 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Superb!
Works.
tested with
xpra proxy ....
comment:39 Changed 4 years ago by
Minor fix: r15757 allows us to use relative paths, even when daemonizing the server.
comment:41 Changed 4 years ago by
Summary: | Authorisation. New module - sql → new auth module: sql |
---|
(editing ticket title)
comment:42 Changed 4 years ago by
Summary: | new auth module: sql → new auth module: sqlsqlite |
---|
comment:43 Changed 3 years ago by
As of r17435, the username is no longer unique and we allow multiple passwords for the same user.
Will be useful for things like #1486 and may allow us to move away from multifile.
With a sqlite backend, we could easily provide a simple GUI too.