-
Notifications
You must be signed in to change notification settings - Fork 1
Connect extension with logged in Credentials #9
Conversation
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.
Looking good too. Let me know when this PR is ready for review and I'll give it the 👍
@@ -8,6 +6,16 @@ addEventListener("install", () => { | |||
}); | |||
}); | |||
|
|||
// TODO The same login URL should be the cookie URL | |||
// TODO2 What should we do if user is not logged in? |
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.
If the user is not logged in, you should send them back with an error message. I'll our the designer for mocks
], | ||
"host_permissions": ["<all_urls>"], | ||
"host_permissions": ["https://*/*", "<all_urls>"], |
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.
🙏 thanks, this permission stuff always confuses me
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. Re the cookie: we should ask @Luigi-Sirius what he suggests to secure that token :)
const sendCode = (email: string, code: string) => apiCall('auth/email/complete', { email, code }) | ||
|
||
// TODO see ways to make more secure this cookie... |
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.
@Luigi-Sirius this is your realm, any thoughts here?
Before merging this, the following branches should get merged first:
#6
#7
Both branches are inside this one.