solid-client-authn-js icon indicating copy to clipboard operation
solid-client-authn-js copied to clipboard

Why do we have a separate ISessionInfo object?

Open RubenVerborgh opened this issue 5 years ago • 4 comments

When reading the docs, I saw a weird

session.info.isLoggedIn

What's the rationale for having this ghost object, instead of:

  • session.isLoggedIn?
  • session.webId?
  • session.sessionId?

The notion of info objects is not something I see in interfaces; I think it may point to a design flaw.

I also want to point out that, even if there is some need for such an info object (read-only session, maybe?), that one does not exclude the other. We could perfectly have session.isLoggedIn and other.isLoggedIn. The session.info.isLoggedIn just reads like a flaw.

RubenVerborgh avatar Nov 09 '20 10:11 RubenVerborgh

This object was initially introduced to match the return type of handleincomingRedirect, which returns not a Session, but a SessionInfo. However, I don't see any reason why the Session itself should not implement the ISessionInfo interface, and expose the isLoggedin, webId and sessionId properties instead of exposing an intermediary object that does.

NSeydoux avatar Nov 12 '20 08:11 NSeydoux

Thanks, @NSeydoux!

handleincomingRedirect, which returns not a Session, but a SessionInfo

Can we iterate a bit more on this? What exactly does handleincomingRedirect do and why does it return this specific data structure?

In any case, SessionInfo (at least the Info bit of it) seems to be a misnomer.

And there is also SessionInfoManager; should probably be SessionManager or so.

I don't see any reason why the Session itself should not implement the ISessionInfo interface

Super, tracking in https://github.com/inrupt/solid-client-authn-js/issues/590

RubenVerborgh avatar Nov 12 '20 15:11 RubenVerborgh

handleincomingRedirect is the function that completes the login: after the user is redirected from the OIDC provider, this reads the authorization code from the query params, retrieves the code verifier from storage, and makes the back channel request to the token endopoint to exchange the code for the ID, access and refresh tokens. Then, handleIncomingRedirect builds the authenticated fetch function holding the tokens in its closure, and binds this fetch to the Session. Therefore, only when handleIncomingRedirect returns is the login process complete. And for convenience, session.handleIncomingRedirect(someUrl) returns a SessionInfo object.

For a non-breaking change, what could be done is to make it so that handleIncomingRedirect returns the session it is bound to, which would implement the ISessionInfo interface. This way, libraries that use the return value would have no change to make, but the info could be deprecated easily.

NSeydoux avatar Nov 13 '20 09:11 NSeydoux

For a non-breaking change, what could be done is to make it so that handleIncomingRedirect returns the session it is bound to, which would implement the ISessionInfo interface. This way, libraries that use the return value would have no change to make, but the info could be deprecated easily.

Exactly. I think this is what we intended all along in the initial design; there was no ISessionInfo object. Updated the steps in https://github.com/inrupt/solid-client-authn-js/issues/590 to reflect this.

RubenVerborgh avatar Nov 13 '20 10:11 RubenVerborgh