Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement saving and loading session info from local storage #85

Open
brunomoutinho opened this issue Jun 8, 2023 · 37 comments
Open

Implement saving and loading session info from local storage #85

brunomoutinho opened this issue Jun 8, 2023 · 37 comments
Assignees
@brunomoutinho @seanaguinaga
Labels
enhancement New feature or request

Comments

@brunomoutinho
Copy link
Collaborator

brunomoutinho commented Jun 8, 2023 •
edited
Loading

https://magic.link/docs/connect/wallet-api-reference/javascript-client-sdk
@brunomoutinho brunomoutinho self-assigned this Jun 8, 2023
@brunomoutinho brunomoutinho added the enhancement New feature or request label Jun 8, 2023
@brunomoutinho
Copy link
Collaborator Author

@seanaguinaga this should not be too big to work on :)

@seanaguinaga
Copy link
Collaborator

I understand saving the token to local storage, however I am not sure where or for what you would use the token for? I don't see anywhere that you are explicitly needing the idToken to make requests?

const idToken = await magic.user.getIdToken();
localStorage.setItem('sessionToken', idToken);

const userData = {
  ...user,
  id,
  walletAddress,
  email,
  isSignedIn: true,
  sessionToken: idToken, // Add this line
};

Do you mean something like this?

@brunomoutinho
Copy link
Collaborator Author

This is not meant for being able to make requests to the API. The reasoning behind it is that we use Magic to sign in to the application, and Magic maintains the signed status if the user closes and opens the application again. We need to make sure we keep our applications (and anyone's application that uses Connect SDK) and Magic synced.

The SDK must, then, expose methods/data that a application that uses it can call to validate if the user is signed in or not.

Part of the task is to identify in Magic Documentation how to keep this data up to date.

@seanaguinaga
Copy link
Collaborator

Interesting

I think I understand

https://magic.link/docs/auth/api-reference/client-side-sdks/web#isloggedin

magic.user.isLoggedIn() could be used

@seanaguinaga
Copy link
Collaborator

https://magic.link/docs/auth/more/customization/session-management

users are automatically logged out after 7 days with no option to refresh auth state without upgrading to premium?

Is there a specific user experience scenario that prompted this issue @brunomoutinho?

I assume that maybe after 7 days, the user was still showing as logged in and auth issues were occurring? Or they showed as signed in when they in fact were not?

@brunomoutinho
Copy link
Collaborator Author

So far there was no control on Connect to keep the session. So imagine an application (like Greenruhm Web) where we require the user to be signed in to access any pages other than sign in/up. When the user closed the application and opened again, there would be no indication that the user is, in fact, signed in through magic and we would redirect the user to the sign in page, only for Magic to tell them that they're already signed in.

By implementing this feature, Greenruhm Web can check in Connect's SDK whether or not the user is signed in and redirect them to sign in only when they're not 😄

@seanaguinaga
Copy link
Collaborator

Do I need access to the greenruhm-web repo?

@seanaguinaga
Copy link
Collaborator

Error is still happening even using the vanilla npm pack and tar.gz method

Screenshot 2023-08-24 at 16 07 31

@brunomoutinho
Copy link
Collaborator Author

@seanaguinaga Make sure you have committed and pushed the changes. Tomorrow I can checkout your branch and investigate.

@seanaguinaga
Copy link
Collaborator

I deleted node_modules in both projects

Reinstalled

And rebuilt as well

@seanaguinaga
Copy link
Collaborator

branch: enhancement-localStorage-AuthPersistence

@seanaguinaga
Copy link
Collaborator

@brunomoutinho any luck? I am completely stumped 🤔

@brunomoutinho
Copy link
Collaborator Author

@seanaguinaga would you try to run greenruhm-web with the deprecate-magic-link branch? I just noticed it has not been merged to master and this might be an issue.

@seanaguinaga
Copy link
Collaborator

Yes! that does not have that error, do you know why?

@brunomoutinho
Copy link
Collaborator Author

brunomoutinho commented Aug 29, 2023 via email

@seanaguinaga
Copy link
Collaborator

Interesting!

I will!

now this....

Screenshot 2023-08-29 at 14 46 17

How can I point that url at localhost?

Do you guys use .env for this repo too? I could not find any details in the readme

@brunomoutinho
Copy link
Collaborator Author

brunomoutinho commented Aug 29, 2023 via email

@brunomoutinho
Copy link
Collaborator Author

brunomoutinho commented Aug 29, 2023 via email

@seanaguinaga
Copy link
Collaborator

Got it

Looks like you are using vercel for env vars (very cool)

However, it requires me to have a seat

At this point, I am not sure if I should try to e2e test this or if you should?

PS - I still don't see anywhere in the code where it would know how to call to greenroom.com/api and not localhost. This app is not using env vars for it's urls from what I can see?

@seanaguinaga
Copy link
Collaborator

Alright we are getting closer!

From what I can see, this API endpoint is already wrapped with CORS already handled? (circled in green)

Screenshot 2023-09-06 at 11 14 14

Do you have a fix for this in development?

@seanaguinaga
Copy link
Collaborator

Nevermind, I had the two projects running on the wrong ports

Connect - 3001
Web - 3000

@brunomoutinho
Copy link
Collaborator Author

When running Greenruhm Web, you do not need Greenruhm Connect to be running as well. You can just update the dependency using npm run local-dependency.

Greenruhm Connect needs to be running only when using its frontend, which we don't really do unless testing components in isolation. In this case, we also need Greenruhm Web to be running since it has the API endpoints.

@seanaguinaga
Copy link
Collaborator

yes! you taught me that amazing trick earlier! I have it working, now we just need to handle a redirect if the user is signed in? Or add another AuthStatus for the SignInView that says you are signed in?

@seanaguinaga
Copy link
Collaborator

I feel like, if the user is signed in, they should be sent to a user-details page or something like that that looks almost like the success page, from there they can log out, change stuff (maybe?)

@seanaguinaga
Copy link
Collaborator

Or do we take in a redirectURL as a part of the config, and let the consumer of the page specify a redirect, or perhaps both of these?

@brunomoutinho
Copy link
Collaborator Author

I think we should do the following:

  1. If the user was trying to get to a specific page (e.g. /u/<username>/create-drop) and we validated the user is signed in, we should redirect the user to that page (in this particular example, this new page would validate if the user in question is the same as the URI, but that's not something to worry now);
  2. If the user just opened the app (e.g. /), we should just sign them in and render the root page. For now, we do not have a specific index page for the application, so we can just show anything (even the success page for the sign in, for example).

Do you think it makes sense?

@seanaguinaga
Copy link
Collaborator

Do you think we should let the user be able to determine which pages are protected?

@brunomoutinho
Copy link
Collaborator Author

If by user you mean the client application, yes. At the end of the day, we're just providing them the tools to use our on-chain smart contracts. They should be able to manage everything related to their application :)

@seanaguinaga
Copy link
Collaborator

seanaguinaga commented Sep 7, 2023 •
edited
Loading

I am not seeing how we can provide the user's auth status so that the consumer of this package will be able to protect routes? There is a period of time where state is undefined when referenced on the landing page for example.

The state inside of connect also seems completely independent from the user-reducer inside of greenruhm-web? (which is why the UI is not reflecting actual auth state)

I am having a hard time following things.

Also, this API-style below may be useful for protecting routes and puts all control in the users hands and takes away our responsibility for owning pages.

import { withUser, AuthAction } from 'next-firebase-auth'

const MyLoader = () => <div>Loading...</div>

const LoginPage = () => <div>My login page</div>

export default withUser({
  whenAuthed: AuthAction.REDIRECT_TO_APP,
  whenUnauthedBeforeInit: AuthAction.SHOW_LOADER,
  whenUnauthedAfterInit: AuthAction.RENDER,
  LoaderComponent: MyLoader,
})(LoginPage)

@brunomoutinho
Copy link
Collaborator Author

Sending the user's authentication status back to the client application is the reason we introduced the onChangedSignIn prop to the instantiation of the connect SDK. The idea behind it is that the client application will initialize the SDK passing the onChangedSignIn callback function to it, then the SDK will check the information and, if the user is signed in and everything seems correct, it will call the onChangedSignIn callback function to tell the client application the new signed in status of the user (in this case, that the user is signed in).

In case the user goes through a sign out process, we're going to call that callback function as well, same for sign in and sign up :)

Connect is only concerned about the Magic authentication status. Connect is not going to care about routes or anything else. We're sending the client application the information we got from the user so that the client can decide what to do with it.

For Greenruhm Web, this includes making sure that specific routes are only accessible for signed in users, and some routes are only accessible to specific users. All this is to be controlled by the client application, not Greenruhm Connect.

Did this help in any way? Are there still questions about the workflow?

@seanaguinaga
Copy link
Collaborator

Got it

I am not sure which method on the connect object would return any user details? Which the consumer could then use to display data on who is signed in, etc.

I added one in the last two commits on enhancement-localStorage-AuthPersistence and tried to consume it in the sign in page as a test use.

@brunomoutinho
Copy link
Collaborator Author

You have exposed the getUser selector to the SDK, so I think we're good :) The client application would be able to use that to get all the user details we have in the SDK.

@seanaguinaga
Copy link
Collaborator

Okay cool

The sign-in page also uses a useEffect to use that state in the UI.

Would replicating this to the other pages and fixing copy errors be good enough to record a test for?

@brunomoutinho
Copy link
Collaborator Author

I think it would be good, yes :D

@brunomoutinho
Copy link
Collaborator Author

You can also create a HOC and include that in the pageHOC so that it will already be shared with every page :)

@seanaguinaga
Copy link
Collaborator

It's async though? Would that be okay to pass down? User | undefined?

@brunomoutinho
Copy link
Collaborator Author

Yes, I guess it would. The client application should react to the changes to authentication, so they'll only be trying to get the user after the user is there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brunomoutinho brunomoutinho

@seanaguinaga seanaguinaga

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@brunomoutinho @seanaguinaga