Skip to content
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

CLI should check parameter name, and exit in error when encountering an unknown parameter #2046

Closed
oupala opened this issue Oct 14, 2018 · 19 comments

Comments

Projects
None yet
4 participants
@oupala
Copy link

commented Oct 14, 2018

When I enter an api password in the web interface, I get an error because of an error format. But there is no tip about a specific format.

When I set the api password with the cli, I get no error, only a success. With the same password. But in the end, the password does not work with EasyRSS.

What is the minimal size of the api password? If the password is shorter and is set via the cli, should it work or should it fail?

@Alkarex

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

The minimum size from the Web UI is 7 (same as the main password, as written), and no constraint from the CLI. But I do not think this is related to the password working or not with API clients.
You might have to quit EasyRSS and re-open it for trying another password.

@oupala

This comment has been minimized.

Copy link
Author

commented Oct 22, 2018

In the Web UI, it is not very clear to see that the api password is also constrained to 7 chars (as the main password.

@oupala

This comment has been minimized.

Copy link
Author

commented Oct 22, 2018

As far as I have tested, creating a user via CLI does not set the api password.

Here is my command line:

{{ _freshrss.install_path }}/cli/create-user.php --user {{ _freshrss.end_user.login }} --password '{{ _freshrss.end_user.password }}' --api-password '{{ _freshrss.end_user.password }}' --language {{ _freshrss.locale }} --email {{ _freshrss.end_user.email }} --token {{ _freshrss.token }}

As you can see, I'm trying to set the api password: --api-password '{{ _freshrss.end_user.password }}' (all vars are infered by ansible).

But in the end, the config file shows that no password is set:

  'passwordHash' => 'filled-with-a-hash',
  'apiPasswordHash' => '',

I can however set the api password by the following command line:

{{ _freshrss.install_path }}/cli/update-user.php --user myusername --api_password 'myapipassword'

So, as far as I understand, cli/update-user.php works while cli/create-user.php does not.

@Alkarex

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Please note that it is --api_password with an underscore, in both cases

@oupala

This comment has been minimized.

Copy link
Author

commented Oct 23, 2018

It works way better with the right parameter name. Thanks!

The strange thing is that hashes are different event if passwords are the same:

--password '{{ _freshrss.end_user.password }}' --api-password '{{ _freshrss.end_user.password }}'

By the way, this could be an opportunity to rename this issue and to improve the CLI robustness by checking the parameter's name and by exiting in error if one parameter is not recognized. This would be clearly better that silently ignoring a parameter that is wrongly spelled... Don't you think?

@Alkarex

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Yes, hashes are unique, because of a random salt.
Mja, we could put some warnings

@oupala oupala changed the title api password format CLI should check parameter name, and exit in error when encountering an unknown parameter Oct 23, 2018

aledeg added a commit to aledeg/FreshRSS that referenced this issue Mar 17, 2019

Add an option validation on cli commands
If an option used on cli is not recognized, the command
aborts and displays an error message.
If the typed option is similar to one of the recognized
options, a hint is displayed.

At the moment, there is a limitation on long options.
Short options are not validated at the moment.

See FreshRSS#2046

@Alkarex Alkarex added this to the 1.14.0 milestone Mar 17, 2019

aledeg added a commit to aledeg/FreshRSS that referenced this issue Mar 17, 2019

Add an option validation on cli commands
If an option used on cli is not recognized, the command
aborts and displays an error message.
If the typed option is similar to one of the recognized
options, a hint is displayed.

At the moment, there is a limitation on long options.
Short options are not validated at the moment.

See FreshRSS#2046
@aledeg

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@oupala Please review the related PR!

@oupala

This comment has been minimized.

Copy link
Author

commented Mar 18, 2019

I'm not sure to understand what pull request you are talking about. Could you please be more precise?

@aledeg

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@oupala

This comment has been minimized.

Copy link
Author

commented Mar 19, 2019

Ok I see. But I'm not involved in this pull request. I think you confused me with @Alkarex.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

@oupala Not involved, but it fixes the bug you found. :-)

@aledeg

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@oupala I was asking if the PR solves your issue or if there is still something missing.

aledeg added a commit to aledeg/FreshRSS that referenced this issue Mar 19, 2019

Add an option validation on cli commands
If an option used on cli is not recognized, the command
aborts and displays an error message.
If the typed option is similar to one of the recognized
options, a hint is displayed.

At the moment, there is a limitation on long options.
Short options are not validated at the moment.

See FreshRSS#2046

Alkarex added a commit that referenced this issue Mar 19, 2019

Add an option validation on cli commands (#2278)
If an option used on cli is not recognized, the command
aborts and displays an error message.
If the typed option is similar to one of the recognized
options, a hint is displayed.

At the moment, there is a limitation on long options.
Short options are not validated at the moment.

See #2046
@aledeg

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@oupala the fix is now in dev branch. Does this solve your issue?

@oupala

This comment has been minimized.

Copy link
Author

commented Mar 19, 2019

Ok, I get it now. Sorry for being so long to understand...

I'll test it as soon as possible. Thanks for the notifications!

@aledeg

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

No problem!

@oupala

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

As far as I have tested merge request #2278, it fixes this bug.

Thanks @aledeg for fixing this.

I still have 2 remarks:

  • it is very strange to get syntax inconsistency in parameter name: --api_password seems not natural as we usually use dash or underscore, but not both of them, we should convert all parameter to dash syntax: --api-password
  • it is also very strange to see that installing FreshRSS on an already installed FreshRSS returns an error, following idempotency we should more return a 0 return code:
>php cli/do-install.php --default_user myuser --language fr --api-enabled --db-type sqlite
FreshRSS looks to be already installed! Please use `./cli/reconfigure.php` instead.
>echo $?
1
@aledeg

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Ping @Alkarex

@Alkarex

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

The options name are with underscore, to match the values of https://github.com/FreshRSS/FreshRSS/blob/master/config.default.php and https://github.com/FreshRSS/FreshRSS/blob/master/config-user.default.php . It is not always super consistent.
db is special because it is to map an array of options (with the idea of possibly automating some mapping aspects, thanks to naming conventions).
I am not against changing, but then back-compatibility needs to be ensured somehow.

@Alkarex

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

P.S. Let's open another issue if desired, as the main one here seems to be fixed by @aledeg , and can be included in the upcoming FreshRSS 1.14.0

@Alkarex Alkarex closed this Mar 23, 2019

Alkarex added a commit that referenced this issue Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.