feat: Expose Granted Scopes while fetching credentials #230

bajajneha27 · 2023-08-11T11:01:25Z

When user credentials are fetched and refreshed, the OAuth server returns granted scopes. This change exposes those scopes in the credentials object

lib/signet/oauth_2/client.rb

sai-sunder-s · 2023-08-11T15:58:00Z

lib/signet/oauth_2/client.rb

+        if status == 200
+          parsed_response = ::Signet::OAuth2.parse_credentials body, content_type
+          parsed_response["granted_scopes"] = parsed_response.delete("scope") if parsed_response
+          parsed_response


should this be a return statement?

The return keyword is not necessary, and is generally idiomatically omitted, since the value of the last expression in a method (which in this case is whatever clause in the if-elsif-else is selected), is implicitly the return value.

That said, this construct would be better turned around, because currently the return value is buried in the middle of the method and thus isn't obvious. (It also invites a subtle bug where someone might inadvertently add another statement after this if-elsif-else clause, thus causing something different to be returned from the method.) It would be better for the else clause to be elsif status != 200, and then put this parsed_response stuff at the very end of the method, so that the return value is positioned in the obvious place at the end.

sai-sunder-s · 2023-08-11T15:58:35Z

lib/signet/oauth_2/client.rb

+
+        if status == 200
+          parsed_response = ::Signet::OAuth2.parse_credentials body, content_type
+          parsed_response["granted_scopes"] = parsed_response.delete("scope") if parsed_response


Can you explain this line to me please? I am not familiar with Ruby nor this library.

We already have a field called scope which represents the requested scope. The update_token method updates the field scope if the options hash passed to the method contains a key scope. I need to make sure that we're not mixing the requested scope and granted scopes. So, I create a separate key called granted_scopes ( and delete the scope key so that it doesn't get updated for requested scope ) and pass it to update_token method.

IIUC, "scope" field was already existing, if options had it, then the field is set. Now, that functionality is being removed?

I am trying to understand why both "Scopes" and "granted_scopes" cannot co-exist.

IIUC, "scope" field was already existing, if options had it, then the field is set. Now, that functionality is being removed?

The scope field here represents granted_scopes while the already existing scope field represents requested scopes. We need to differentiate the two and hence this deletion .

Let me know if it isn't clear. We can discuss it offline

bajajneha27 · 2023-08-21T10:35:03Z

@sai-sunder-s Gentle reminder :)

dazuma · 2023-08-22T05:46:48Z

lib/signet/oauth_2/client.rb

+      #   expressed as either an Array of String objects or as a
+      #   space-delimited String.
+      def granted_scopes= new_granted_scopes
+        @granted_scopes = new_granted_scopes&.split


If an Array is passed in for new_granted_scopes (see your docs above), it won't respond to the message split.

Yes. I have updated the docs to reflect the expected behavior here.

I'd suggest allowing Array to be passed. Otherwise, granted_scopes= takes a String but granted_scopes returns an Array, which would be confusing and invite buggy usage.

I didn't want this method to accept Array since we get space delimited String in response and this method should ideally not be used to update granted_scopes manually.
But for the sake of not introducing bugs, I'll make the change.

dazuma · 2023-08-22T05:47:11Z

lib/signet/oauth_2/client.rb

+      ##
+      # Sets the scopes returned by authorization server for this client.
+      #
+      # @param [Array] new_granted_scopes


If you want to support both Array and String (see your docs below), you should include both types here.

Also, do we need to support setting it back to nil?

Thanks for pointing this out. This was a miss from my end.
The auth server returns a space delimited string, so I thought of supporting only String for now.
And yes, we can set it back to nil.

spec/signet/oauth_2/client_spec.rb

dazuma · 2023-08-22T05:49:55Z

lib/signet/oauth_2/client.rb

@@ -167,6 +170,8 @@ def initialize options = {}
      #   - <code>:extension_parameters</code> -
      #     When using an extension grant type, this is the set of parameters used
      #     by that extension.
+      #   - <code>:granted_scopes</code> -


Can we use backticks instead of html tags?

bajajneha27 · 2023-08-24T08:54:48Z

@dazuma I addressed all the comments.

dazuma · 2023-08-28T16:16:04Z

lib/signet/oauth_2/client.rb

@@ -32,52 +32,54 @@ class Client
      #
      # @param [Hash] options
      #   The configuration parameters for the client.
-      #   - <code>:authorization_uri</code> -
+      #   - `authorization_uri` -


I'd keep the colons to emphasize that the keys are symbols.

dazuma · 2023-08-28T16:18:10Z

lib/signet/oauth_2/client.rb

+      #   expressed as either an Array of String objects or as a
+      #   space-delimited String.
+      def granted_scopes= new_granted_scopes
+        @granted_scopes = new_granted_scopes&.split


I'd suggest allowing Array to be passed. Otherwise, granted_scopes= takes a String but granted_scopes returns an Array, which would be confusing and invite buggy usage.

dazuma · 2023-08-28T16:18:53Z

lib/signet/oauth_2/client.rb

+      ##
+      # Returns the scopes granted by the authorization server.
+      #
+      # @return [Array] The scope of access returned by the authorization server.


It looks like this can return nil as well as Array, if the granted scopes were never set.

bmulholland · 2023-09-14T14:47:31Z

lib/signet/oauth_2/client.rb

      #     A shared symmetric secret issued by the authorization server,
      #     which is used to authenticate the client.
-      #   - <code>:scope</code> -
+      #   - `:scope` -


This is still listed in the comment but no longer exists; it's deleted below

bajajneha27 added 3 commits August 11, 2023 10:59

feat: Expose Granted Scopes while fetching credentials

463e0d5

ci failure fix

d205c62

rubocpp failure fix

0908a9e

sai-sunder-s reviewed Aug 11, 2023

View reviewed changes

bajajneha27 marked this pull request as ready for review August 11, 2023 17:04

bajajneha27 requested a review from a team as a code owner August 11, 2023 17:05

address review comments

73590bd

sai-sunder-s approved these changes Aug 21, 2023

View reviewed changes

dazuma requested changes Aug 22, 2023

View reviewed changes

Update documentation for new_granted_scopes

dc79e81

dazuma requested changes Aug 28, 2023

View reviewed changes

address review comments

38a6ae8

dazuma approved these changes Aug 29, 2023

View reviewed changes

bajajneha27 merged commit 48a6b78 into googleapis:main Sep 1, 2023
10 checks passed

release-please bot mentioned this pull request Sep 1, 2023

chore(main): release signet 0.18.0 #231

Merged

bmulholland reviewed Sep 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Expose Granted Scopes while fetching credentials #230

feat: Expose Granted Scopes while fetching credentials #230

bajajneha27 commented Aug 11, 2023 •

edited

Loading

sai-sunder-s Aug 11, 2023

dazuma Aug 11, 2023

sai-sunder-s Aug 11, 2023

bajajneha27 Aug 14, 2023

sai-sunder-s Aug 16, 2023

bajajneha27 Aug 17, 2023

bajajneha27 Aug 17, 2023

bajajneha27 commented Aug 21, 2023

dazuma Aug 22, 2023

bajajneha27 Aug 22, 2023

dazuma Aug 28, 2023

bajajneha27 Aug 29, 2023

dazuma Aug 22, 2023

bajajneha27 Aug 22, 2023

dazuma Aug 22, 2023

bajajneha27 Aug 22, 2023

bajajneha27 commented Aug 24, 2023

dazuma Aug 28, 2023

dazuma Aug 28, 2023

dazuma Aug 28, 2023

bmulholland Sep 14, 2023

feat: Expose Granted Scopes while fetching credentials #230

feat: Expose Granted Scopes while fetching credentials #230

Conversation

bajajneha27 commented Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajajneha27 commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajajneha27 commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajajneha27 commented Aug 11, 2023 •

edited

Loading