Swap behavior out for exelon mobile apps. #151
Conversation
We need to support legacy clients being upgraded and ensure that the MFA challenge is presented when there is a problem with the refresh token. The only system would provide an invalid auth, but not trigger the MFA. This was tested by corrupting login_data with a bad token.
|
@tronikos providing an InvalidAuth flow does not trigger an MFA in the current home assistant opower integration. This means you get caught in a loop where it tries to reuse the login data which is what would cause the InvalidAuth to begin with. In this PR I've also fixed it so instead of InvalidAuth it triggers an MfaChallenge when there is an authentication issue, which I think would resolve the issue in the latest home assistant. I would propose that if there was an InvalidAuth we might want to consider the login_data to be bad and not re-use it for a subsequent login. I think your pge integration might not have the same issue since it might respond with an MFA challenge instead of an invalid authentication. However, if the user was to change the password it might follow a different flow since there would be invalid cookies not null. The obvious workaround is simply to delete the integration and restart. |
Yes this is WAI. You need to raise MfaChallenge to trigger MFA. If you raise InvalidAuth it will ask you to enter your username and password. |
PaulSD
left a comment
PaulSD
left a comment
There was a problem hiding this comment.
Based on a manual code review: Overall looks good. Just some minor comments/suggestions.
| f"https://{self._base_url}/oauth2/v2.0/token", | ||
| data={ | ||
| "grant_type": "authorization_code", | ||
| "scope": "openid offline_access " + self._client_id, |
There was a problem hiding this comment.
Does the mobile app include client_id in the scope? That doesn't look right to me. I think it should just be "openid offline_access".
There was a problem hiding this comment.
Yes, the mobile app is passing the client id into the scope for some reason. When I mess with the fields on how they ought to be, it seems to have broken. I was testing a few things at once, but I noted the importance of client_id and redirect_uri pairing. When you remove this piece from authorization_code it does not work.
| data={ | ||
| "grant_type": "refresh_token", | ||
| "response_type": "token", | ||
| "scope": "openid offline_access " + self._client_id, |
There was a problem hiding this comment.
Does the mobile app include client_id in the scope? That doesn't look right to me. I think it should just be "openid offline_access".
There was a problem hiding this comment.
Yes, the mobile app is passing the client id into the scope here too. However, testing confirms we do not need it. I am however leaving it in because that is how at least one of the mobile apps behave.
| account for account in result_json.get("data", {}) if account.get("status", "") == "Active" | ||
| ] | ||
|
|
||
| if len(active_accounts) == 0: |
There was a problem hiding this comment.
Might also be helpful to log a message if there are multiple accounts:
if len(active_accounts) > 1:
_LOGGER.info("Found multiple active accounts, using {active_accounts[0].get("accountNumber", "")}")
There was a problem hiding this comment.
Sure, extra debug logging does not hurt to have.
src/opower/utilities/exelon.py
Outdated
| account = active_accounts[0] | ||
| account_number = account["accountNumber"] | ||
| # set the first active one | ||
| return active_accounts[0] |
There was a problem hiding this comment.
Having this return outside of the try is a little weird and hard to follow. Would probably be easier to follow if it was inside the try block.
There was a problem hiding this comment.
Agreed, and implemented.
src/opower/utilities/exelon.py
Outdated
| # Get the account type & state | ||
|
|
||
| isResidential = account["isResidential"] | ||
| is_residential = account["isResidential"] |
There was a problem hiding this comment.
Should use account.get("isResidential", false), to avoid failures if the isResidential field is removed or renamed.
There was a problem hiding this comment.
Agreed, I was cleaning this code up but did not have a good way to test. Fixed in the latest commit.
src/opower/utilities/exelon.py
Outdated
|
|
||
| isResidential = account["isResidential"] | ||
| is_residential = account["isResidential"] | ||
| state = account["PremiseInfo"][0]["mainAddress"]["townDetail"]["stateOrProvince"] |
There was a problem hiding this comment.
To avoid failures if these fields are removed/renamed/restructured/etc:
try:
state = result_json["PremiseInfo"][0]["mainAddress"]["townDetail"]["stateOrProvince"]
except (KeyError, IndexError):
state = None
src/opower/utilities/exelon.py
Outdated
| # Determine subdomain to use by matching logic found in https://cls.login_domain()/dist/app.js | ||
| Exelon._subdomain = cls.primary_subdomain() | ||
| if not isResidential or state != "MD": | ||
| if not is_residential or state != "MD": |
There was a problem hiding this comment.
This might be a little easier to read?
if not (is_residential and state == "MD"):
Minor adjustments to the style to make it easier for others to follow based on the comments in the pull request.
|
Thanks for the review @PaulSD @tronikos we have multiple subsidiaries tested and confirmed, after several hours the token is holding as well so I have good confidence this will last at least longer than what is there. This seems good enough to start the formal review process/submit so it is ready by next release. I will keep testing through the weekend just in case but we can fix anything that comes up with a followup. |
3 participants
This change switches out the existing exelon structure from website, where we get short lived tokens, to a mobile app where we get refresh tokens. This was tested on a single subsidiary, so we should verify it across a few to make sure before submitting this change. In theory this would create an indefinite access since we would be performing refreshes on the refresh token daily.
This resolves #150