Comment 6 Alexander Zeijlon 2024-04-19 10:14:30 CEST

I'm looking through the commits for tl-env, and the change in option parser introduces a change in behavior that would require users to revise how they use the script. This is in cases where the command, tl-env is supposed to run, is also provided with its own arguments and options.

For example if you try to run ls -la through tl-env, with the old getopt-parser you can simply run 
> tl-env ls -la
and the options "-la" will be assumed to belong to ls.

With the new usage of our option parser, that extends optparse.OptionParser, the very same call will result in "-l" being interpreted as belonging to tl-env (which has no such option).

To work around this, one can provide the command with options and arguments but add "--" before the command.
> tl-env -- ls -la

This change in behavior could have unforeseen implications for our users, e.g. arguments for sub commands silently being interpreted as arguments to tl-env etc. Hence, we should look at preserving the old behavior when using the new OptionParser.

Comment 9 Alexander Zeijlon 2024-04-19 10:53:46 CEST

The reason this issue occurs with optparse is that it by default supports interspersed arguments, where it assumes that simple options that take no arguments can come in any order [1], which means that it will not stop parsing on the first non-option argument. Hence, as with the ls example in comment 6, it would then assume that -l is to be interpreted as an option to tl-env.

[1] https://docs.python.org/3/library/optparse.html#querying-and-manipulating-your-option-parser

Thankfully, optparse provides a method for turning this behavior off, which we can use to get the behavior we had with the getopt parser while still using our optparse implementation.

Comment 14 Linn 2024-04-22 09:40:38 CEST

Tested running tl-config (hivetool under the hood) after swapping to our option parser, and the behaviour is equivalent to 4.16.0 except for some minor changes:

 * To show the help, flag '-?' has been replaced with flag '-h'. This is more in line with both our other scripts and general Linux syntax.

 * To show the version number, flag '-v' has been removed, so '--version' is now the only flag to show the version number. Other scripts that use our option parser don't allow '-v', so it is better to be consistent with our other scripts.

Both of these flags should be rare to use in scripts, so breaking backwards compatibility here should have very limited impact. Note that the swap to using ThinLinc's option parser in hivetool means that hivetool no longer is independent from ThinLinc.


Below is a list of the commands I tested on both 4.16.0 and 4.16.0post:
https://www.cendio.com/resources/docs/tag-devel/html/man/tl-config.1.html
> tl-config -a shadowing
> tl-config --recursive -a /shadowing
> tl-config --recursive --all-entries /shadowing
> tl-config -r /opt/thinlinc/etc/conf.d/shadowing.hconf -Ra /
> tl-config --root /opt/thinlinc/etc/conf.d/shadowing.hconf -Ra /
> tl-config /shadowing/shadowing_mode="wooo"
> 
>  # -R/--recursive must come before -a/--all-entries
> tl-config -aR /shadowing
> tl-config -a --recursive /shadowing
>  # Error output for both calls:
>  # /shadowing: No such parameter
>  # R: Folder not found
> 
>  # Output when using -e/--eval does not include variable value
> tl-config -e /shadowing/shadowing_mode="nootnoot"
>  # Output: /shadowing/shadowing_mode=''
> tl-config --eval /shadowing/shadowing_mode="nootnoot"
>  # Output: /shadowing/shadowing_mode=''
> 
> tl-config -E /vsmserver
> tl-config -x -E /shadowing
> tl-config --export -E /shadowing
> 
> # Purging duplicate params I put in  vsmserver.hconf
> tl-config -p /opt/thinlinc/etc/conf.d/vsmserver.hconf
> tl-config --purge /opt/thinlinc/etc/conf.d/vsmserver.hconf
> 
> # Added new params in test.hconf, can list them after import
> tl-config -i test.hconf
> tl-config --import test.hconf
> 
> tl-config -h
> tl-config --help
> tl-config --version
> 
> tl-config -?
> tl-config -v
>  # No longer works, flags have been removed/replaced
>  # Output:
>  # Usage: hivetool [options] [type:]parameter[=value] ...
>  #
>  # hivetool: error: no such option: -v

Comment 21 Adam Halim 2024-04-23 12:27:47 CEST

tl-ldap-certalias has the following options available:
    * -v, --verbose
    * -d, --debug
    * -s, --simulate
    * -h, --help
    * --version

Tested the following combinations, and the output was identical to how it was
in 4.16.0:
> tl-ldap-certalias -s
> tl-ldap-certalias -d
> tl-ldap-certalias -v
> tl-ldap-certalias '-s -d'
> tl-ldap-certalias '-s -d -v'
> tl-ldap-certalias '-s -v'
Also ran test_5880 [1] on both versions and got the same results.

[1] https://git.cendio.se/cendio/test_5880

Comment 28 Adam Halim 2024-04-25 11:25:01 CEST

tl-session-param has the following options available:
    * -a, --all-entries
    * -e, --eval
    * -h, --help
    * --version

We tested the following on 4.16.0 and 4.16.0post:

In 4.16.0, the examples from the man page are printed as well.
Otherwise, the output is identical:
> /opt/thinlinc/bin/tl-session-param
The following commands have the same output in both versions:
> /opt/thinlinc/bin/tl-session-param -a /
> /opt/thinlinc/bin/tl-session-param -e LANG=/client_params/capabilities/client_lang
> /opt/thinlinc/bin/tl-session-param /client_ip
> /opt/thinlinc/bin/tl-session-param -e LANG=/client_params/capabilities/client_lang -a /
This command has a regression since 4.16.0, in 4.16.0 we get:
> /opt/thinlinc/bin/tl-session-param /client_ip -e LANG=/client_params/capabilities/client_lang 
> 10.47.1.80
> No such parameter as -e
and in 4.16.0post we get:
> LANG=en_GB.UTF-8
> 10.47.1.
Seems like the argument (/client_lang) has to come last for it to work:
> /opt/thinlinc/bin/tl-session-param -e LANG=/client_params/capabilities/client_lang -a / /client_ip

Just as in comment #14, the '-?' flag has been replaced with '-h' and '--help'.

Overall, the script looks good and behaves better now than in 4.16.0.

Comment 40 Adam Halim 2024-04-29 15:26:10 CEST

tl-support has the following options available:
    * -p, --port
    * -u, --user
    * -h, --help
    * --version
The script also accepts a hostname argument.

Tested the following on 4.16.0 and 4.16.0post and got the same behaviour on both versions:
> tl-support
> tl-support -p 10
> tl-support -u user
> tl-support -p 10 -u user
In 4.16.0, not supplying an argument to an option will print the usage
(--help). In 4.16.0post, we get a helpful error message, as well as a short
usage example. The same applies if an invalid argument is passed:
> tl-support -p
> tl-support -u
> tl-support -p 10 -u
> tl-support -u user -p
> tl-support -invalid_arg
> tl-support -u user -invalid_arg
> tl-support -p 10 -invalid_arg
> tl-support -p 10 -u user -invalid_arg
In 4.16.0, the argument/hostname has to come last for it
to be parsed correctly. In 4.16.0, the ordering of options/args
is irrelevant. We get the same behaviour in both versions:
> tl-support hostname
> tl-support -p 10 hostname
> tl-support -p 10 -u user hostname
> tl-support -p 10 hostname:10
> tl-support -u user hostname:10
> tl-support -p 10 -u user hostname:10
Same behaviour on both versions when supplying invalid argument,
except we used to get --help in addition to "Invalid hostname" printed
in 4.16.0.
> tl-support hostname:10:10
> tl-support -p 10 hostname:10:10
> tl-support -p 10 -u user hostname:10:10
> tl-support -p 10 hostname:10:10
> tl-support -u user hostname:10:10
> tl-support -p 10 -u user hostname:10:10
Overall, the behaviour is improved in 4.16.0post as we now get a more helpful
error message if the wrong argument is passed. The order of args/options is
also more robust now.

Comment 58 Samuel Mannehed 2024-05-10 17:06:20 CEST

The work done on this bug so far has been mostly testing and improved unit tests.  The verification of this bug will be done together with testing for bug 7867.

Comment 59 Tobias 2024-06-04 14:59:08 CEST

Expanded the scope of the MUST AC such that scripts/tools not only necessarily display the same behaviour, but are also potentially improved.

Also added a new SHOULD AC which require that man pages exhibit consistent layout.

Comment 60 Linn 2024-06-07 09:28:14 CEST

This bug will be tested at the same time as bug 7867.

Comment 61 Linn 2024-06-07 09:54:31 CEST

Regressiontested scripts tl-sso-password and tl-sso-token-passphrase. These have the same arguments and functionality, except that the former is for passwords and the latter is for smart cards. Below are all flags for the scripts.
>  -c	--check
>  -h	--help
>  -r	--remove
>  	--version
The examples here are only for tl-sso-password, but tl-sso-token-passphrase was tested in the same way.
> # Output the passphrase/PIN
> tl-sso-password | cat
> 
> # This command only outputs "hej" if a passphrase/PIN exists
> tl-sso-password -c && echo "hej"
> tl-sso-password --check && echo "hej"
> 
> # Remove the saved password/PIN
> tl-sso-password -r
> tl-sso-password --remove
> 
> # flags for -h, --help and --version works as before
Also checked the man pages for both scripts, they follow the same layout.

Comment 63 Linn 2024-06-07 13:20:41 CEST

Tested tl-support, and could successfully gain access to the test machine when following instructions on the intranet. Also tested the following:
> # Uses random port, default user 'support' and default host
> tl-support
> 
> # Uses the specified port
> tl-support -p 1234
> tl-support --port 1234
> 
> # Uses the specified username
> tl-support -u user1
> tl-support --user user1
> 
> # Uses to the specified host
> tl-support host.example.com
Also checked the man page, after commit r41011 it is in sync with the other man pages.

Comment 69 Samuel Mannehed 2024-06-07 16:55:02 CEST

Tested tl-mount-localdrives and tl-umount-localdrives from build 3623 on RHEL 9. 

I compared the man pages between build 3623 and 4.16.0, things look improved. Long versions of all flags, as well as --version were added. All new flags work as intended, as seen below. The sorting and capitalization of the options in the man page was also fixed.

tl-mount-localdrives:

 ✓ Can still successfully mount local drives.
   - This only applies when ran inside a ThinLinc session.
 ✓ The -h/--help flag prints man page contents.
 ✓ The -v/--verbose flag makes it print some info about what happened.

tl-umount-localdrives:

 ✓ Without options it umounts local drives for the current user session.
   - This only applies when ran inside a ThinLinc session.
 ✓ The -a/--all-users flag allows an admin to unmount all local drives.
   - Tested with an admin user unmounting drives for a different user.
 ✓ The -l/--no-adjust-symlink causes the ~/thindrives folder to remain.
   - This only applies when ran inside a ThinLinc session.
   - The flag has no effect when ran as an admin (see bug 8342).
 ✓ The -s/--all-sessions flag unmounts all session's drives of the user.
   - This is different from the default behavior of leaving drives mounted
     for other sessions for the current user.
   - This only applies when ran inside a ThinLinc session.
 ✓ The -v/--verbose flag makes it print some info about what happened.
 ✓ The --version flag prints the ThinLinc server version.

Comment 72 Alexander Zeijlon 2024-06-10 15:52:52 CEST

Tested tl-session-param from build 3627 on RHEL 9.

This build remains functionally consistent with the current 4.16.0 release. There is no change in behavior between versions when providing individual options (and arguments).

The --help is now more consistent with other scripts since we are using the same parser everywhere. The current release had some usage examples in --help that are no longer present, though they are still present in the man pages, which is good enough. Less is more!

The code in the main-function seems to indicate that it is possible to provide multiple arguments at the same time. E.g.:
> tl-session-param /client_ip /fbsize_x --eval VAR=/client_params/hwaddr --eval VAR2=/session_pid

This works on the new build, and to some extent on the current release. The current release seems to be a bit picky about in which order the inputs to the script are provided. The new build handles this more gracefully, where each option-argument pair can be placed arbitrarily while still being parsed correctly.

Comment 73 Tobias 2024-06-10 16:30:07 CEST

Tested tl-setup using build #3627 on RHEL9.

Options are working as intended and the supporting documentation is consistent with other tools.

Comment 74 Tobias 2024-06-10 16:36:52 CEST

Tested tl-config using build #3628 on RHEL9.

Options are working as intended and the documentation is consistent with
the other tools. The option -r/--root has now been hidden since it's not intended to be used.

The bug 3166 is still present, in that "$ tl-config -Ra /" works fine while "$
tl-config -aR /" doesn't work.

Comment 76 Alexander Zeijlon 2024-06-13 12:48:06 CEST

Tested tl-env using build #3630 on RHEL9.

There is no functional change observed since the migration to our option parser.

The documentation has been updated to reflect that we are now not presenting the users with the save option, -s, since this is only meant to be used by the server itself when sessions are created.

Comment 77 Linn 2024-06-13 16:14:31 CEST

Tested tl-ldap-certalias with Jenkins build 3630 according to the docker test from bug 5880. That test run still outputs the correct value for the users and their corresponding cert:

✓ Verify that a user with several certificates works as expected
✓ Verify that invalid signature of certificate is handled correctly
✓ Verify that invalid certificates are ignored, both expired and revoked

The docker tests also verifies the following:
✓ Verify that tl-ldap-certalias extracts public keys and populate ssh keys
✓ Verify that /etc/passwdaliases is populated with subject names
✓ Verify that invalid certificates can be ignored and used using 
  allow_invalid_certificates configuration key
✓ Verify that a valid certificate that is later being revoked removes the 
  public_key from authorized_keys

I edited the docker test to run in simulation mode (tl-ldap-certalias -s), and could see that the files in the user's home directory did not get updated. Output for -h, --help and --version also looked good.
 
The remaining flags only affects the output of the command, so I did not setup a working ldap environment, but rather checked that the script output enough messages depending on the flag:
> # Default output in my environment, no flags
> tl-ldap-certalias: ERROR: Configuration: user source not configured - exiting.
> 
> $ tl-ldap-certalias -d
> $ tl-ldap-certalias --debug
> tl-ldap-certalias:  INFO: This is a SELinux-enabled system.
> tl-ldap-certalias: DEBUG: Reading configuration at /opt/thinlinc/etc/thinlinc.hconf
> tl-ldap-certalias: ERROR: Configuration: user source not configured - exiting.
> tl-ldap-certalias:  INFO: tl-ldap-certalias returned error code 1
> 
> $ tl-ldap-certalias -v
> $ tl-ldap-certalias --verbose
> tl-ldap-certalias:  INFO: This is a SELinux-enabled system.
> tl-ldap-certalias: ERROR: Configuration: user source not configured - exiting.
> tl-ldap-certalias:  INFO: tl-ldap-certalias returned error code 1

Comment 78 Alexander Zeijlon 2024-06-14 13:08:32 CEST

Tested tl-notify using build #3630 on RHEL9.

There is no functional change observed since the migration to our option parser.

The documentation is consistent with our other tools. The output of --help has changed slightly, and for the better, where it now contains a short synopsis of what the command does.

Comment 79 Samuel Mannehed 2024-06-14 13:53:46 CEST

Tested tl-limit-printers using build 3630 on RHEL9. I also compared with 4.16.0 on RHEL9.

The help text is improved and the version flag was added:
> [cendio@lab-74 ~]$ tl-limit-printers --help
> Usage: tl-limit-printers [option]
> 
> Limit list of printers $USER is allowed to use and see
> The USER environment variable must be set to the username of the user for
> which the list of printers should be limited
> 
> Options:
>   -h, --help     show this help message and exit
>   -v, --verbose  will print extended information to stderr
>   --version      prints the ThinLinc version and exits
> [cendio@lab-74 ~]$ tl-limit-printers -h
> Usage: tl-limit-printers [option]
> 
> Limit list of printers $USER is allowed to use and see
> The USER environment variable must be set to the username of the user for
> which the list of printers should be limited
> 
> Options:
>   -h, --help     show this help message and exit
>   -v, --verbose  will print extended information to stderr
>   --version      prints the ThinLinc version and exits
> [cendio@lab-74 ~]$ tl-limit-printers -v
> Error opening lockfile (/var/opt/thinlinc/utils/tl-printer/tl-limit-printers.lock) in append mode: Permission denied
> Not limiting list of printers
> [cendio@lab-74 ~]$ tl-limit-printers --verbose
> Error opening lockfile (/var/opt/thinlinc/utils/tl-printer/tl-limit-printers.lock) in append mode: Permission denied
> Not limiting list of printers
> [cendio@lab-74 ~]$ tl-limit-printers --version
> 4.16.0post
After activating tl-limit-printers, our printer access control works as intended:
> [cendio@lab-74 ~]$ sudo ln -s /opt/thinlinc/sbin/tl-limit-printers \
>    /opt/thinlinc/etc/sessionstartup.d
> [cendio@lab-74 ~]$ sudo ln -s /opt/thinlinc/sbin/tl-limit-printers \
>    /opt/thinlinc/etc/sessionreconnect.d
Also verified the functionality according to our testbug (bug 7454).

Comment 80 Alexander Zeijlon 2024-06-17 10:28:24 CEST

> MUST:
> * Scripts should have the same or improved behaviour before and after the
>   option parser conversion (consider common and edge cases)
All tested scripts have the same or improved behavior. The new parser handles multiple prepetitions of the same option+arg more gracefully than the old parser. This improves the usability of our scripts.
> SHOULD:
> * There should be an autotest in place to ensure that all current and
>   future scripts use the correct option parser
We now have test_python_scripts_parser_import() which checks that we do not import Python's built-in parsers.
> * Man pages should exhibit layout consistency
All scripts now have both shortopt and longopt versions of their options, these can be seen both in manpages and via the --help option.
All scripts output the same version help message.
Overall --help and manpage consistency between scripts have been looked at during testing.