cloudendpoints / endpoints-java

A Java framework for building RESTful APIs on Google App Engine
Apache License 2.0
32 stars 35 forks source link

Add all declared scopes is auth.oauth2.scopes discovery file section #92

Closed clementdenis closed 6 years ago

clementdenis commented 7 years ago

Currently, only the email scope is asked by the API explorer, because other scopes declared by APIs and API methods are not aggregated in the discovery file. This brings Cloud Endpoints on par with the behavior of Google own APIs.

codecov-io commented 7 years ago

Codecov Report

Merging #92 into master will increase coverage by 0.06%. The diff coverage is 93.93%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #92      +/-   ##
============================================
+ Coverage     80.02%   80.09%   +0.06%     
- Complexity     1681     1689       +8     
============================================
  Files           156      157       +1     
  Lines          5597     5626      +29     
  Branches        731      732       +1     
============================================
+ Hits           4479     4506      +27     
- Misses          838      841       +3     
+ Partials        280      279       -1
Impacted Files Coverage Δ Complexity Δ
...e/api/server/spi/discovery/DiscoveryGenerator.java 88.84% <100%> (+0.44%) 63 <0> (+1) :arrow_up:
...i/server/spi/config/model/AuthScopeRepository.java 90% <90%> (ø) 7 <7> (?)
...ig/annotationreader/ApiAnnotationIntrospector.java 91.42% <0%> (-1.43%) 29% <0%> (-1%)
.../server/spi/discovery/CommonPathPrefixBuilder.java 100% <0%> (+5.55%) 9% <0%> (+1%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b16ce3f...d21d9cd. Read the comment docs.

clementdenis commented 6 years ago

Hi @tangiel , Could you have another look at this one? I added AuthScopeDescriptions to fetch up-to-date scopes automatically (updating scopeDescriptions.properties is still manual though).

clementdenis commented 6 years ago

I extracted the logic for scope description handling to its own class, should be clearer.

tangiel commented 6 years ago

I don't know if you saw my comment since it was in an outdated comment, but: This probably needs to be documented better, but the comma separation is a logical OR for scopes. To require multiple scopes simultaneously you are supposed to use one string with space separation.

clementdenis commented 6 years ago

I just updated the PR to handle scope conjunction expressions properly.

And I learned something about scope expressions :-)

tangiel commented 6 years ago

Thanks @clementdenis, sorry it took so long to review.