manfredsteyer / angular-oauth2-oidc

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

Provide content-type for default HttpInterceptor #988

Open zuboje opened 3 years ago

zuboje commented 3 years ago

Is your feature request related to a problem? Please describe. When you are using default interceptors which are sufficient for 90% of use cases, the interceptor should not assume Content-Type. Right now if I use this interceptor I still need to provide a manually Content-type, but it should allow specifying content-type for all requests. Since we are already intercepting all HTTP requests it would be handy to add in a content-type or if not content type allow Array of key/value pars for additional "default" headers when using

OAuthModule.forRoot({
    resourceServer: {
        allowedUrls: ['http://www.angular.at/api'],
        sendAccessToken: true
    }
})

Describe the solution you'd like Allow array parameter where we can supply additional default header values like content-type or similar

Describe alternatives you've considered We can still write another class if we want to do this or we can use a custom interceptor.

Additional context It is common when calling API services that we use different content types and we add additional header values, it would be great if we can add key/value pairs for headers to the default interceptor built with this plugin

jeroenheijmans commented 3 years ago

I'll tag it as a feature request. I'm not sure if it should go into this library though, as it would loose focus and its single responsibility. The alternative you describe seems just fine to me? On a side note, I nearly never even set a custom content-type (but that might be because I tend to send just json?), when would you actually need this? Just curious.

zuboje commented 3 years ago

@jeroenheijmans the reason I mention is that angular by default sends as plain text, you have to specify the content-type to be application/json. While I can make my own interceptor for this, it would be nice if I could use a single interceptor for HTTP requests rather than creating for each little thing. But with the raise of Angular usage, you may find developers using XML as content type as some API developers prefer XML over JSON.

jeroenheijmans commented 3 years ago

a single interceptor for HTTP requests rather than creating for each little thing

I guess that's a matter of taste, I'd personally prefer to adhere to the 'single responsibility' advice and keep units focused. Especially in a library, where every line of code is one (potential bug) that you ship to all users.

Another reason to keep it out of the library, is that there will be no end to the amount of customizations needed to this feature. Some applications will need different content-type for different endpoints, have exceptions to the rule, will want to change it based on the data being sent, etc.

I really feel people should create their own application-specific interceptor to handle this concern, and that the library should remain focused on doing only OAuth2/OIDC related work.

zuboje commented 3 years ago

@jeroenheijmans I would not disagree with you and I do see your point. I was simply asking for 2-3 lines of codes where I can optionally provide content-type (if provided, add to headers if not great). But to your point then you should not introduce at all your HTTP interceptor that added to header "Authorization" value, because in your own words your library should not be doing that, and consumers of your plugin should have that created themselves. Again I respect your work and plugin so please don't get me wrong but you did against your own principle here, yet when I asked for simple enhancement I was told it is a bad way of doing it :)

jeroenheijmans commented 3 years ago

I meant no offense! Nor did I mean to imply that your way of doing things or feature request is inherently 'bad'. I did mean to say that there's a trade-off when adding code to a library, as it adds complexity, responsibility, etc. Even if it's only a few lines of code.

In this case I personally don't think it's a good trade-off. For the built in interceptor that adds the bearer header, I think the case is different. This is a concern of the library, as it deals with OAuth2/OIDC work so the client doesn't have to. And that's directly related to the explicit purpose of the library.

Then again, I'm just one voice, and mostly a moderator of issues. I'll leave the final call to maintainer, with possibly input from the rest of the community.