Repository: GoogleCloudPlatform/google-auth-library-python (python)

Issue:
GCE Credentials should not derive from Scoped
Because `with_scopes` raises `NotImplementedError`, the class cannot be used polymorphically where instances of `Scoped` are expected (see https://github.com/GoogleCloudPlatform/google-cloud-python/issues/3933).  For instance, in C++,`google.auth.compute_engine.credentials.Credentials` wouldn't even be instantiable, because `with_scopes` is a pure virtual method cognate.

If there is a need for *part* of the interface to be expressed (e.g., the `scopes`, `requires_scopes`, and `has_scopes` methods), that should be factored into another class (maybed `ReadonlyScoped`?), with the current `Scoped` class extending it.

Pull request:
Move read-only methods of 'Scoped' into new interface, 'ReadOnlyScoped'.
Not all subclasses of 'Scoped' can sanely implement `with_scopes` (e.g, on GCE the scopes are hard-wired in when creating the GCE node).

Make `Scoped` derive from `ReadOnlyScoped`, adding the `with_scopes` method.

Make GCE's `Credentials` class derive from `ReadOnlyScoped`.

Closes #194.

Patch:
diff --git a/google/auth/compute_engine/credentials.py b/google/auth/compute_engine/credentials.py
index 5729956..f2a4656 100644
--- a/google/auth/compute_engine/credentials.py
+++ b/google/auth/compute_engine/credentials.py
@@ -24,7 +24,7 @@ from google.auth import exceptions
 from google.auth.compute_engine import _metadata
 
 
-class Credentials(credentials.Scoped, credentials.Credentials):
+class Credentials(credentials.ReadOnnlyScoped, credentials.Credentials):
     """Compute Engine Credentials.
 
     These credentials use the Google Compute Engine metadata server to obtain
@@ -105,17 +105,3 @@ class Credentials(credentials.Scoped, credentials.Credentials):
     def requires_scopes(self):
         """False: Compute Engine credentials can not be scoped."""
         return False
-
-    def with_scopes(self, scopes):
-        """Unavailable, Compute Engine credentials can not be scoped.
-
-        Scopes can only be set at Compute Engine instance creation time.
-        See the `Compute Engine authentication documentation`_ for details on
-        how to configure instance scopes.
-
-        .. _Compute Engine authentication documentation:
-            https://cloud.google.com/compute/docs/authentication#using
-        """
-        raise NotImplementedError(
-            'Compute Engine credentials can not set scopes. Scopes must be '
-            'set when the Compute Engine instance is created.')
diff --git a/google/auth/credentials.py b/google/auth/credentials.py
index 74d6788..83683eb 100644
--- a/google/auth/credentials.py
+++ b/google/auth/credentials.py
@@ -123,8 +123,8 @@ class Credentials(object):
 
 
 @six.add_metaclass(abc.ABCMeta)
-class Scoped(object):
-    """Interface for scoped credentials.
+class ReadOnnlyScoped(object):
+    """Interface for credentials whose scopes can be queried.
 
     OAuth 2.0-based credentials allow limiting access using scopes as described
     in `RFC6749 Section 3.3`_.
@@ -152,7 +152,7 @@ class Scoped(object):
     .. _RFC6749 Section 3.3: https://tools.ietf.org/html/rfc6749#section-3.3
     """
     def __init__(self):
-        super(Scoped, self).__init__()
+        super(ReadOnnlyScoped, self).__init__()
         self._scopes = None
 
     @property
@@ -166,6 +166,46 @@ class Scoped(object):
         """
         return False
 
+    def has_scopes(self, scopes):
+        """Checks if the credentials have the given scopes.
+
+        .. warning: This method is not guaranteed to be accurate if the
+            credentials are :attr:`~Credentials.invalid`.
+
+        Returns:
+            bool: True if the credentials have the given scopes.
+        """
+        return set(scopes).issubset(set(self._scopes or []))
+
+
+class Scoped(ReadOnnlyScoped):
+    """Interface for credentials whose scopes can be replaced while copying.
+
+    OAuth 2.0-based credentials allow limiting access using scopes as described
+    in `RFC6749 Section 3.3`_.
+    If a credential class implements this interface then the credentials either
+    use scopes in their implementation.
+
+    Some credentials require scopes in order to obtain a token. You can check
+    if scoping is necessary with :attr:`requires_scopes`::
+
+        if credentials.requires_scopes:
+            # Scoping is required.
+            credentials = credentials.create_scoped(['one', 'two'])
+
+    Credentials that require scopes must either be constructed with scopes::
+
+        credentials = SomeScopedCredentials(scopes=['one', 'two'])
+
+    Or must copy an existing instance using :meth:`with_scopes`::
+
+        scoped_credentials = credentials.with_scopes(scopes=['one', 'two'])
+
+    Some credentials have scopes but do not allow or require scopes to be set,
+    these credentials can be used as-is.
+
+    .. _RFC6749 Section 3.3: https://tools.ietf.org/html/rfc6749#section-3.3
+    """
     @abc.abstractmethod
     def with_scopes(self, scopes):
         """Create a copy of these credentials with the specified scopes.
@@ -180,17 +220,6 @@ class Scoped(object):
         """
         raise NotImplementedError('This class does not require scoping.')
 
-    def has_scopes(self, scopes):
-        """Checks if the credentials have the given scopes.
-
-        .. warning: This method is not guaranteed to be accurate if the
-            credentials are :attr:`~Credentials.invalid`.
-
-        Returns:
-            bool: True if the credentials have the given scopes.
-        """
-        return set(scopes).issubset(set(self._scopes or []))
-
 
 def with_scopes_if_required(credentials, scopes):
     """Creates a copy of the credentials with scopes if scoping is required.


Test patch:
diff --git a/tests/compute_engine/test_credentials.py b/tests/compute_engine/test_credentials.py
index 732cb41..ae2597d 100644
--- a/tests/compute_engine/test_credentials.py
+++ b/tests/compute_engine/test_credentials.py
@@ -105,7 +105,3 @@ class TestCredentials(object):
 
         # Credentials should now be valid.
         assert self.credentials.valid
-
-    def test_with_scopes(self):
-        with pytest.raises(NotImplementedError):
-            self.credentials.with_scopes(['one', 'two'])
diff --git a/tests/test_credentials.py b/tests/test_credentials.py
index b5a540d..ae53cd9 100644
--- a/tests/test_credentials.py
+++ b/tests/test_credentials.py
@@ -77,22 +77,20 @@ def test_before_request():
     assert headers['authorization'] == 'Bearer token'
 
 
-class ScopedCredentialsImpl(credentials.Scoped, CredentialsImpl):
+class ReadOnnlyScopedCredentialsImpl(credentials.ReadOnnlyScoped,
+                                     CredentialsImpl):
     @property
     def requires_scopes(self):
-        return super(ScopedCredentialsImpl, self).requires_scopes
-
-    def with_scopes(self, scopes):
-        raise NotImplementedError
+        return super(ReadOnnlyScopedCredentialsImpl, self).requires_scopes
 
 
-def test_scoped_credentials_constructor():
-    credentials = ScopedCredentialsImpl()
+def test_readonly_scoped_credentials_constructor():
+    credentials = ReadOnnlyScopedCredentialsImpl()
     assert credentials._scopes is None
 
 
-def test_scoped_credentials_scopes():
-    credentials = ScopedCredentialsImpl()
+def test_readonly_scoped_credentials_scopes():
+    credentials = ReadOnnlyScopedCredentialsImpl()
     credentials._scopes = ['one', 'two']
     assert credentials.scopes == ['one', 'two']
     assert credentials.has_scopes(['one'])
@@ -101,8 +99,8 @@ def test_scoped_credentials_scopes():
     assert not credentials.has_scopes(['three'])
 
 
-def test_scoped_credentials_requires_scopes():
-    credentials = ScopedCredentialsImpl()
+def test_readonly_scoped_credentials_requires_scopes():
+    credentials = ReadOnnlyScopedCredentialsImpl()
     assert not credentials.requires_scopes