gh-1464 OAuth2 - Fix Authorization Code Grant Type Not Working #1465
Conversation
* Minimize Boot's AutoConfiguration for Security * Maintain full backwards compatibility - no test changes * Trigger OAuth2 Authorization Code Grant Type only for Browsers and only if they don't send a `application/json` Accept header
| final List<MediaType> supportedMediaTypes = super.resolveMediaTypes(request); | ||
|
|
||
| final String userAgent = request.getHeader(HttpHeaders.USER_AGENT); | ||
| if (userAgent != null && userAgent.contains("Mozilla/5.0") |
There was a problem hiding this comment.
I've understood that sniffing user-agent on server side is wrong and adds more maintenance. What about if it's something else that Mozilla/5.0?
There was a problem hiding this comment.
For 2.0 we need to revamp our:
- url structure and
- security setup
E.g. we should really move our api under something like /api. That way we can make more appropriate/explicit security assumption
We should also also eliminate this whole basic authentication setup for accessing the REST API eventually.
Thus, looking at the user-agent is more of a temporary solution. Took some hues from Initializr. See also:
http://www.useragentstring.com/pages/useragentstring.php
First I looked at triggering basic authentication only if a Json accept header is being sent. But that would have broken several tests - This late in the game I did not want to cause ANY breaking changes.
There was a problem hiding this comment.
Why get rid of basic auth? We can't assume OAuth setup everywhere, especially just 'out of the box' where someone on PWS/PCF or k8s on google would expose an app to the public without any security....
Merging now though....
|
It seems like not having a change to the test implies what we didn't have a test wrt to ensuring this functionality was working. Since OAuth functionality is important in the PCF environment, we should test there before releasing. |
|
@markpollack Added issue: #1468 to add tests to cover this functionality. |
|
BTW - there was a failing checkstyle test...fixing. |
|
merged as d82f10f |
application/jsonAccept headerresolves #1464