new auth module: sqlsqlite

[Denis01]

Create possibility to keep the users records (like multifile records structure) in mySQL table

[Antoine Martin]

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.

[Antoine Martin]

stub for sql auth

[Antoine Martin]

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.

[Denis01]

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.

[Antoine Martin]

• 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.

[Denis01]

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

[Antoine Martin]

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.

[Denis01]

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 :-)

[Denis01]

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...

[Antoine Martin]

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.

[Denis01]

Hello!
I apologize! was very tired and posted another file!
Hope that new one is better

[Denis01]

sql_auth_NEW.py​ is a correct one

[Antoine Martin]

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"

[Denis01]

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

[Denis01]

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?

[Antoine Martin]

• user_conn_str is not used - remove it, this doesn't look like the right place for it anyway
• sql_file_name should be filename (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 to self.session_options: missing spaces, don't use self.
• 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

[Denis01]

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

[Antoine Martin]

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

[Denis01]

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 :-)

[Antoine Martin]

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 it
• conn.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)

[Denis01]

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()

[Denis01]

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.

[Antoine Martin]

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.

[Denis01]

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

[Antoine Martin]

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.

[Antoine Martin]

Here's a hint: address ALL my earlier comments and it will probably be called.
Indentation matters.

[Denis01]

:-) 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

[Denis01]

Just without it can't remove "init" function.
Filename is not defined

[Antoine Martin]

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.

[Denis01]

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

[Antoine Martin]

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.

[Denis01]

Ok, done.
And all the comments from "comment:19" are included.

[Antoine Martin]

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).

[Denis01]

:-)
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...

[Denis01]

So should be like that (4 version in attached file)

[Denis01]

And 5th version.
DB schema adjusted - "username", "uid", "guid" have to be UNIQUE and NOT NULL

[Denis01]

No,uid and guid should not be UNIQUE.
Users can login to the same environement
Will be corrected in the code

[Antoine Martin]

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.

[Denis01]

Superb!
Works.
tested with

xpra proxy ....

[Antoine Martin]

Minor fix: r15757 allows us to use relative paths, even when daemonizing the server.

[Antoine Martin]

(edit milestone)

[Antoine Martin]

(editing ticket title)

[Antoine Martin]

As of r17435, the username is no longer unique and we allow multiple passwords for the same user.

[Antoine Martin]

r17435 caused a bug: since the username is no longer unique (only username+password is), the query for returning the sessions needed updating to take into account the password (which we need to keep around for that purpose). Done in r20501.

[Antoine Martin]

Follow up: #2287 for mysql.