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

Added 2FA, fix bugs, auto format file #37

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

LoSunny
Copy link

@LoSunny LoSunny commented Apr 23, 2021

The access token will be stored in the CONFIGURATION file, such that the token can be reused and doesn't need to be updated each time.

@LoSunny
Copy link
Author

LoSunny commented Apr 23, 2021

@lwitzani
Copy link
Owner

Hey,
Thanks for that. Will take a look at it and probably merge it 👍🏻

@lwitzani
Copy link
Owner

I looked at the code and run it on my phone. I don't quite get how this will work indefinitely?
After your first successful authentication with an OTP the token is now saved in the configuration.
Now in the getAuthToken() function you first check if the saved token is still valid (and if yes it is used). At the time when it is not valid anymore, the authentication without any credentials is checked and if that does not lead to a new token then the saved credentials are again used (like the very first time). But now the OTP is maybe not valid anymore so why would a request now lead to a new auth token?

I always thought that for 2FA support, i would need some logic in the code which calculates the current valid OTP based on the shared secret.

Thanks for explaining

@LoSunny
Copy link
Author

LoSunny commented Apr 24, 2021

I looked at the code and run it on my phone. I don't quite get how this will work indefinitely?
This will not work indefinitely, once the access token expires, you'll need to re-enter the 2FA code and run the script again

But now the OTP is maybe not valid anymore so why would a request now lead to a new auth token?
Because we still need to take care of those who didn't use 2FA to generate a new access token base on their username and password.

And for those who have 2FA enabled, this message popup
image
And they need to re-enter the 2FA code and run the script again

--EDIT--
To prevent the access token from expiring, we can increase the time for the session in the config.json of homebridge-config-ui-x as state in https://github.com/oznu/homebridge-config-ui-x/wiki/Config-Options#sessiontimeout
The maximum time allowed is 86400000 seconds (source)

@lwitzani
Copy link
Owner

okay setting the session timeout to 86400000s seems to be a reasonable solution ;) would just have to add this to the readme then which would be no problem.

I noticed the problem that you cannot run the script inside the Scriptable app twice in a row when overwritePersistedConfig is true...this is probably not a good idea to have since playing around with e.g. the color settings (change color, run script, change color, run script, ...) would be a huge pain in the ass :) ...

@LoSunny
Copy link
Author

LoSunny commented Apr 24, 2021

Maybe we should add to the docs?
change back overwritePersistedConfig to false once you finish running the script to save the config

@lwitzani
Copy link
Owner

I know that it works after that. But the User will be confused...
The way it is in your branch -> it prevents playing around with settings

@LoSunny
Copy link
Author

LoSunny commented Apr 24, 2021

I'm actually debugging out the problem. It seems that Scriptable has some bugs when it goes to Reading files from cache, Reading files from disk, Saving files to disk.

What am I trying to do

  • As the CONFIGURATION won't be save until the code is finish
  • I want to read the config.json again during the getAuthToken
  • If the configTmp has the accessToken, use it

But I've face some problems and I've pushed a commit to my dev branch. Here are the ways to reproduce the error

  1. Make sure there is a config.json file
  2. Run the script with overwritePersistedConfig true and a valid otp
  3. In the logs, you'll see the word Saving, and then the file content to be saved, which includes the accessToken
  4. You can double-check the config.json content in the Files app
  5. Re-run the script, which should have the word Read config, and Returning result, the line after Returning result won't have the accessToken
  6. But there isn't any "Save to disk" action before the "Read config" action? So why did it disappears

@LoSunny
Copy link
Author

LoSunny commented Jun 3, 2021

I've re-coded the whole 2FA run down, which now can fetch the 2FA code from Authy by my new script
(Sorry for those who use Google Authenticator or others)
The variable twoFactorAuthentication is to see if need to enable 2FA.

The function is to check if they use the parameter method and enable 2FA, which is impossible as need to use the access token. The reason we need to use an access token instead of re-auth(ing) each time is that a 2FA token can be only used once. If you try to re-auth with the same 2FA code, it will throw an error and you need to wait for 30 seconds for a new 2FA code

if (twoFactorAuthentication && foundCredentialsInParameter) {
  throw ('You cannot use this method if you want to enable 2FA');
}

If the user disables usePersistedConfiguration, it will only fetch the access token from the config and uses the configuration from the script

if (!usePersistedConfiguration && twoFactorAuthentication) {
  CONFIGURATION.access_token = (await getPersistedObject(fm, pathToConfig, CONFIGURATION_JSON_VERSION, CONFIGURATION, false)).access_token;
  log('Loaded access_token from config');
}

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