solid-client-authn-browser should use storage abstraction for storing a current session or offer control of a namespace in the localstorage

[Maximvdw]

Search terms you've used

currentSession, prefix localstorage, localstorage, sub diretory,

Impacted package

Which packages do you think might be impacted by the bug ?

• solid-client-authn-browser
• solid-client-authn-node
• solid-client-authn-core
• oidc-client-ext
• Other (please specify): ...

Bug description

I have a use case where I have 2 or more solid apps in the same domain under a different path. Each app uses
localstorage for session management with a unique prefix (e.g. app1.string.solidClientAuthenticationUser:...). This works fine apart
for the currentSession in combination with the restorePreviousSession option. Logging in to another application will cause the restore session to fail for other apps in the domain.

According to the description:

solid-client-authn-js/packages/browser/src/Session.ts

Lines 227 to 233 in 3fd7bb4

	// When a session is logged in, we want to track its ID in local storage to
	// enable silent refresh. The current session ID specifically stored in 'localStorage'
	// (as opposed to using our storage abstraction layer) because it is only
	// used in a browser-specific mechanism.
	this.on(EVENTS.LOGIN, () =>
	window.localStorage.setItem(KEY_CURRENT_SESSION, this.info.sessionId)
	);

the reason for not using the storage abstraction layer is because of the browser-only context where
this current session is stored. However, as a developer there is no control with this decision.
There is also no way to change the namespace of the localstorage to include a prefix.

To Reproduce

1. Create two applications under the same domain in different sub directories (/app1, /app2 )
2. Use a custom storage method (localstorage) with a unique prefix for each sub directory
3. Log in to /app1 with restorePreviousSession enabled
4. Refresh the page (you will not have to log in)
5. Visit /app2 with ``restorePreviousSession` enabled and log in
6. Refresh the page (you will not have to log in)
7. Visit /app1 again, you will have to log in again

Expected result

I would expect solid-client-authn-browser to use the storage method that is provided, or to have some control in the key that is used (i.e. a prefix). Developers are also not able to handle the storing of a current session themselves due to the unexported silentlyAuthenticate.

Personally I feel that it should use the provided storage method - as this method is used to find the session information

solid-client-authn-js/packages/browser/src/Session.ts

Lines 350 to 368 in 3fd7bb4

	} else if (options.restorePreviousSession === true) {
	// Silent authentication happens after a refresh, which means there are no
	// OAuth params in the current location IRI. It can only succeed if a session
	// was previously logged in, in which case its ID will be present with a known
	// identifier in local storage.
	// Check if we have a locally stored session ID...
	const storedSessionId = window.localStorage.getItem(KEY_CURRENT_SESSION);
	// ...if not, then there is no ID token, and so silent authentication cannot happen, but
	// if we do have a stored session ID, attempt to re-authenticate now silently.
	if (storedSessionId !== null) {
	// TODO: iframe-based authentication being still experimental, it is disabled
	// by default here. When it settles down, the following could be set to true,
	// in which case the unresolving promise afterwards would need to be changed.
	const attemptedSilentAuthentication = await silentlyAuthenticate(
	storedSessionId,
	this.clientAuthentication,
	undefined,
	this
	);

Actual result

The client library has full control over the key causing unexpected behaviour as mentioned in the bug report.

Environment

    @inrupt/solid-client-authn-browser: ^1.11.7 => 1.11.7

[Maximvdw]

and update here?

[Maximvdw]

Is there any change required to make this fix happen? This is a huge limitation for same-domain solid apps

[NSeydoux]

Hi @Maximvdw , I'm sorry this hasn't been replied to. Currently, there is no support for same-origin apps, as these would end up sharing resources in the browser that are partitioned by origin and we want to keep isolated. Storage is one such resource.

I understand this feature request is a workaround that limitation, but two apps under the same origin would still be able to go into each others local storage, even if we had a namespacing mechanism which would only make this work on the happy path.

Supporting same-origin apps is not on the roadmap, so I don't think this issue will be fixed anytime soon if ever.

[Chiyo-no-sake]

Apart from same-origin apps, I found this problem also when writing an application that manages two different oidc contexts.
Many other libraries do use the oidc. prefix to store session management, and as I've just found out, this causes loops in authentication between the two, because this library clears the same storage. (see e.g. oidc-client-ts).

Maybe a simple change of this prefix with a more specific one for solid-client-authn-js will at least solve this problem, whitout the need to add support for same origin apps or the ability to set a dynamic prefix.

At least documentation should mention this as 'oidc' really is a common prefix to use.

[NSeydoux]

@Chiyo-no-sake this is most probably caused by us using a fork of oidc-client-js (before oidc-client-ts was a thing). Since they come from the same codebase, they use the same naming scheme. I'll look if this is configurable.

May I ask why you are using two OpenID client libraries side-by-side?

[Chiyo-no-sake]

I need my user to log in both to their SOLID Pod via this library and to the company's IDP via oidc-client-ts, to interact with their APIs.
The prefix is configurable for oidc-client-ts indeed, but I think it's easy to encounter this problem if it's not clear to the user which prefixes in the storage are managed via this library, and the user tries to use the same prefix for other logic