oidc-client-js icon indicating copy to clipboard operation
oidc-client-js copied to clipboard

Documentation

Open ShadSterling opened this issue 7 years ago • 26 comments

The documentation in your wiki is horribly inadequate.

Maybe I'm not your target audience. I'm not familiar with the OIDC protocol, and I was handed a already started webapp which uses this library for authentication. The way this library was used in the app barely worked at all, and when trying to make improvements the documentation was very little help. Having to figure out how the library is intended to be used rather than having it explained in the documentation was enough work that for a while we weren't sure it was worth using. Now that I've got everything working I think the library works well, but desperately needs better documentation.

With that in mind, here are a few things that should be included in your documentation, along with whatever corrections are necessary for things I've misunderstood.

I'm doing everything in TypeScript, so there may be some differences in usage from pure JavaScript.

I'm not using this repository directly, I'm using a fork that was made for the app before it was handed to me, and modified to be compatible with a GLUU server: https://github.com/gastate/oidc-client-js

Basics

All signin methods require at least these two steps:

  1. Make the browser load a URL on the authentication server
  2. Handle the redirect when the authentication server navigates the browser back to your redirect handler

The redirect will have a query string that this library can decode into user information

There are 3 variations of this process:

  1. signinRedirect, where the library navigates the browser away from your page to the login form on the authentication server
  2. signinSilent, where the library generates a hidden iframe into which the browser loads a URL on the authentication server
  3. signingPopup, where the library opens a popup window with a URL on the authentication server

It may not be possible to distinguish which variation is being used from the query string generated buy your authentication server, so you should expect to need a separate redirect handler URL for each variation that you use.

signinRedirect

In this variation the browser leaves your page entirely, then comes back to it after the user has successfully authenticated. This always works to get user information.

The URL that the user will be sent back to is the redirect_uri given when constructing the UserManager on which you call signinRedirect. If signinRedirect is given a parameter that has a state property, the value of that property will be reconstructed later on the User object.

The page that loads at redirect_uri can generate a User by reconstructing the UserManager and invoking signinRedirectCallback with the query string that came from the authentication server. The state property on the User object will be the state property of the parameter given to signinRedirect.

signinRedirectCallback causes the User object to be generated, but the promise it returns resolves to null, so the object must be retrieved by other means. If User generation fails, the promise will reject with a non-null error.

signinSilent

In this variation the library generates a hidden iframe into which the browser loads a URL on the authentication server, then redirects the iframe to your page. This only works to get user information when the authentication server is able to recognize an active session for the browser without any user interaction.

The URL that the iframe will be redirected to is the silent_redirect_uri given when constructing the UserManager on which you call signinSilent. If signinSilent is given a parameter that has a state property, the value of that property will be reconstructed later on the User object.

The page that loads at silent_redirect_uri cannot generate a User itself; the iframe is hidden and temporary, so that probably wouldn't be useful anyway. What it can do is trigger the generation of a User in the parent page by reconstructing the UserManager and invoking signinSilentCallback with the query string that came from the authentication server. The state property on the User object will be the state property of the parameter given to signinRedirect.

signinRedirect causes the User object to be generated, but the promise it returns resolves to null, so the object must be retrieved by other means. If User generation fails, the promise will reject with a non-null error.

Retrieving the User object

Since the methods that trigger generation of a User object do not return that object (or a promise that resolves to that object, the generated User object must be retrieved by one of these means

  1. The getUser method on UserManager returns a promise that resolves to a User object. This can be done any time after generation, and must be done again if other User objects are generated later. If getUser is called before generation, it may resolve to a stale User with invalid tokens.

  2. The events.addUserLoaded method on UserManager takes a callback function as a parameter, and thereafter whenever a User is generated the callback will be invoked with that User as a parameter. This can be done any time before generation, and does not need to be done again if other User objects are generated later (the same callback will be invoked again).

For me, being in a violently asynchronous environment and required to not call getUser until after generation, I'm just not calling getUser at all. Unfortunately, since the methods that trigger generation don't also provide the generated User, and I have a class that abstracts this whole login process away from the rest of the app (and handles the various callbacks) where I want to have methods that always resolve to some user information regardless of whether generation is necessary, I ended up having to make this library to "convert" events to promises.

User validity

The library makes some attempt to save user information where it can be reloaded without making network requests. If getUser is called before triggering generation it might return a stale User with expired access tokens. If you get a User from getUser before triggering generation, do not assume the access tokens are valid. There are at least 2 ways to ensure that you don't use an expired User:

  1. Check the expired and expires_in properties on the User object. These can tell you if the user has expired, but cannot tell you if the user has logged out in another tab and thereby invalidated the tokens.

  2. Just do a signinSilent before you even think about calling getUser. If it resolves, the User you get immediately after is valid; if it rejects you need to do an interactive login e.g. with signinRedirect.

The querySessionStatus method on UserManager may be able to give complete status information, but it performs a callback to silent_redirect_uri that has a very different query than signinSilent, and somehow while I was doing that I didn't think to pass that to signinSilentCallback (probably because I hadn't figured out that the different URLs MUST be distinct and was trying to detect based on the query). But it seems silly to do a querySessionStatus which does almost the same thing as a signinSilent but might have to be followed by a signinSilent anyway.

... But I've also seen cases where signinSilent seems to generate a user with invalid tokens, so I think I'm still missing something here.

Misc notes

Your callback URLs must be specified in the authentication server configuration, or the authentication server will not redirect to them.

Some authentication servers take your callback URL and append a query string that starts with "&" without including a "?" anywhere, which generates non-compliant URLs. If your callback URLs already have a query in them, appending a query that starts with "&" results in a compliant URL.

signinRedirectCallback etc can accept the entire URI in window.location.href; it isn't necessary to extract the query string first, and they will find the information in a non-compliant URL which is missing the "?"

automaticSilentRenew amounts to a periodic signinSilent. If a callback has been given to events.addUserLoaded, it is invoked when renewal succeeds.

ShadSterling avatar May 30 '17 22:05 ShadSterling

I appreciate your feedback and detailed notes.

Given that this is not a commercial product, I'm not sure what you expected in terms of docs. I work on this project in my free time (between paid work, which means nights/weekends/etc), and I'm guessing that the countless hours I've put into it has still saved you time on your project (despite the time it's taken you to learn how to use it). I certainly welcome contributions, especially to the docs/wiki.

I will attempt to take this feedback above and consider how to incorporate it into the wiki. I'm happy for you to continue in any additional level of assistance you're willing to offer.

brockallen avatar May 30 '17 22:05 brockallen

Not that this assuages your frustration, but I have added a clause to the wiki to set expectations: https://github.com/IdentityModel/oidc-client-js/wiki/Home/2fc5816d3ac19b392a07b136f0179c1be254d131

I'm still interested in reviewing your detailed feedback and incorporating it into the wiki/docs. I don't have time in the near term for that, but I will add it to my queue. Thanks again.

brockallen avatar May 30 '17 22:05 brockallen

To clear, I appreciate your work, was frustrated at the situation as a whole, and tried to write what would have most helped me at the beginning. I've never tried to suggest edits to a wiki, so I'm not sure how that works. I did look up tutorials on the protocol, and found them all to be very long and far more in depth that I hoped I would need when using the library - and if my summary above is even close to adequate I was right about that. For the time being I'll be focused on other things, but when I have occasion to revisit this I'll try to be helpful.

ShadSterling avatar May 31 '17 20:05 ShadSterling

Thanks so much @Polyergic that was SO SO helpful. Our authentication went from pretty shoddy to infinitely better in under a day just by reading your discoveries.

houseme-brandon avatar Oct 27 '17 14:10 houseme-brandon

@brockallen I'd be happy to help update the Wiki with Shad's already excellent write up if that was something you were interested in still.

davidamidon avatar Apr 25 '18 00:04 davidamidon

Sure. If the wiki works, then that's a fine place to put it.

brockallen avatar Apr 26 '18 13:04 brockallen

Thanks @brockallen but I don't see an edit option on the wiki. Can you give me edits rights so that I may update it.

davidamidon avatar May 02 '18 23:05 davidamidon

Unfortunately the wiki in github does not have such fine grained controls. I'd suggest writing up some markdown and we can review to merge into the wiki.

brockallen avatar May 02 '18 23:05 brockallen

Ahh ok no worries I'll work on it this way and put in a PR when ready

davidamidon avatar May 02 '18 23:05 davidamidon

yea, even just create some doc.md file -- that's perfect. and if you want to do a whole docs folder, then go crazy

brockallen avatar May 02 '18 23:05 brockallen

I don't think you have access to the markdown of my comments, so I pulled it out into a gist: https://gist.github.com/ShadSterling/e28e55751c7b709e859aa17635989c00

ShadSterling avatar May 03 '18 00:05 ShadSterling

Great, thank you both. I'll get something put together this week.

davidamidon avatar May 03 '18 00:05 davidamidon

Here is what I put together. Please let me know if you would like it submitted in some other way or have any suggested edits. I tried to tie it into what was already included in the wiki and removed the first person portions.

https://gist.github.com/davidamidon/24c8a6980e116e62f781be4d6239d10d

davidamidon avatar May 04 '18 04:05 davidamidon

Having my explanatory text merged in with the more reference-style docs is a bit weird. It might be better to have multiple pages. The worst bit is having the "Retrieving the User object" and "User validity" headings under the "Methods" heading; there's no clear delimiter between the "User validity" subsection and the actual list of methods, which is confusing to read. It's also weird that my howtoish information about signinRedirect and signinSilent isn't in the methods section as documentation of those methods. Even if not on separate pages, it might be better to keep the reference sections together and have links to & from the explanatory text rather than intermingling them.

ShadSterling avatar May 04 '18 20:05 ShadSterling

Ok thank you for the feedback, I'll make some edits based on this and resubmit.

davidamidon avatar May 07 '18 19:05 davidamidon

Ok I've updated to include multiple pages. Please review.

https://github.com/davidamidon/oidc-client-js/wiki

davidamidon avatar May 08 '18 16:05 davidamidon

That looks like progress! Thanks for doing this.

It would be nice to establish a process for future contributions to the documentation. I notice when I view those pages I can see an edit button, and apparently I can just edit it without any review. Did you add me to the project in some way or is it just open for anyone to edit? Is there an option to allow suggested edits? If not, there is at least one suggestion for allowing wiki contributions here: https://stackoverflow.com/questions/10642928/how-to-pull-request-a-wiki-page-on-github

ShadSterling avatar May 11 '18 00:05 ShadSterling

I notice when I view those pages I can see an edit button, and apparently I can just edit it without any review.

Yea, that's an aspect of how wiki works on github. Perhaps something more like readthedocs? I never had time for that (and felt it might be a bit overkill).

brockallen avatar May 11 '18 00:05 brockallen

I'm not familiar with readthedocs, but it looks more like it's for documentation hosting/distribution than for collaborative editing.

ShadSterling avatar May 14 '18 16:05 ShadSterling

Right -- but we'd host the content here somewhere as content which PRs can be done against. Like we do in IdentityServer: https://github.com/IdentityServer/IdentityServer4/tree/dev/docs and get pushed to here: http://docs.identityserver.io/en/release/

brockallen avatar May 15 '18 21:05 brockallen

That looks like a good arrangement.

ShadSterling avatar May 19 '18 15:05 ShadSterling

@brockallen also there is no mention of userSessionChanged event anywhere in docs. Occasionally found it in source.

Greencash avatar Oct 25 '18 15:10 Greencash

There still seems to be a lot of info in this issue that isn't added to the docs? (I'd add it myself if I wasn't as confused about how this library works)

Powersource avatar Nov 19 '20 20:11 Powersource

A lot has happened since 2018. I've noticed there is a github repo for IdentityModel docs that contains one little section for oidc-client. I'm assuming the intention is to start writing documentation there one day?

https://github.com/IdentityModel/Documentation/tree/main/docs/js https://identitymodel.readthedocs.io/en/latest/js/overview.html

coolhome avatar Dec 21 '20 20:12 coolhome

Yes, at some point. But given that I don't have a company sponsoring me to work on oidc-client-js (and I happen to have a lot of other things going on in my life now) this project is unfortunately suffering from neglect. There's only so much time in the day and for my own health/sanity I'm trying to not also work 7 days a week (like I used to) :/

brockallen avatar Dec 21 '20 22:12 brockallen

Could a good start be to allow external contributors to edit the wiki?

Powersource avatar Jan 08 '21 16:01 Powersource