Skip to content
This repository was archived by the owner on Jun 30, 2023. It is now read-only.

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

Merged
tangiel merged 4 commits intocloudendpoints:masterfrom
AODocs:scopes_in_discovery
Aug 16, 2018
Merged

Add all declared scopes is auth.oauth2.scopes discovery file section#92
tangiel merged 4 commits intocloudendpoints:masterfrom
AODocs:scopes_in_discovery

Conversation

@clementdenis
Copy link
Contributor

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
Copy link

codecov-io commented May 11, 2017

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) ⬆️
...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%) ⬆️

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.

Copy link
Contributor

@tangiel tangiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the slow review.

@@ -0,0 +1,219 @@
#Source: https://developers.google.com/identity/protocols/googlescopes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file generated or hand written? Might be a hassle to keep up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keys and values were extracted separately using the xpath expressions below, and merged in a spreadsheet.
Then some transformations were performed to deduplicate, sort the entries and add comments.

All of this could be easily automated, but I was hoping you had an "official" list of scope descriptions at Google.
This would be much better than scrapping a web page. Do you know if something like that exists?

@clementdenis clementdenis force-pushed the scopes_in_discovery branch 3 times, most recently from 420aa53 to 6e3550b Compare July 31, 2018 12:35
@clementdenis clementdenis force-pushed the scopes_in_discovery branch from 6e3550b to 17e4de6 Compare July 31, 2018 13:45
Move scopeDescriptions.properties to DiscoveryGenerator packge
@clementdenis
Copy link
Contributor Author

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).

}

LinkedHashMap<String, ScopesElement> scopeElements = new LinkedHashMap<>();
for (String scope : allScopes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably this doesn't work when multiple scopes are required? What would happen if the scope expression is "email profile" for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but is this how the @Api*.scopes field is supposed to be used?
It is a String array, so I assumed each scope must be declared independently.
i.e. @apimethod(scopes = {"email", "profile"}) rather than @apimethod(scopes = {"email profile"})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- follows the schema repository pattern already used in DiscoveryGenerator
- could be reused for swagger generation (if / when supporting accessToken security definition)
- added some tests for proper merging of existing scope declarations
@clementdenis
Copy link
Contributor Author

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

@tangiel
Copy link
Contributor

tangiel commented Aug 14, 2018

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
Copy link
Contributor Author

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

And I learned something about scope expressions :-)

@tangiel
Copy link
Contributor

tangiel commented Aug 16, 2018

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

@tangiel tangiel merged commit b3ec380 into cloudendpoints:master Aug 16, 2018
@clementdenis clementdenis deleted the scopes_in_discovery branch February 21, 2022 13:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants