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

Consider adding a new loadDiscoveryDocumentAnd.....() function #359

Closed
jeroenheijmans opened this issue Jun 16, 2018 · 4 comments
Closed
Labels
feature-request Improvements and additions to the library.

Comments

@jeroenheijmans
Copy link
Collaborator

jeroenheijmans commented Jun 16, 2018

While investigating how to best use the library, I found that these:

  • loadDiscoveryDocumentAndTryLogin()
  • loadDiscoveryDocumentAndLogin()

both did not suit my needs. I want something in between, because it might be the case that:

  • You don't have a hash fragment from the IdServer (so tryLogin(...) won't help
  • You do have a logged in session at the IdServer ("remember me")

In these cases the methods:

  • loadDiscoveryDocumentAndTryLogin() will not log you in
  • loadDiscoveryDocumentAndLogin() will log you in, but it will redirect to-and-back from the IdServer

The redirects are not needed, because the session with IdServer allows for a silent refresh.

So I wrote this flow:

// 0. LOAD CONFIG:
// First we have to check to see how the IdServer is
// currently configured:
this.authService.loadDiscoveryDocument()

  // 1. HASH LOGIN:
  // Try to log in via hash fragment after redirect back
  // from IdServer from initImplicitFlow:
  .then(() => this.authService.tryLogin())

  .then(() => {
    if (!this.authService.hasValidAccessToken()) {

      // 2. SILENT LOGIN:
      // Try to log in via silent refresh because the IdServer
      // might have a cookie to remember the user, so we can
      // prevent doing a redirect:
      this.authService.silentRefresh()
        .catch(result => {
          // Subset of situations from https://openid.net/specs/openid-connect-core-1_0.html#AuthError
          // Only the ones where it's reasonably sure that sending the
          // user to the IdServer will help.
          const errorResponsesRequiringUserInteraction = [
            'interaction_required',
            'login_required',
            'account_selection_required',
            'consent_required',
          ];

          if (result && result.reason && errorResponsesRequiringUserInteraction.indexOf(result.reason.error) >= 0) {

            // 3. ASK FOR LOGIN:
            // At this point we know for sure that we have to ask the
            // user to log in, so we redirect them to the IdServer to
            // enter credentials:
            this.authService.initImplicitFlow();
          }
        });
    }
  });

I suggest adding this to the library as an additional convenience method (happy to make a PR if feedback's a green light). The only thing is I'm not sure what name to give it that's in line with the existing names, makes sense, and explains what's happening. The best I can come up with is loadDiscoveryDocumentAndLoginAsSilentlyAsPossible() which of course is terrible. Suggestions welcome.

Alternatively, if this new function is deemed to much for the basis, it might be good to make it part (a) of the readme, and/or (b) the samples, and/or (c) as an "additional documentation" bit.

Or perhaps my logic is completely flawed?

Feedback welcome!

@alecrt
Copy link

alecrt commented Jul 12, 2018

Great idea!

@ggjersund
Copy link

I'd love to see this as well, as I prefer to avoid visible redirects. Any plans when this will be added?

@ronnyek
Copy link

ronnyek commented Nov 28, 2018

Honestly, I couldn't get the authentication working remotely consistently without something like this... this seems to patch all the holes to me.

@jeroenheijmans
Copy link
Collaborator Author

I've found that:

  • no PRs (also not from myself) came in since I requested this in 2018)
  • with code flow now, the number of overloads would grow potentially
  • my applications tended to have slight variations of the above, so a default method may not be so useful

So I'm closing my own PR.

If you landed here to get a nice login sequence, I suggest checking out my example implementation and making it your own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Improvements and additions to the library.
Projects
None yet
Development

No branches or pull requests

4 participants