-
Notifications
You must be signed in to change notification settings - Fork 1
Mysky autologin stored settings and logout #188
Conversation
scripts/ui.ts
Outdated
@@ -117,6 +115,9 @@ async function requestLoginAccess(permissions: Permission[]): Promise<[boolean, | |||
// Save the seed and email in local storage. | |||
saveSeedAndEmail(seed, email); | |||
|
|||
// Wait for Main MySky to login successfully. | |||
await waitForMySkyPortalLogin(); |
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.
nit
looks kind of weird to be naming an async
function waitFor
because you don't necessarily await it
scripts/ui.ts
Outdated
* Waits for portal login on Main MySky to complete. | ||
*/ | ||
async function waitForMySkyPortalLogin(): Promise<void> { | ||
return new Promise((resolve, reject) => |
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.
this might block forever, that's ok right? would've expected to see a timeout mechanism of sorts here
src/mysky.ts
Outdated
JsonData, | ||
deriveEncryptedFileKeyEntropy, | ||
EncryptedJSONResponse, | ||
MAX_REVISION, |
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.
what are all these GA failures about? (Module '"skynet-js"' has no exported member 'EncryptedJSONResponse'.
)
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.
You need to link to the dependency PR mentioned in the description.
const mySky = new MySky(initialClient, mySkyDomain, referrerDomain, permissionsProvider, preferredPortal); | ||
|
||
// Login to portal. | ||
{ |
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.
this is weird? what are these curly braces all about?
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.
I thought it made it more clear that the comment applies to the whole scope.
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.
hm ok, kind of implies it should/could be a separate function but equally fine leaving this in I guess 🤷♂️
|
||
const methods = { | ||
checkLogin: this.checkLogin.bind(this), | ||
getEncryptedFileSeed: this.getEncryptedPathSeed.bind(this), |
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.
oof! should be covered with a unit test
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.
Added a comment.
src/mysky.ts
Outdated
return { data: json }; | ||
} | ||
|
||
// TODO |
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.
probably best to describe what's left to be done
src/mysky.ts
Outdated
const dataKey = deriveEncryptedFileTweak(pathSeed); | ||
|
||
// Immediately fail if the mutex is not available. | ||
return await this.client.db.revisionNumberCache.withCachedEntryLock( |
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.
it says revisionNumberCache
is not defined? definitely issue with the typing here
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.
This PR assumes the SkyDB rework has been merged in. The dependency PR is built on top of the latest changes in skynet-js.
* @param seed - The user seed. | ||
* @param email - The user email. | ||
*/ | ||
protected setupAutoRelogin(seed: Uint8Array, email: string): void { |
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.
Not 100% sure I'm following here but this is a foot gun, or at least it looks like one.
Why not do this through some configuration variable or setting?
The implementation of executeRequest
should simply have the error handling at all times and do:
if res.status == 401 && autoReloginEnabled {
// login
// retry
}
...
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.
I tried moving this logic into executeRequest
as you suggested and it didn't seem like an improvement. If anything it just spread the logic out over two projects and coupled skynet-js
even more closely with MySky. The way it is, the logic is confined to one place, and though it is slightly confusing I've seen much more confusing hook/callback type logic in Javascript...
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.
I was debugging this code and I realized that your suggestion would actually simplify it (as well as fix my bug -- think I was missing a .bind(this)
somewhere but that's not needed if the logic is already in client.executeRequest
.) I pushed the change in the other PR. I called it loginFn
though to hopefully not couple this too closely with "MySky auto-relogin".
src/mysky.ts
Outdated
this.client = getLoginClient(seed, null); | ||
|
||
// Login to portal. | ||
{ |
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.
is this some kind of scope magic?
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.
Yes, it's a block scope.
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.
Was this confusing to you? I googled to see if it was standard and some people on StackOverflow were confused by it. Not sure how seriously to take that, though, because people on the internet have different experience levels. To my eyes this looks pretty clear. I wanted to avoid extracting this into a new function because having a lot of nested functions can also be hard to follow.
src/mysky.ts
Outdated
isEmailProvided = storedEmail !== null; | ||
} | ||
|
||
if (storedEmail) { |
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.
isEmailProvided
is weird I think, I think I understand what you are doing but I would do this:
const storedEmail = email || checkStoredEmail();
if (storedEmail) {
...
// If the email was found in local storage, save it in the user settings.
if (!email) {
await this.setUserSettings(seed, { portal: preferredPortal, email: storedEmail });
}
}
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.
this saves you a bunch of lines and a boolean, in any case I would rename isEmailProvided
to something that explains why it will be used, or update the user settings inside of the if (!storedEmail) {
, both would read easier
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.
Renamed the variable and added an explanatory comment.
- [X] Fix domain extraction when on specific portal server - [X] Fix domain extraction for referrers - [X] Fix email not being saved in user settings (domain bug)
- [X] Fix portal account endpoints broken - [X] Fix bug with getting user settings (bad URL) - [X] Use `ensureUrl` when initializing client?
This is not exactly a code problem but we need to stick to some naming convention. Right now we have a bunch of kebab-case and a bunch of snake_case filenames, sometimes in the same directory. |
…and-logout' into mysky-autologin-stored-settings-and-logout
"crypto-browserify": "^3.12.0", | ||
"idb-keyval": "^6.1.0", | ||
"mustache": "^4.2.0", | ||
"post-me": "^0.4.5", | ||
"punycode": "^2.1.1", | ||
"randombytes": "^2.1.0", | ||
"skynet-js": "^4.0.20-beta", | ||
"skynet-js": "^4.0.22-beta", |
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.
We should update this to the latest version and proactively deal with any breaking changes.
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.
This doesn't matter for now because I've linked to a specific branch in skynet-js
locally. Changing this line wouldn't affect anything, I just have to keep the skynet-js
branch up-to-date with recent changes. I will change this line once the skynet-js
branch has been merged and a version change published.
* | ||
* @returns - An empty promise. | ||
*/ | ||
async function resolveOnMySkyPortalLogin(): Promise<void> { |
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.
This one took me a while to get it.
If I understand it correctly, what's happening is:
- we register an event listener on
storage
which will resolve only if thestorage
action is called with keyPORTAL_LOGIN_COMPLETE_SENTINEL_KEY
and value1
- we also register a timeout that will
reject
the promise race afterMYSKY_PORTAL_LOGIN_TIMEOUT
if there hasn't been a valid login until then
So, the basic idea is that we wait for a login and if it doesn't happen withing a timeout, we reject.
If that's what's going on then it's fine. It's a little more complicated than I'd ideally like but I understand that the inter-process communication here is not exactly simple.
Questions:
-
Can we add a few more comments, outlining the ideas here? That actual registrations of listeners and so on are more than clear but the way the parts fit together in the big picture could be spelled out a bit more.
-
Having in mind that any JS on this page can write to the storage (please correct me if I'm wrong, my front end knowledge is not amazing), are we risking any security holes with this? Or is it that if there is malicious JS that can get to localStorage then we're dead anyway?
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.
I will happily add more clarifying comments, thank you for the suggestion!
Regarding question 2: local storage is sandboxed per domain, so badapp.hns.siasky.net
can't write to MySky's local storage. And MySky should only ever run its own code. If this invariant is ever broken, then yes: we're dead.
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 for explaining! One day I'll read all those fascinating things about browsers - storage types, workers, all of it. Until then I'll keep asking silly questions. ;)
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. All we need to do, from my perspective, is merge the skynet-js's new version and update package.json here. I'm holding off the approval until then but the code looks sound.
@@ -111,7 +111,7 @@ <h2 class="mt-6 text-center text-3xl font-extrabold text-gray-900"> | |||
<!-- Signup form --> | |||
<div class="mt-8 sm:mx-auto sm:w-full sm:max-w-md"> | |||
<div class="bg-white py-8 px-4 shadow sm:rounded-lg sm:px-10"> | |||
<form class="space-y-6" action="#" onsubmit="window.signUp()"> | |||
<form class="space-y-6" action="#" onsubmit="window.signUp(event)"> |
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.
nit
I think usually you want to call prevent default on the event, but you also want to return false here. I don't know why though unfortunately, I just know onsubmit="whatever(); return false"
or onsubmit="return myValidator()"
is a thing. Decided to leave the comment since it might be useful to you but feel free to just resolve if we don't care.
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.
That sounds familiar to me, though I unfortunately don't understand what that does either. However,
- I know that everything works the way it is now,
- I would have to do all the deploys and testing again after making this change, and this was pretty annoying to test.
To me it makes sense to move this to a followup. Maybe at some point I can do a blitz through Issues with Karol or Farzad.
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.
const promise1 = new Promise<void>((resolve, reject) => { | ||
const handleEvent = async ({ key, newValue }: StorageEvent) => { | ||
if (key !== PORTAL_LOGIN_COMPLETE_SENTINEL_KEY) { | ||
return; |
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.
so this makes it so promise1
never resolves, are we good with that?
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.
Yeah, we only want the promise to resolve when the right storage key is encountered. Any other storage key shouldn't trigger a resolve
.
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.
Should we reject
here, then?
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.
No! 😅 We don't want login to fail if a different key is set for whatever reason. If there is another storage event listener for that key, it will also be triggered. Here, we are only interested in the specific key we are listening for.
} | ||
if (!newValue) { | ||
// Key was removed. | ||
return; |
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.
same here
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.
Added a comment.
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.
I'm not fully up to speed with the feature, but from a pure code standpoint this PR looks solid and I don't have any blocking comments, LGTM 👍
PULL REQUEST
Overview
Portal refresh flow
TODO
These TODOs are also documented in comments in the code.
Persisting data across portals
Testing
Of course this needs to be tested as well.
SDK PR
See SkynetLabs/skynet-js#361, which implements the page refresh in the SDK. These must be tested in tandem.