-
Notifications
You must be signed in to change notification settings - Fork 128
Fix #3600, warn about failed login due to third party cookies #3632
Conversation
@ianb @6a68 and i tried this and we don't think it's working as intended. Sometimes the error message shows and sometimes it doesnt. In all cases the App UI appears to be logged in. I think we should HIDE any message UI behind clicks on the |
@johngruen: oops, sorry – I set this so the error message always shows if you have third party cookies off, to make it easier to fix up the message box. I meant to mention that, but forgot. There's some FIXMEs to remove, I was going to wait to fix the rest of those up after the warning message itself is settled on. If you want a permanent hide option in there, then if there's a message/button to that effect I can wire it up in a later commit. I.e., once the message is in there and working I can do the rest to take this commit over the finish line. |
I was seeing the error message with third party cookies enabled, fwiw |
I've updated this so the error message simply is on all the time on every shot page. I'd like to get the design of the error message right; when everything is in place correctly the reproduction steps are complicated, which makes the design hard to test. |
This adds backupCookieRequest to the sitehelper login process, to tell the site if third party cookies SHOULD work. If the site sees that third party cookies might not be enabled, then it does a second check to GET /api/set-login-cookie?check=1. If that request shows the cookie isn't set, then it changes the model to warn the user.
Note that this is a real pain to test. To really test it we'd have to deploy to dev, then find or make an old version of the dev add-on. It would have to be an add-on built before #3581 landed (a4b6485 I guess) Maybe easiest is landing on dev once code review is done, then building such an add-on, and emailing QA with the xpi. |
@ianb let's talk about it in our meeting, i think i can match the UI changes in the feedback PR without needing this merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM, let's test this carefully on dev before releasing.
I do think we should document the auth flow carefully at this point. I could see it being confusing that req.deviceId
is both set (from a special first-party-like XHR instance, used to set props.isOwner
) and unset (from a third-party fetch
, used to set props.loginFailed
).
This adds backupCookieRequest to the sitehelper login process, to tell the site if third party cookies SHOULD work. If the site sees that third party cookies might not be enabled, then it does a second check to GET /api/check-login-cookie. If that request shows the cookie isn't set, then it changes the model to warn the user.
TODO: