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

feat: Persist OTPs in keystore and prevent reuse of OTPs #1609

Merged
merged 9 commits into from
Oct 18, 2023

Conversation

sitaram-kalluri
Copy link
Member

@sitaram-kalluri sitaram-kalluri commented Oct 12, 2023

- What I did

  • Store OTPs in the Hive keystore along with a "TTL" (Time To Live) value, representing the OTP's expiration time.
  • The OTPs are stored in the format - private:< OTP >atsign. The "private:" ensure that keys are not synced to the client and keys are not returned in the scan verb.
  • By default, the "TTL" is set to 5 minutes but can be overridden by the client by specifying the expiry time in milliseconds when using the "OTP" verb.
  • In abstract_verb_handler.dart, "isOTPValid" function which verifies if the OTP is valid and removes the OTP from the keystore to prevent reuse. The otp_verb_handler and enroll_verb_handler will invoke the function.
  • Replace the use deprecated constants with "AtConstants"

- How to verify it

  • Following are the unit tests:

    • A test to verify otp:get with TTL set is active before TTL is met
    • A test to verify otp:get with TTL set expires after the TTL is met
    • A test to verify "isOTPValid" returns false if OTP is not available in the keystore.
    • A test to verify otp:validate invalidates an OTP after it is used
  • Following are the functional tests:

    • A test to generate OTP and returns valid before OTP is not expired
    • A test to generate OTP and returns invalid when TTL is met
    • Because the OTPs cannot be reused, updated the functional tests in enroll_verb_test.dart to fetch otp for each request

- Description for the changelog

  • Enable client to set the OTP expiry duration.

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

I don't like the idea of verb handlers using internal functionality of other verb handlers. Is there a better / cleaner way of making that validation functionality more explicitly available?

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 14, 2023 07:08
@sitaram-kalluri sitaram-kalluri requested a review from gkc October 17, 2023 08:51
git:
url: https://github.com/atsign-foundation/at_libraries.git
path: packages/at_commons
ref: expire_otp_changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I've merged the at_commons PR so please publish at_commons 3.0.57 and remove this dependency override

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure Gary, Thank you.

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 17, 2023 16:02
@sitaram-kalluri
Copy link
Member Author

Updated the at_commons version to 3.0.57.


OtpVerbHandler(SecondaryKeyStore keyStore) : super(keyStore);

@override
bool accept(String command) =>
command == 'otp:get' || command.startsWith('otp:validate');
bool accept(String command) => command.startsWith('otp');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be command.startsWith('otp:get')

Copy link
Contributor

Choose a reason for hiding this comment

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

or command == 'otp:get'

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 18, 2023 05:19
@sitaram-kalluri sitaram-kalluri merged commit 28a7edf into trunk Oct 18, 2023
17 of 18 checks passed
@sitaram-kalluri sitaram-kalluri deleted the expire_otp_changes branch October 18, 2023 11:29
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.

Allow client to configure the expiry on the OTP
3 participants