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.

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

Restore previous session #8

Closed
angelo-v opened this issue Jan 14, 2023 · 5 comments
Closed

Restore previous session #8

angelo-v opened this issue Jan 14, 2023 · 5 comments
Labels
enhancement New feature or request user story A description of what a user wants to achieve with the application

Comments

@angelo-v
Copy link
Contributor

angelo-v commented Jan 14, 2023

As an authenticated user of PodOS browser I want to be able to use my browser bookmarks and work with mutliple tabs without losing my session, or beeing redirected to other URLs, so that I can rely on core browser functionallity without being distracted.

Acceptance criteria:

  • When signed in in one tab, I can copy/duplicate the tab and I am still signed in on the new tab
  • When doing so, the new tab shows the correct resource, I am not redirected to somewhere else
  • When I open a bookmark of a protected resource it shows the bookmarked resource, because I am signed in
  • When hosting PodOS browser on a Solid Pod, opening a link in a new tab keeps me signed in

out of scope:

  • For hosted PodOS Browser opening a link in a new tab will navigate away from the app to the original resource URL, which is fine. The session for the target URL is out of control of the app.
@angelo-v angelo-v added the enhancement New feature or request label Jan 14, 2023
@angelo-v angelo-v added the user story A description of what a user wants to achieve with the application label Jan 25, 2023
@josephguillaume
Copy link
Contributor

I understand this is currently blocked at

async handleIncomingRedirect() {
return this.session.handleIncomingRedirect({
// session restore disabled, due to
// https://github.com/inrupt/solid-client-authn-js/issues/1647
restorePreviousSession: false,
});

For the standalone netlify version this is not an issue, but I understand the acceptance criteria here have a wider scope.

@josephguillaume
Copy link
Contributor

Current plan is to implement this at the same time as #27 in pos-app by using clientIds to support session restore, i.e. restoring dynamic client registrations would not (yet?) be supported.

Restoring previous session works by identifying that an oidcIssuer has been saved in localStorage and attempting a silent authentication with that issuer. If it succeeds, the app then handles the incoming redirect. If it fails, the stored oidcIssuer is removed. The os.session.login function is called by the login component and sets the oidcIssuer for the first time.

This works for clientIds because the redirectUrl and clientId are static as a property of the app. With dynamic registration, the client identifer and potentially the redirect URL can change, meaning that they need to be saved to localstorage and a valid session on the IDP may not exist.

pos-app will:

  • Load oidcIssuer from localStorage
  • redirectUrl and clientId pos-app attributes are saved as properties on the os.session object so they are available to the login function.
  • authentication.ts is modified to save oidcIssuer and the current url before performing the login (see Allow client identifier to be provided to login #27)
  • logout also needs to remove the saved oidcIssuer. Note that this is a local client logout - the user remains logged in on the IDP
  • os.session.session.onLogin needs to window.history.replaceState({}, "", origUrl) and trigger the app to process that new URL (equivalent to manually calling a window.onload function)
  • os.session.session.onError needs to remove the saved oidcIssuer and ideally show the user an error, i.e. log in failed

The core session restore code does:

  os.session.handleIncomingRedirect().then(info => {
    if (!info.isLoggedIn && oidcIssuer) {
      localStorage.setItem(clientId, oidcIssuer);
      localStorage.setItem(KEY_CURRENT_URL, window.location.href);
      os.session.session.login({
        prompt: "none",
        oidcIssuer: oidcIssuer,
        redirectUrl: redirectUrl,
        clientId: clientId
      });
    }
  });

@angelo-v
Copy link
Contributor Author

The session refresh as described in the story is working and fulfilling all the acceptance criteria. Anything going further should move to a new ticket.

@josephguillaume
Copy link
Contributor

I'm all for this mostly working solution, but I do want to document here that the acceptance criteria are not met when multiple PodOS apps are served from the same domain. If I am already logged in on another app on the same domain, I will instead be redirected to that app

This doesn't affect https://browser.pod-os.org/ as far as I can tell.

I've opened a new issue #49
In any case, this us the same blocker originally identified. There apparently hasn't been any progress upstream. #8 (comment)

@angelo-v
Copy link
Contributor Author

angelo-v commented Dec 9, 2024

The user story only refers to a single app (PodOS browser) and for this the acceptance criteria are met. Thanks for opening the new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request user story A description of what a user wants to achieve with the application
Projects
None yet
Development

No branches or pull requests

2 participants