-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
Implement prototype for handling multiple logins [WIP] #120
Conversation
src/storage.js
Outdated
@@ -105,3 +75,43 @@ export function ipcStorage(client: Client): AsyncStorage { | |||
client.request('storage/removeItem', key) | |||
} | |||
} | |||
|
|||
export class StorageSession { |
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.
SessionStorage
?
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, that is definitely better
Thanks, great stuff! I'm impressed. Some quick points below.
True. You can force a logout though (which is what solid-auth-client will do if you call
Is your solution backward and forward compatible?
Maybe the session terminology is confusing. How about I am wondering a bit about how to name sessions/clients. How would you expect this to be used in practice? Would we really want to give them names, or just let people log in multiple times (and, for instance, identify by WebID)? |
Thanks for your feedback, I will continue working on it when I find the time.
Are both of following logout requests used for this? (i.e. can I simply call the logout function in webid-oidc.js if a login is requested, but a different instance already is logged in with that provider?)
When switching, the current logins would be forgotten (could be changed by changing the default storage name). And I agree with you, that the naming is confusing. Regarding the IDs, I am not sure of the ways people will use it. The reason I did it that way is, to give apps the ability to differentiate between accounts which serve different purposes. With the id, they can hardcode the purpose into the id and always receive it that way. Without the id, they would need to associate the id and the webid and store this across sessions (e.g. localStorage, cookies). const senderAuthClient = new solid.auth.openSession('sender');
const receiverAuthClient = new solid.auth.openSession('receiver');
// always send a file from the sender pod to the receiver pod That's why I thought it could be convenient to specify an ID. Maybe it could default to the webId. |
Just the IDP logout should do.
Let's do that 👍
Just a note: might affect the popup (but you probably looked into that).
We're creating a client, I'd say.
I think this will be the most common scenario. I can't imagine the IDs really working for apps. (And if it does, they can keep their own internal mapping.) |
Don't enforcing an ID as parameter makes it a bit trickier from my point of view. Here are some options I see: Storage format(1) Using webId as a key (2) Using an array For both of these options, a temporary storage (sessionStorage or the memStorage) seems useful to store the state prior to the login. Do you have any thoughts on this? I would tend to the second option. |
The second option seems good, then we can offer the options as a set. I was thinking that maybe we can do both:
So a key would be optional (but you can still use it if you want), and there is an additional option to get all the client instances. That way, we don't have to predict how people will use the interface. |
I've added the possibility to pass no id in the constructor. So
If we do it this way, people will have to reenter their credentials every time they are prompted to login. I think a good way would be to provide them a list of currently logged in accounts from which they can pick and a "Different account..." option. But that's likely a bit of work to do. I didn't manage to get the tests working, so I didn't commit and push yet. If you want I can force a commit and push or send it as a zip(/...). I tried following to test but the jest tests failed: git clone https://github.com/solid/solid-auth-client
cd solid-auth-client
npm install
npm test Here are the test results as json: test.txt I've tested it on localhost, but only the popup method worked (also with the original). It seems to work fine, but there are no automatic tests for multiple logins yet. |
I still don't know how to fix these errors. I've just pushed it now so you can check it out locally if you want (or take a look at the Travis tests). |
I've changed the storage behavior to allow the storing of multiple login sessions.
Fixes #117
With these changes it's possible to login with multiple accounts (from different service providers for now) and make fetch requests to them. The different account credentials are stored in different locations in the localStorage (or other provided asyncStorage).
A short example running in the demo app:
Something I didn't manage to get working yet is to login with two accounts from the same provider (e.g.
xyz.solid.community
andabc.solid.community
). I believe that the redirection here checks if you are currently logged in on this provider and, if yes, doesn't ask for your credentials again. But I didn't have much time to investigate that so I'm not sure. Here's where I think the error happens:https://github.com/solid/solid-auth-client/blob/186236c5ebaa8b7f474f8f8ab3d38d15c06e101c/src/webid-oidc.js#L157-L159
I don't have time in the next days to work in this. So if you want, you can continue working on this in the meantime, or I will take it up again when I have time. And feel free to criticize anything :)
TL;DR
solid.auth.openSession(sessionId)
)