Fix/add device key to auth params if set#306
Open
NodeJSmith wants to merge 8 commits intoNabuCasa:masterfrom
Open
Fix/add device key to auth params if set#306NodeJSmith wants to merge 8 commits intoNabuCasa:masterfrom
NodeJSmith wants to merge 8 commits intoNabuCasa:masterfrom
Conversation
…ce and setting DEVICE_KEY in auth params
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for device authentication metadata throughout the library, ensuring that device keys and related parameters are properly accepted, sent in authentication requests, and handled on responses.
- Extended constructors in both
RequestsSrpAuth(utils.py) andCognito(__init__.py) to acceptdevice_key,device_group_key,device_password, anddevice_name. - Updated
authenticateandrenew_access_tokeninCognitoto includeDEVICE_KEYin auth parameters when present. - Handled
NewDeviceMetadatain both_set_tokensand post-auth flows to confirm devices and store returned metadata.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pycognito/utils.py | Added device metadata parameters to RequestsSrpAuth init and AWSSRP call |
| pycognito/init.py | Updated Cognito init, authenticate, renew_access_token, and _set_tokens to handle device metadata |
Comments suppressed due to low confidence (2)
pycognito/init.py:732
- Consider adding unit tests for the refresh flow to verify that
DEVICE_KEYis included inAuthParameterswhendevice_keyis set, and that no device confirmation is triggered whendevice_keyis absent.
if self.device_key is not None:
pycognito/init.py:732
- [nitpick] The logic to inject
DEVICE_KEYinto auth parameters is duplicated betweenauthenticateandrenew_access_token. Extract this into a shared helper to reduce duplication and improve maintainability.
if self.device_key is not None:
…e error and wrong check
Author
|
@ludeeus Would you be the right person to ping for a review of this PR? No rush, but want to make sure someone is aware of it. |
Member
|
I have asked around internally, will keep you posted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While
AWSSRPwas updated in a previous PR to handle device authentication support, theCognitoclass and theRequestsSrpAuthclass were not. This left pycognito stating that it supported this while being unable to deliver on that support.This PR fixes this:
DEVICE_KEYto the auth params during authentication, ifself.device_keyis setCognitoinstance whenNewDeviceMetadatakey is included in authentication responseCloses #133