Bug 7554 - Parts of Web Administration is still written in Python 2
Summary: Parts of Web Administration is still written in Python 2
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Web Administration (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.13.0
Assignee: Frida Flodin
URL:
Keywords: nikle_tester, ossman_tester, prosaic
Depends on: 7551 7553 7556 7557
Blocks: 4586
  Show dependency treegraph
 
Reported: 2020-09-09 16:20 CEST by Linn
Modified: 2021-07-14 09:31 CEST (History)
3 users (show)

See Also:
Acceptance Criteria:


Attachments

Description Linn cendio 2020-09-09 16:20:07 CEST
Python 2 is getting old and unsupported so we should rewrite the parts of Web Administration that uses Python 2 to python 3.
Comment 28 Frida Flodin cendio 2020-10-27 16:21:01 CET
tl_desktop
----------
tl_desktop.groups and tl_desktop.apps should now be fully converted to support
Python 2 and 3. There are two usages of these modules at the moment:

  1. The 'Desktop Customizer'-page in tlwebadm
> tladm/modules/thinlinc/tlwebadm/desktop.py
  I have tested that this page still works correctly in Python 2. Testing
  included:
   - Adding and deleting apps via 'Applications (Manual)'.
   - Adding and deleting groups via 'Application Groups'
   - Adding a new sub menu via 'Menu Structure'
   - Add applications to the new menu by changing 'Applications added to menu' 
     in a group.
   - Then checking that the group name is shown next to the application in one 
     of the 'Application' pages.
  (All testing included non-ascii characters)


  2. tl-desktop-builder script
> tlmisc/tl-desktop/bin/tl-desktop-builder
  I have tested editing a group (via tlwebadm), adding a menu application and a 
  desktop application and then add my user to user list. Then start a client 
  and check that the application is in the menu and on the desktop.



The behavior of tl_desktop should be exactly the same as before this conversion. I have done some changes to the walk function that looks for *.desktop files. The unit tests for apps.py is extended to test
this function in more detail. I tested this when running tlwebadm with Python 2
and placing a new *.desktop file in a new sub directory under
/opt/thinlinc/desktops/xdg_data_dir/applications/thinlinc
Then the applications should show up in tlwebadm under 'Applications (Manual)'.

A note on string handling:
- In Python 2 all strings passed in to tl_desktop should be utf-8 encoded bytes strings.
  And out also utf-8 encoded bytes strings (string objects).
- In Python 3 all strings in and out from tl_desktop should be str objects
  (i.e. unicoded).

Things that can be cleaned up when dropping Python 2 support:
- group.py: Remove Python 2 string handling indicated with FIXME and
> if bytes == str
- test_groups and test_apps: clean up imports, indicated with FIXME
- test_groups: Remove Python 2 string handling indicated with FIXME
Comment 36 Frida Flodin cendio 2020-11-06 14:33:13 CET
tlprinter.py
----------

Tlprinter is now compatible with both Python 2 and 3. There are no major changes to the Python 2 code, but I tested the following under Pyhton 2 after my changes.

1. Printing to nearest printer
>printer/cups/nearest:
Tested scenario:
Server on Ubuntu 20.04
Client on Fedora 32

Configured a location and a terminal with my MAC-address via webadm.
Connected with the Client on my workstation.
Printed to 'nearest'.
The printing job was sent to the first printer configured for the location.

Added a fake printer to the server and did not add it to my location.
When I list all available printers from the client, I only see the ones added to the location and not the new one.
When printing to an allowed printer it works and when trying to print to the other printers, that are not allowed it is denied.

So everything seems to work as expected.

2. Printing to thinlocal
>printer/cups/thinlocal:
Tested scenario:
Server on Ubuntu 20.04
Clients on Fedora 32, MacOS 10.15.7 and Windows10

Printing to thinlocal on all clients works fine. The print job is sent to the default printer on the client computer.

Also tested raw printing by running

>lpr -P thinlocal -l <file_to_print>
Works fine.

3. Administrating printer access
>tladm/modules/thinlinc/tlwebadm/locations.py:
Tested in from Webadmin:

- Add a printer in Locations/Locations/Add new location/Add printer
- Add new terminal and enter unknown hardware address
- Add new terminal and enter known but non-internal format hardware address like:  0123456789AB. It is converted to the internal format 01:23:45:67:89:AB
Comment 55 Linn cendio 2020-11-13 17:00:14 CET
Python 2 compatibility code to remove:

subcluster.py
-------------
* No compability code. Finished as is.

test_subcluster.py
------------------
* Clean up import of mock

* There are two versions of test:
> get_agents_using_unicode_username_returns_correct_agents()
 The test that is only run in Python 2 should be removed, and the other test should have an if-case in the beginning of the test removed. Indicated by FIXME:s.

vsm/tests/base.py
-----------------
* Clean up import of mock
Comment 81 Niko Lehto cendio 2020-11-20 13:47:02 CET
When running an latin-9 (iso 8859-15) RHEL8 system. If the username contains UTF-8 (in my case 'ö') we get a trace back in tlwebadmin page status/sessions.

To reproduce:
Given:
  - latin-9 system
  - username containing 'ö'
  - ^user has active session
When:
  - accessing status/sessions tab in webadmin pages
Then:
  - The page crashes
  - Trace back can now be seen in tlwebadm.log on the server

Another thing that dies is the licence-graph in webadmin status/home. This image will not be displayed properly and you get similar trace back in tlwebadm.log

This issue started after changes in r35723.

The trace back I get is:
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20] Exception happened during processing of request from ('::ffff:10.47.1.20', 44580, 0, 0)
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20] Traceback (most recent call last):
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/forkingserver.py", line 61, in process_request
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     self . finish_request ( request , client_address )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 311, in finish_request
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     TLSServer . finish_request ( self , request , client_address )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/tlstunnel.py", line 95, in finish_request
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     self . TLSRequestHandlerClass ( request , client_address , self )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 85, in __init__
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     client_address , server )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/usr/lib64/python2.7/SocketServer.py", line 655, in __init__
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     self.handle()
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 285, in handle
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     RequestHandlerClass . handle ( self )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/usr/lib64/python2.7/BaseHTTPServer.py", line 340, in handle
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     self.handle_one_request()
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/httpserver.py", line 113, in handle_one_request
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     BaseHTTPRequestHandler . handle_one_request ( self )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/usr/lib64/python2.7/BaseHTTPServer.py", line 328, in handle_one_request
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     method()
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/server.py", line 150, in do_GET
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     self . post_or_get ( "do_GET" , oO )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/server.py", line 184, in post_or_get
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20]     self . _write ( 200 , oOOO00o , i11 . encode ( 'utf-8' ) )
>2020-11-20 09:34:28 ERROR tlwebadm[6929]: [::ffff:10.47.1.20] UnicodeDecodeError: 'ascii' codec can't decode byte 0x89 in position 0: ordinal not in range(128)
Comment 94 Frida Flodin cendio 2020-11-25 10:11:26 CET
LowPortServerProxy
------------------

As a dependency to tlprinter, LowPortServerProxy is now supporting Python 3. This affects printer access control, tl-limit-printers.

tl-limit-printers still run with Python 2 and I have tested that it still works after adding Python 3 support to LowPortServerProxy. I tested with server on Ubuntu20.04 and one terminal configured with one printer. I did no stress test, just one user on one terminal. It worked fine.
Comment 95 Frida Flodin cendio 2020-11-25 11:34:07 CET
tlprinter
---------

When getting current printers, we now read from lpstat output in text mode. This can cause UnicodeDecodeError. However, it should not happen in practice as long as lpstat writes to stdout with the same encoding as we use when reading. Since lpstat inherits our environment, this should be the case.
Comment 97 Frida Flodin cendio 2020-11-25 12:43:51 CET
tlwebadm locations page
-----------------------

The locations page now support Python 3. 
I have tested the following in Python 3: 

* Adding and deleting terminals and locations.
  - With normal names
  - With Unicode names

* Editing already existing locations and terminals.

* Adding printers to locations and terminals.

* Adding printer with Unicode name (Tested on utf-8 and latin-1 system).
  - Created a printer on the server
>     lpadmin -p printör -E -v file:///dev/null
  - When listing all printers, when adding new location, the correct name 
    is displayed.
  - Add 'printör' and save, works fine.

* Connecting a terminal to a location.

* Using a location for unknown terminals.

* Try to add terminal and location with already existing names.

* Printer access control, tl-limit-printers, after my configuration via tlwebadm:
  - With normal printer name
  - With Unicode printer name
Comment 166 Linn cendio 2020-12-09 09:35:42 CET
For testing:

To be able to reproduce the log warnings for tlwebadm about deprecation and unclosed sockets/files, run command:
> sudo PYTHONMALLOC=debug PYTHONASYNCIODEBUG=1 python3 -W default -X faulthandler -X tracemalloc=5 /opt/thinlinc/sbin/tlwebadm --foreground
The warnings were seen when entering 'System Health', 'Status' and 'VSM' pages.
Comment 169 Linn cendio 2020-12-10 17:04:40 CET
I tested webadmin on Firefox 82 on Ubuntu 18, and didn't find any missed errors. 

I visited all different pages, used non-ascii input and added/deleted subclusters, locations, apps, etc. Authentication also worked well, and I didn't see anything suspicious in the log.

The only thing I didn't test was shadowing (Status -> Sessions), since it didn't work for me because tlclient.cgi wasn't available on my server. This is a known bug (https://www.cendio.com/bugzilla/show_bug.cgi?id=5813), but it means that this feature is not tested.
Comment 170 Linn cendio 2020-12-14 15:48:19 CET
Things that have changed and need testing:

tlwebadm
--------
* Everything

tlwebaccess
-----------
* logging

vsmagent
--------
* handle_accept()

vsmserver
---------
* subcluster

sso
---
* getting and setting

printing
--------
* nearest
* thinlocal
* respooling
* tl-limit-printer

misc
----
* tl-gen-auth
* tl-desktop-builder
* tlmisc/tl-rsync-all
* tlmisc/tl-ssh-all
Comment 173 Linn cendio 2020-12-18 17:41:46 CET
All dependencies above have been tested, and everything seems to be working fine.

Everything was tested on RHEL8, except for tl-desktop-builder which was tested on Ubuntu 18.04. Below are some extra comments about what was tested.


vsmserver
---------
* subcluster (Can handle two agents in a subcluster)

sso
---
* getting and setting (Getting password/passphrase used when logging in. Updating
                       password.)

printing
--------
* nearest (Using both printer set on terminal and printer only set on location)
* tl-limit-printer (Works for both know and unknown terminals)

misc
----
* tl-gen-auth (Tested both ascii and unicode password)
* tl-desktop-builder (Adding programs in menus and on desktop. Added a manual
                      program. Unicode menu names)
Comment 174 Pierre Ossman cendio 2020-12-21 09:21:24 CET
Tested on SLES 12, with Python 3.4:

Main:

  Nothing to test

System Health:

  Services status
  User and group lookup
  Unicode user lookup

Status:

  Graphs
  Load
  Load with subclusters
  Load with downed agent

Sessions:

  List
  Details
  Terminate
  Multiple sessions for a user
  Unicode all above
Comment 175 Niko Lehto cendio 2020-12-22 14:40:16 CET
Tested on RHEL8, Python 3.6:

Webadmin:
  VSM
  Profiles
  Locations
  Desktop customizer
  Documentation center

Webaccess:
  Logging

Printing:
  Nearest
  Thinlocal
  Respooling
  tl-limit-printers

VSMserver:
  Subclusters

VSMagent
  handle_accept()

SSO:
  Getting
  Setting

Misc:
  tl-gen-auth
  tl-desktop-builder
  tl-rsync-all
  tl-ssh-all
Comment 176 Frida Flodin cendio 2021-02-16 16:57:28 CET
(In reply to Frida from comment #95)
> tlprinter
> ---------
> 
> When getting current printers, we now read from lpstat output in text mode.
> This can cause UnicodeDecodeError. However, it should not happen in practice
> as long as lpstat writes to stdout with the same encoding as we use when
> reading. Since lpstat inherits our environment, this should be the case.

Listing current printers is broken when running with locale set to zh_CN.GB18030 on Ubuntu20.04.

To reproduce:
- Set locale to zh_CN.GB18030
- reboot
- Go to tlwebadm /locations/terminals and click 'Add new terminal'
- Get: Error code: 500 and see traceback bellow

It works to get the printers when running in Python 2. tl-limit-printers works fine. So it seems to be different behavior with the universal_newline parameter to Popen in Python 2 and 3.



> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30] ----------------------------------------
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30] Traceback (most recent call last):
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/server.py", line 162, in post_or_get
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]     OooO0O00o0 , OOo = getattr ( IIiII11I , action ) ( I11ii1IiIii , query , OooO0O00o0 )
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/main.py", line 131, in do_GET
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]     self . _GET_METHODS . get ( page_name , self . error_404 ) ( query ) )
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]   File "/opt/thinlinc/modules/thinlinc/tlwebadm/locations.py", line 138, in terminals
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]     Ooo [ "all_printers" ] = get_current_printerlist ( username = None )
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]   File "/opt/thinlinc/modules/thinlinc/tlprinter.py", line 413, in get_current_printerlist
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30]     oO00oOOoooO = IiIi11iI . stdout . readlines ( )
> 2021-02-16 15:42:27 ERROR tlwebadm[4667]: [::ffff:10.47.1.30] gnutls_record_recv: The TLS connection was non-properly terminated.
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30] UnicodeDecodeError: 'gb18030' codec can't decode byte 0x80 in position 52: illegal multibyte sequence
> 2021-02-16 15:42:27 ERROR tlwebadm[4667]: [::ffff:10.47.1.30] gnutls_bye: The specified session has been invalidated for some reason.
> 2021-02-16 15:42:27 ERROR tlwebadm[4666]: [::ffff:10.47.1.30] ----------------------------------------
Comment 179 Frida Flodin cendio 2021-02-19 14:18:08 CET
(In reply to Frida from comment #176)
Fixed now!

Tested the following on Ubuntu20.04 with build 6753.r36201:
✓ ASCII printer name both tlwebadm and tl-limit-printer
✓ non-ASCII printer name in tlwebadm
✗ non-ASCII printer name in tl-limit-printers*
✓ tlwebadm/locations on server with locale GB18030 and UTF-8
✓ tl-limit-printers on server with locale GB18030 and UTF-8


* This is broken as long as we still run tl-limit-printers with Python 2. When tl-limit-printers is running with Python 3 this should work.
Comment 184 William Sjöblom cendio 2021-03-02 10:10:16 CET
Tested on Ubuntu 20.04 with revision r36259.

Using the same tests as described by Frida (see comment 179), but with a wider range of encodings: UTF-8, Latin-1, Latin-9, BIG5-HKSCS, GB18030. 

The problem with non-ASCII printer names in tl-limit-printers still remain until it has been converted to python 3 (as further detailed by Frida in comment 179).
Comment 185 Pierre Ossman cendio 2021-03-30 16:15:30 CEST
Trying to start a second tlwebadm results in a ResourceWarning for the pid file:

> /opt/thinlinc/sbin/tlwebadm:72: ResourceWarning: unclosed file <_io.TextIOWrapper name='/var/run/tlwebadm.pid' mode='r' encoding='UTF-8'>
>   Ooo0OO = int ( open ( I1i1Ii ) . readline ( ) )
> ResourceWarning: Enable tracemalloc to get the object allocation traceback
Comment 188 Pierre Ossman cendio 2021-03-31 14:19:26 CEST
Fixed now.
Comment 190 Niko Lehto cendio 2021-04-01 11:05:15 CEST
Reproduced on Fedora 33 running Python 3.9

This resource warning is no longer present when starting another tlwebadmn service.

Note You need to log in before you can comment on or make changes to this bug.