manfredsteyer / angular-oauth2-oidc

Support for OAuth 2 and OpenId Connect (OIDC) in Angular.
MIT License
1.9k stars 688 forks source link

Custom discovery and code endpoints, optional fields in discovery document #1166

Open Bromles opened 2 years ago

Bromles commented 2 years ago

Is your feature request related to a problem? Please describe.

I am creating an application using data from a resource server that requires an OAuth 2 token obtained from a specific authorization server. Let's assume that the address of this server is "http://example.org". Then the authorization endpoint will be "/v2/oauth/authorize", the token endpoint "/v2/oauth/token", and the discovery endpoint "/.well-known/oauth-authorization-server".

In addition, their discovery document does not have some fields declared in OidcDiscoveryDoc as required and contains fields that are missing in OidcDiscoveryDoc, so even if the endpoint is changed, it will be impossible to load it.

At the moment, I can only set the endpoint of the token in the config, the other two are hardcoded in the library and cannot be changed. This prevents me from using the given cool library in my application

Describe the solution you'd like

Move the discovery and authorization endpoints to the AuthConfig with default values equal to the current hardcoded values. Make fields in the discovery document optional

Describe alternatives you've considered

Move the discovery and authorization endpoints to the AuthConfig with default values equal to the current hardcoded values. Give the opportunity to provide custom discovery document class

jeroenheijmans commented 2 years ago

Ooh, interesting issue you're facing with your authorization server.

Not sure if this totally prevents you from using the library. I think your question / request is the same as #1164 which also requests a way to partially override the results from loading the discovery document.

I personally think these extra moving parts are perhaps not worth the complexity to all other users of this library, as there are already workarounds available:

But in the end the call to find these workarounds lacking and consider it a feature request after all I'll leave up to the maintainer.

Hope this helps!

Bromles commented 2 years ago

Thank you for your reply! And yes, this is an interesting auth server, something between OAuth2 and OpenID Connect. But unfortunately, the resource server I need only accepts tokens from that server.

I personally think these extra moving parts are perhaps not worth the complexity to all other users of this library

I don’t think so, because the changes that I proposed (move the address of the discovery endpoint to the config, making it optional and specifying the default value equal to the current hardcoded one) will not affect current users in any way (this item will be absent in their config and will still be use the default value, that is, the current one), but it will make it possible to redefine this address for people like me, saving us an extra work. Perhaps I can arrange it myself in the form of a pull request, if no one is against it and I find the time

By the way, thanks for mentioning this workarounds. Indeed, I can use it, did not think about it and also was confused by some naming (token endpoint is called tokenEndpoint, authorization endpoint is called loginUrl) so I thought that I can't correctly specify the endpoints for the library

Bromles commented 2 years ago

Sorry, unintentional click

jeroenheijmans commented 2 years ago

Cheers, glad my comment helped you out!

Regarding:

will not affect current users in any way

I agree that it can be easily made in a backwards compatible fashion.

The concern I have is that it is yet another moving part and yet more configuration options. The cost lies in complexity to understand and use the library, as well as more code inherently leading to more (possible) bugs.

Your use case is certainly valid, I appreciate you (and others) have to work with a wide range of IDSes out there. I just don't think "more specific config options" is healthy, or at least my order of preference for options:

  1. Generic interception options that allow not only this use case, but also others. Requires more work, but is worth it since this lib is so highly used.
  2. Workarounds for non-spec-compliant IDSes such as http interceptors or not loading the discovery doc at all (the workarounds I mention above)
  3. Specific config options to deal with this, even though I prefer above 2 options, I feel your suggestion is still better than option 4.
  4. Don't do anything and leave people that need this without any option other than using a different library.

But again that's my 2 cents as a library user / community moderator. I'll leave the call up to the library's maintainer.

Bromles commented 2 years ago

Okay, let's wait for the opinion of the maintainers. After all, this is not a critical issue.

By the way, could you please suggest a custom token async storage workaround? I use Ionic and Capacitor Storage, but OAuthStorage does not allow me to make the getItem method async, and the Capacitor Storage returns a promise. So returning value is always null, because getItem is synchronous and Storage returns value after method's completion

Here is my implementation:

setItem and removeItem is fine, getItem always return null ``` import { Injectable } from '@angular/core'; import { Storage } from '@capacitor/storage'; import { OAuthStorage } from 'angular-oauth2-oidc'; @Injectable() export class CapacitorStorage extends OAuthStorage { constructor() { super(); } async setItem(key: string, value: string) { await Storage.set({ key, value }); } getItem(key: string) { let value = null; Storage.get({ key }).then(result => value = result.value); return value; } async removeItem(key: string) { await Storage.remove({ key }); } } ```