search

openid / AppAuth-JS

JavaScript client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
Apache License 2.0
977 stars 162 forks source link

Add support for additional OpenID Connect Discovery fields #99

Closed emusgrave closed 5 years ago

emusgrave commented 5 years ago

The AuthorizationServiceConfiguration only supported returning a handful of the fields that are fetched from the ./.well-known/openid-configuration endpoint. I have added the remaining fields that are defined in the spec here.

I also added the rimraf package and changed the clean npm script to use it instead of rm, because on Windows rm does not exist.

tikurahul commented 5 years ago

Something is off here. Your change essentially reformats all existing code. I am not sure why. Also why are you removing the prepare script ?

emusgrave commented 5 years ago

Sorry, I mistakenly pushed a commit which had the prepare script removed. That was just something I was experimenting with locally and didn't mean for it to make it into this pull request. (I pushed the commit after the PR was submitted, but I supposed that github picks up everything on the fork)

As for all the files being changed, I guess I don't understand the purpose of the built directory being committed to the repo. I made the code changes and then ran the build and committed all of the resulting js files and .d.ts files. Are these not supposed to be pushed? I could use some instruction on how to handle this situation.

Thanks!

tikurahul commented 5 years ago

The built directory is the one that get's used by packages that define a dependency on this library. We write TypeScript, and that gets compiled to the built directory. Everyone eventually depends on the built javascript.

Taking a step back, I am not sure I understand the rationale for this change to begin with. Yes, there are additional fields in the spec for the configuration. Why should this library care about fields that we don't intend to use ? Also, you can easily extend AuthorizationServiceConfiguration to do your own thing, without having to pollute the library with all these additional (and potentially confusing) keys.

I appreciate the change, but I think we are better served by being a lean library. My intention is to also not cover the full breadth of the OpenID spec. I only intend to cover the meaningful parts for OAuth2 + PKCE which happens to be the CBP for authentication.

emusgrave commented 5 years ago

@tikurahul I apologize for misunderstanding the scope of this library. I figured that as a JS library under the openid namespace with the description "JavaScript client SDK for communicating with OAuth 2.0 and OpenID Connect providers.", you would want to support the full spec.

Closing PR. I'll go a different route for adding openid support to my programs...