Greenruhm / connect

Greenruhm Connect is your white label, custom-branded connection to the music metaverse. It allows you to provide music NFT services inside your app with your branding.
MIT License
1 stars 0 forks source link

Implement saving and loading session info from local storage #85

Open brunomoutinho opened 1 year ago

brunomoutinho commented 1 year ago

https://magic.link/docs/connect/wallet-api-reference/javascript-client-sdk

brunomoutinho commented 1 year ago

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

seanaguinaga commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Interesting

I think I understand

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

magic.user.isLoggedIn() could be used

seanaguinaga commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Do I need access to the greenruhm-web repo?

seanaguinaga commented 1 year ago

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

Screenshot 2023-08-24 at 16 07 31
brunomoutinho commented 1 year ago

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

seanaguinaga commented 1 year ago

I deleted node_modules in both projects

Reinstalled

And rebuilt as well

seanaguinaga commented 1 year ago

branch: enhancement-localStorage-AuthPersistence

seanaguinaga commented 1 year ago

@brunomoutinho any luck? I am completely stumped 🤔

brunomoutinho commented 1 year ago

@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 commented 1 year ago

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

brunomoutinho commented 1 year ago

The version prior to the branch I mentioned is not configured to use the correct version of greenruhm web and, if it uses the latest version, it does not use it in the correct way. If you look into the open PR in greenruhm-web, you can see the changes made :) actually, feel free to review it!

Enviado via Proton Mail para dispositivos móveis

-------- Mensagem Original -------- Em 29 de ago. de 2023 18:40, Sean Aguinaga escreveu:

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

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

seanaguinaga commented 1 year ago

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 commented 1 year ago

If I am not mistaken, there is a description in one of the readmes to set an environment variable to localhost. Can you check it? I am away from my computer for the next hours.

Enviado via Proton Mail para dispositivos móveis

-------- Mensagem Original -------- Em 29 de ago. de 2023 18:47, Sean Aguinaga escreveu:

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

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

brunomoutinho commented 1 year ago

In case you do not find in Amy of the readmes and md files (connect and web) the description of the environment variable, let me know and I'll send you the configuration steps when I get home.

Enviado via Proton Mail para dispositivos móveis

-------- Mensagem Original -------- Em 29 de ago. de 2023 18:55, Bruno Moutinho escreveu:

If I am not mistaken, there is a description in one of the readmes to set an environment variable to localhost. Can you check it? I am away from my computer for the next hours.

Enviado via Proton Mail para dispositivos móveis

-------- Mensagem Original -------- Em 29 de ago. de 2023 18:47, Sean Aguinaga escreveu:

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

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

seanaguinaga commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Connect - 3001 Web - 3000

brunomoutinho commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

brunomoutinho commented 1 year ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

I think it would be good, yes :D

brunomoutinho commented 12 months ago

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

seanaguinaga commented 12 months ago

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

brunomoutinho commented 12 months ago

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.