Skip to content

Commit 9f92042

Browse files
dhermesdhermes
committed
Merge pull request #1377 from dhermes/fix-1050
Only allow one entity to come from a BQ access grant.
2 parents ffee6e7 + fa34466 commit 9f92042

2 files changed

Lines changed: 104 additions & 43 deletions

File tree

‎gcloud/bigquery/dataset.py‎

Lines changed: 47 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,53 @@
2323
class AccessGrant(object):
2424
"""Represent grant of an access role to an entity.
2525
26+
Every entry in the access list will have exactly one of
27+
``userByEmail``, ``groupByEmail``, ``domain``, ``specialGroup`` or
28+
``view`` set. And if anything but ``view`` is set, it'll also have a
29+
``role`` specified. ``role`` is omitted for a ``view``, since
30+
``view`` s are always read-only.
31+
32+
See https://cloud.google.com/bigquery/docs/reference/v2/datasets.
33+
2634
:type role: string
2735
:param role: Role granted to the entity. One of
2836
2937
* ``'OWNER'``
3038
* ``'WRITER'``
3139
* ``'READER'``
3240
41+
May also be ``None`` if the ``entity_type`` is ``view``.
42+
3343
:type entity_type: string
3444
:param entity_type: Type of entity being granted the role. One of
35-
36-
* ``'specialGroup'``
37-
* ``'groupByEmail'``
38-
* ``'userByEmail'``
45+
:attr:`ENTITY_TYPES`.
3946
4047
:type entity_id: string
4148
:param entity_id: ID of entity being granted the role.
49+
50+
:raises: :class:`ValueError` if the ``entity_type`` is not among
51+
:attr:`ENTITY_TYPES`, or if a ``view`` has ``role`` set or
52+
a non ``view`` **does not** have a ``role`` set.
4253
"""
54+
55+
ENTITY_TYPES = frozenset(['userByEmail', 'groupByEmail', 'domain',
56+
'specialGroup', 'view'])
57+
"""Allowed entity types."""
58+
4359
def __init__(self, role, entity_type, entity_id):
60+
if entity_type not in self.ENTITY_TYPES:
61+
message = 'Entity type %r not among: %s' % (
62+
entity_type, ', '.join(self.ENTITY_TYPES))
63+
raise ValueError(message)
64+
if entity_type == 'view':
65+
if role is not None:
66+
raise ValueError('Role must be None for a view. Received '
67+
'role: %r' % (role,))
68+
else:
69+
if role is None:
70+
raise ValueError('Role must be set for entity '
71+
'type %r' % (entity_type,))
72+
4473
self.role = role
4574
self.entity_type = entity_type
4675
self.entity_id = entity_id
@@ -297,22 +326,26 @@ def _require_client(self, client):
297326
def _parse_access_grants(access):
298327
"""Parse a resource fragment into a set of access grants.
299328
329+
``role`` augments the entity type and present **unless** the entity
330+
type is ``view``.
331+
300332
:type access: list of mappings
301333
:param access: each mapping represents a single access grant
302334
303335
:rtype: list of :class:`AccessGrant`
304336
:returns: a list of parsed grants
337+
:raises: :class:`ValueError` if a grant in ``access`` has more keys
338+
than ``role`` and one additional key.
305339
"""
306340
result = []
307341
for grant in access:
308342
grant = grant.copy()
309-
role = grant.pop('role')
310-
# Hypothetical case: we don't know that the back-end will ever
311-
# return such structures, but they are logical. See:
312-
# https://github.com/GoogleCloudPlatform/gcloud-python/pull/1046#discussion_r36687769
313-
for entity_type, entity_id in sorted(grant.items()):
314-
result.append(
315-
AccessGrant(role, entity_type, entity_id))
343+
role = grant.pop('role', None)
344+
entity_type, entity_id = grant.popitem()
345+
if len(grant) != 0:
346+
raise ValueError('Grant has unexpected keys remaining.', grant)
347+
result.append(
348+
AccessGrant(role, entity_type, entity_id))
316349
return result
317350

318351
def _set_properties(self, api_response):
@@ -335,7 +368,9 @@ def _build_access_resource(self):
335368
"""Generate a resource fragment for dataset's access grants."""
336369
result = []
337370
for grant in self.access_grants:
338-
info = {'role': grant.role, grant.entity_type: grant.entity_id}
371+
info = {grant.entity_type: grant.entity_id}
372+
if grant.role is not None:
373+
info['role'] = grant.role
339374
result.append(info)
340375
return result
341376

‎gcloud/bigquery/test_dataset.py‎

Lines changed: 57 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,31 @@ def test_ctor_defaults(self):
3030
self.assertEqual(grant.entity_type, 'userByEmail')
3131
self.assertEqual(grant.entity_id, 'phred@example.com')
3232

33+
def test_ctor_bad_entity_type(self):
34+
with self.assertRaises(ValueError):
35+
self._makeOne(None, 'unknown', None)
36+
37+
def test_ctor_view_with_role(self):
38+
role = 'READER'
39+
entity_type = 'view'
40+
with self.assertRaises(ValueError):
41+
self._makeOne(role, entity_type, None)
42+
43+
def test_ctor_view_success(self):
44+
role = None
45+
entity_type = 'view'
46+
entity_id = object()
47+
grant = self._makeOne(role, entity_type, entity_id)
48+
self.assertEqual(grant.role, role)
49+
self.assertEqual(grant.entity_type, entity_type)
50+
self.assertEqual(grant.entity_id, entity_id)
51+
52+
def test_ctor_nonview_without_role(self):
53+
role = None
54+
entity_type = 'userByEmail'
55+
with self.assertRaises(ValueError):
56+
self._makeOne(role, entity_type, None)
57+
3358

3459
class TestDataset(unittest2.TestCase):
3560
PROJECT = 'project'
@@ -249,39 +274,27 @@ def test_from_api_repr_w_properties(self):
249274
self._verifyResourceProperties(dataset, RESOURCE)
250275

251276
def test__parse_access_grants_w_unknown_entity_type(self):
252-
USER_EMAIL = 'phred@example.com'
253-
GROUP_EMAIL = 'group-name@lists.example.com'
254-
RESOURCE = {
255-
'access': [
256-
{'role': 'OWNER', 'userByEmail': USER_EMAIL},
257-
{'role': 'WRITER', 'groupByEmail': GROUP_EMAIL},
258-
{'role': 'READER', 'specialGroup': 'projectReaders'},
259-
{'role': 'READER', 'unknown': 'UNKNOWN'}]
260-
}
277+
ACCESS = [
278+
{'role': 'READER', 'unknown': 'UNKNOWN'},
279+
]
261280
client = _Client(self.PROJECT)
262281
dataset = self._makeOne(self.DS_NAME, client=client)
263-
grants = dataset._parse_access_grants(RESOURCE['access'])
264-
self._verifyAccessGrants(grants, RESOURCE)
282+
with self.assertRaises(ValueError):
283+
dataset._parse_access_grants(ACCESS)
265284

266-
def test__parse_access_grants_w_multiple_entity_types(self):
267-
# Hypothetical case: we don't know that the back-end will ever
268-
# return such structures, but they are logical. See:
269-
# https://github.com/GoogleCloudPlatform/gcloud-python/pull/1046#discussion_r36687769
285+
def test__parse_access_grants_w_extra_keys(self):
270286
USER_EMAIL = 'phred@example.com'
271-
OTHER_EMAIL = 'bharney@example.com'
272-
GROUP_EMAIL = 'group-name@lists.example.com'
273-
RESOURCE = {
274-
'access': [
275-
{'role': 'OWNER', 'userByEmail': USER_EMAIL},
276-
{'role': 'WRITER', 'groupByEmail': GROUP_EMAIL},
277-
{'role': 'READER',
278-
'specialGroup': 'projectReaders',
279-
'userByEmail': OTHER_EMAIL}]
280-
}
287+
ACCESS = [
288+
{
289+
'role': 'READER',
290+
'specialGroup': 'projectReaders',
291+
'userByEmail': USER_EMAIL,
292+
},
293+
]
281294
client = _Client(self.PROJECT)
282295
dataset = self._makeOne(self.DS_NAME, client=client)
283-
grants = dataset._parse_access_grants(RESOURCE['access'])
284-
self._verifyAccessGrants(grants, RESOURCE)
296+
with self.assertRaises(ValueError):
297+
dataset._parse_access_grants(ACCESS)
285298

286299
def test_create_w_bound_client(self):
287300
PATH = 'projects/%s/datasets' % self.PROJECT
@@ -320,11 +333,19 @@ def test_create_w_alternate_client(self):
320333
dataset = self._makeOne(self.DS_NAME, client=CLIENT1)
321334
dataset.friendly_name = TITLE
322335
dataset.description = DESCRIPTION
336+
VIEW = {
337+
'projectId': 'my-proj',
338+
'datasetId': 'starry-skies',
339+
'tableId': 'northern-hemisphere',
340+
}
323341
dataset.access_grants = [
324342
AccessGrant('OWNER', 'userByEmail', USER_EMAIL),
325343
AccessGrant('OWNER', 'groupByEmail', GROUP_EMAIL),
344+
AccessGrant('READER', 'domain', 'foo.com'),
326345
AccessGrant('READER', 'specialGroup', 'projectReaders'),
327-
AccessGrant('WRITER', 'specialGroup', 'projectWriters')]
346+
AccessGrant('WRITER', 'specialGroup', 'projectWriters'),
347+
AccessGrant(None, 'view', VIEW),
348+
]
328349

329350
dataset.create(client=CLIENT2)
330351

@@ -334,15 +355,20 @@ def test_create_w_alternate_client(self):
334355
self.assertEqual(req['method'], 'POST')
335356
self.assertEqual(req['path'], '/%s' % PATH)
336357
SENT = {
337-
'datasetReference':
338-
{'projectId': self.PROJECT, 'datasetId': self.DS_NAME},
358+
'datasetReference': {
359+
'projectId': self.PROJECT,
360+
'datasetId': self.DS_NAME,
361+
},
339362
'description': DESCRIPTION,
340363
'friendlyName': TITLE,
341364
'access': [
342365
{'role': 'OWNER', 'userByEmail': USER_EMAIL},
343366
{'role': 'OWNER', 'groupByEmail': GROUP_EMAIL},
367+
{'role': 'READER', 'domain': 'foo.com'},
344368
{'role': 'READER', 'specialGroup': 'projectReaders'},
345-
{'role': 'WRITER', 'specialGroup': 'projectWriters'}],
369+
{'role': 'WRITER', 'specialGroup': 'projectWriters'},
370+
{'view': VIEW},
371+
],
346372
}
347373
self.assertEqual(req['data'], SENT)
348374
self._verifyResourceProperties(dataset, RESOURCE)

0 commit comments

Comments
 (0)