-
Notifications
You must be signed in to change notification settings - Fork 90
Refresh token using ASP cookies #145
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
Conversation
Code that enables SMS and Email verification, tested with one of Exelon's subsidiaries
Merged some more similar pieces of code together and separated out Text vs Phone as Call will likely never be an option we will support.
for more information, see https://pre-commit.ci
Coincidentally site went down during testing and revealed that it raises a different kind of error than invalid auth. This handles that logic since we rely on the redirect.
Thanks to trickv's diagnoses we know there are two forms of MFA in Exelon's codebase. I was able to locate an account that does not have an associated phone number and verify the logic flow between what I am calling "forced" and "opted" MFA. Users who do not provide a phone are forced into MFA (AFAIK there is no non-MFA version anymore). Users who add a phone, will forever be stuck as opted in. This logic adds email verification through both flows.
First pass to try to store the ASP session and cookies which can be used to acquire a new token.
|
Switching it over to review. @tronikos with new login data that gets created during MFA, what's the recommendation for folks to repopulate it? Should they reconfigure the integration (delete and add it again) or can we catch the 401 and force a new MFA? |
The expectation is that the login function will raise invalid auth which will trigger reauth in HA. The expectation is that the login function on success always returns a valid opower access token. The simplest solution is likely to raise invalid auth from your function rather than changing other parts of the code base. You could either detect the problematic previous login data or maybe check whether the access token you are about to return hasn't expired. Also in your browser > developer tools > application > cookies you should be able to see the expiration for these cookies. That will tell you how long before you will be asked for MFA again. |
|
You have to resolve some conflicts cause by #148 How is your testing going? Is MFA now working well? Any change you could resolve this soon so that I can merge and make a release to bump the version in HA before the release on Wednesday? |
|
Adding @PaulSD here in case you could work together to resolve this. |
|
Yeah, I can test this and let you know how it goes. |
|
Happy to test Pepco (Exelon company) which still throws premise info errors. I had completely removed the integration, readded it, and logged back in succesfully with the MFA and then the PremiseInfo errors start:
|
|
@censay |
|
@PaulSD fairly vanilla integration of HA on a proxmox mini PC. So theoretically can change anything. Would probably take an image before adjusting anything. |
If token returns nothing, a reauthentication is needed because the original authentication flow broke. We have to raise an error to support this flow.
for more information, see https://pre-commit.ci
|
The cookies are set to expire at the end of the session (the OpenID ones expire in the past). I was hoping with this change I could keep the session alive for more than the 30 minutes, but alas it did not work a week or even a few hours later of not touching the backend. Unfortunately if the backend can choose to expire the tokens anytime (as we even see in the frontend), that means we need to support re-authentication frequently. Right now I delete/add the integration and get a backfill, so there is an annoying path forward. I need to spend more time to find a way to keep the session alive, which means some more reverse engineering. This patch at least should support the reauth process. |
Thanks for the quick reply. Maybe I'm unclear on the workflow but it looked like the PremiseInfo fix was pulled into main meaning it should work right? I get a successful provision after MFA but then premise info starts. |
|
To apply changes before they are released:
|
|
So far I haven't found a way to create a longer lived token without a forced MFA. This remains to be the case with the website as well, it will timeout and then start the authentication flow at the MFA part. This pull request adds one main feature, it will detect the lack of token and issue an invalid auth. It further adds cookie storage, so when we do uncover how to make it last longer the logic is there (current cookie logic lasts a session which is somewhere around 30 minutes). It may be worth pulling this in, and then opening an issue to find a way to create longer lived ASP cookies. |
|
Is this ready to be merged? Does the title need to be updated? Could you also update the HA docs page? |
|
Title and docs are accurate. The docs claim periodically, in this case it happens to be a little too periodic. I would say we can merge it (PR was for ASP cookies and that is what is here), because it will at least run the InvalidAuth part and instead of add/removing the integration you just reauth. Though I am using the ASP cookies, it still will not last beyond a session. I would propose we merge this and open (assigning me) an issue that the token does not last longer than a session. I am not sure if or when I can fix it, but I will keep an eye out and periodically try to improve it. |
|
I've done some review and testing here. Unfortunately I don't see any easy solutions. However, I can at least provide some more details on the behavior and potential solutions for further exploration. At a high level:
At a lower level:
Potential solutions:
|
This is a draft pull request as I need time to test this out further to make sure that it still works well after a few hours/days. The logic here assumes the cookies set by ASP (which I verified were the minimum subset) are enough to login without needing to trigger the MFA again.
Some more testing remain: