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

Single quotes #53

Merged
merged 7 commits into from
Mar 19, 2023
Merged

Single quotes #53

merged 7 commits into from
Mar 19, 2023

Conversation

Navid200
Copy link
Collaborator

@Navid200 Navid200 commented Mar 15, 2023

This PR mainly changes the behavior of the Install Nightscosut phase 2 script.

Currently, a fresh install uses double quotes in the nsconfig file.
After this PR, single quotes will be used in a new nsconfig file.

The same script also offers the user the option to change API_SECRET. It can be used by someone who has an existing system. In that case, the existing nsconfig file may include double quotes. Therefore, some of the added code was needed to allow the existing API_SECRET to be recovered regardless of is single quotes are used on double quotes in the nsconfig file.

This is the new menu that asks for API_SECRET:
Screenshot 2023-03-18 221623

This is what you will see if what you enter has less than 12 characters.
Screenshot 2023-03-15 150846

This is what you will see if what you enter contains one of the three unacceptable special characters.
Screenshot 2023-03-18 221905

@Navid200
Copy link
Collaborator Author

@Navid200 Navid200 requested a review from tzachi-dar March 15, 2023 18:42

for j in {1..1000}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happened to this part?

Copy link
Collaborator Author

@Navid200 Navid200 Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now using a dialog for the API_SECRET.

I understood that was needed when using a text prompt for asking for the API_SECRET.

Did I misunderstand?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when someone presses a lot of chars before reaching this stage. Are they being removed?

Copy link
Collaborator Author

@Navid200 Navid200 Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a maximum for the number of characters anywhere in any of the dialogs.

Do we need to specify a maximum?
I can add that to the prompt asking user to enter an API_SECRET that has 12-24 characters for example.

Should I do that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is not with the length of the API_SECRET but rather with what happens when the scripts are running and someone presses a few keys. Will this keys be part of the dialog, or are they removed automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dialog will take all the keys as intended characters until the user presses enter. Then, all the characters enterd will be accepted as the password.

Copy link
Collaborator Author

@Navid200 Navid200 Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where that loop is right now before this PR:
Screenshot 2023-03-18 204532

It's after the prompt asking for the password is shown on screen not before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the PR that put that loop there: #24

So, it's my fault. I put it in the wrong place.
The loops should be there when a script is running taking a very long time. Then, if the user presses a button by mistake, we need to make sure what comes next will not be broken by those keys.

But, where I have inserted the loop is wrong.

NS_Install2.sh Outdated
Value=$(dialog --colors --ok-label "Submit" --form " \Zr Developed by the xDrip team \Zn\n\n\n\
Your current API_SECRET is $cs\n\n\
You can press escape to maintain the existing one. Or, enter a new one with at least 12 characters excluding the following.\n\n\
$ \" \\\n " 19 50 0 "API_SECRET:" 1 1 "$secr" 1 14 25 0 2>&1 1>&3)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when using single quotes the following are allowed: $ " \ \n the only one that is not allowed is ' it self.

Copy link
Collaborator Author

@Navid200 Navid200 Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought Nightscout itself had difficulty with $.
Even in Heroku, I thought $ was unacceptable.

Copy link
Collaborator Author

@Navid200 Navid200 Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we still have systems that have " in their nsconfig, how about this for the exceptions?

$ ' "

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also be OK. The most important part is to also remove the ' from the text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what you mean by that.

@Navid200
Copy link
Collaborator Author

@tzachi-dar I ran a test including \ in the API_SECRET and it failed. I only tested that once. I cannot be 100% sure that was the reason. But, I left \ in as an excluded character.
And, I added ' to the excluded list.

I have updated the images above to reflect the updated PR.

The loop is gone because where it is placed in the existing code is useless.

@Navid200 Navid200 requested a review from tzachi-dar March 19, 2023 16:11
@Navid200 Navid200 merged commit 1949c3c into jamorham:vps-dev Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants