Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed issues with resource token #13855

Merged
merged 13 commits into from
Sep 24, 2020
Merged

Fixed issues with resource token #13855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ebe0815
fixed bug in querying by page using continuation token
Aug 24, 2020
205475d
updated changelog
Aug 24, 2020
0c96494
modified changelog
Aug 24, 2020
98d1776
added support from single partition query continuation tokens
Aug 26, 2020
a141470
fixed linting error
Aug 26, 2020
a0973da
addressed PR comments
Aug 26, 2020
62b612b
Fixed resource token issues
Sep 18, 2020
7dfa67e
Merge branch 'users/srnara/contTokenFIx' into users/srnara/resourceTo…
Sep 18, 2020
89582ce
fixed linting error
Sep 18, 2020
ac9f072
sample commit
Sep 18, 2020
ac66ecf
commit reverted
Sep 18, 2020
6b42b9e
fixed failing tests
Sep 18, 2020
b8f58ba
updated changelog
Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/cosmos/azure-cosmos/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

**Bug fixes**
- Fixed bug where continuation token is not honored when query_iterable is used to get results by page. Issue #13265.
- Fixed bug where resource tokens not being honored for document reads and deletes. Issue #13634.


## 4.1.0 (2020-08-10)
Expand Down
12 changes: 8 additions & 4 deletions sdk/cosmos/azure-cosmos/azure/cosmos/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ def __GetAuthorizationTokenUsingResourceTokens(resource_tokens, path, resource_i
# For database account access(through GetDatabaseAccount API), path and
# resource_id_or_fullname are '', so in this case we return the first token to be
# used for creating the auth header as the service will accept any token in this case
path = six.moves.urllib.parse.unquote(path)
if not path and not resource_id_or_fullname:
return next(six.itervalues(resource_tokens))

Expand All @@ -126,7 +127,7 @@ def __GetAuthorizationTokenUsingResourceTokens(resource_tokens, path, resource_i

path_parts = []
if path:
path_parts = path.split("/")
path_parts = [item for item in path.split("/") if item]
resource_types = [
"dbs",
"colls",
Expand All @@ -140,9 +141,12 @@ def __GetAuthorizationTokenUsingResourceTokens(resource_tokens, path, resource_i
"conflicts",
"offers",
]

# Get the last resource id or resource name from the path and get it's token from resource_tokens
for one_part in reversed(path_parts):
if not one_part in resource_types and one_part in resource_tokens:
return resource_tokens[one_part]
for i in range(len(path_parts), 1, -1):
segment = path_parts[i-1]
srinathnarayanan marked this conversation as resolved.
Show resolved Hide resolved
sub_path = "/".join(path_parts[:i])
if not segment in resource_types and sub_path in resource_tokens:
return resource_tokens[sub_path]

return None
158 changes: 68 additions & 90 deletions sdk/cosmos/azure-cosmos/test/test_crud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,21 +531,21 @@ def test_partitioned_collection_document_crud_and_query(self):
def test_partitioned_collection_permissions(self):
created_db = self.databaseForTest

collection_id = 'test_partitioned_collection_permissions all collection'
collection_id = 'test_partitioned_collection_permissions all collection' + str(uuid.uuid4())

all_collection = created_db.create_container(
id=collection_id,
partition_key=PartitionKey(path='/key', kind=documents.PartitionKind.Hash)
)

collection_id = 'test_partitioned_collection_permissions read collection'
collection_id = 'test_partitioned_collection_permissions read collection' + str(uuid.uuid4())

read_collection = created_db.create_container(
id=collection_id,
partition_key=PartitionKey(path='/key', kind=documents.PartitionKind.Hash)
)

user = created_db.create_user(body={'id': 'user'})
user = created_db.create_user(body={'id': 'user' + str(uuid.uuid4())})

permission_definition = {
'id': 'all permission',
Expand All @@ -567,8 +567,8 @@ def test_partitioned_collection_permissions(self):

resource_tokens = {}
# storing the resource tokens based on Resource IDs
resource_tokens[urllib.quote(all_collection.id)] = (all_permission.properties['_token'])
resource_tokens[urllib.quote(read_collection.id)] = (read_permission.properties['_token'])
resource_tokens["dbs/" + created_db.id + "/colls/" + all_collection.id] = (all_permission.properties['_token'])
resource_tokens["dbs/" + created_db.id + "/colls/" + read_collection.id] = (read_permission.properties['_token'])
Copy link
Member

Choose a reason for hiding this comment

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

Was the existing test invalid in some way?
Rather than alter existing tests, we should simply add new ones - we don't want to inadvertently break existing customers. All the existing tests should pass without modification (unless the tests were never correct in the first place).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the way in which resource tokens were implemented were faulty. resource tokens are supposed to be a map of resource link to token . i.e {"dbs/mydb/coll/mycoll1": "token1", "dbs/mydb/colls/mycoll2": "token2"} (this is the implementation in the .Net SDK and is the spec as well). But in the python sdk currently, we expect to get it to be a map of resource id to token. i.e {"mycoll1": "token1", "mycoll2": "token2"}. This is faulty since the same collection name can exist in 2 different databases.
These tests also follow the old pattern. They will break with the new changes, so I changed the tests as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, but is it possible any customers were using the old faulty pattern? For example by manually building this dictionary? Or if they had done that would it not have worked?

Copy link
Contributor

Choose a reason for hiding this comment

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

@annatisch It is possible that some cases would have worked but that is not intentional. We consider it a bug and fixed the same issue in JS SDK recently.

Previous versions of the Cosmos SDK were all based on rid which was a unique identifier resource. The transition to user supplied ids a couple years ago changed that, but we didn't fix resource tokens to account for it. Given all this, and the fact that resource tokens do not see a lot of usage, I am fine with merging this as a bugfix.

Copy link
Member

Choose a reason for hiding this comment

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

I would still prefer this to be backwards compatible - but if you've rolled out the same change in other languages I guess that's okay.


restricted_client = cosmos_client.CosmosClient(
CRUDTests.host, resource_tokens, "Session", connection_policy=CRUDTests.connectionPolicy)
Expand Down Expand Up @@ -1302,71 +1302,48 @@ def __SetupEntities(client):
"""
# create database
db = self.databaseForTest
# create collection1
collection1 = db.create_container(
# create collection
collection = db.create_container(
id='test_authorization' + str(uuid.uuid4()),
partition_key=PartitionKey(path='/id', kind='Hash')
)
# create document1
document1 = collection1.create_item(
body={'id': 'coll1doc1',
document = collection.create_item(
body={'id': 'doc1',
'spam': 'eggs',
'key': 'value'},
)
# create document 2
document2 = collection1.create_item(
body={'id': 'coll1doc2', 'spam': 'eggs2', 'key': 'value2'}
)

# create collection 2
collection2 = db.create_container(
id='test_authorization2' + str(uuid.uuid4()),
partition_key=PartitionKey(path='/id', kind='Hash')
)
# create user1
user1 = db.create_user(body={'id': 'user1'})
collection1_properties = collection1.read()
# create user
user = db.create_user(body={'id': 'user' + str(uuid.uuid4())})

# create permission for collection
permission = {
'id': 'permission On Coll1',
'id': 'permission On Coll',
'permissionMode': documents.PermissionMode.Read,
'resource': collection1_properties['_self']
}
# create permission for collection1
permission_on_coll1 = user1.create_permission(body=permission)
self.assertIsNotNone(permission_on_coll1.properties['_token'],
'permission token is invalid')
permission = {
'id': 'permission On Doc1',
'permissionMode': documents.PermissionMode.All,
'resource': document2['_self']
'resource': "dbs/" + db.id + "/colls/" + collection.id
}
# create permission for document 2
permission_on_doc2 = user1.create_permission(body=permission)
self.assertIsNotNone(permission_on_doc2.properties['_token'],
permission_on_coll = user.create_permission(body=permission)
self.assertIsNotNone(permission_on_coll.properties['_token'],
'permission token is invalid')
# create user 2
user2 = db.create_user(body={'id': 'user2'})
collection2_properties = collection2.read()

# create permission for document
permission = {
'id': 'permission On coll2',
'id': 'permission On Doc',
'permissionMode': documents.PermissionMode.All,
'resource': collection2_properties['_self']
'resource': "dbs/" + db.id + "/colls/" + collection.id + "/docs/" + document["id"]
}
# create permission on collection 2
permission_on_coll2 = user2.create_permission(body=permission)
self.assertIsNotNone(permission_on_coll2.properties['_token'],
permission_on_doc = user.create_permission(body=permission)
self.assertIsNotNone(permission_on_doc.properties['_token'],
'permission token is invalid')

entities = {
'db': db,
'coll1': collection1,
'coll2': collection2,
'doc1': document1,
'doc2': document2,
'user1': user1,
'user2': user2,
'permissionOnColl1': permission_on_coll1,
'permissionOnDoc2': permission_on_doc2,
'permissionOnColl2': permission_on_coll2
'coll': collection,
'doc': document,
'user': user,
'permissionOnColl': permission_on_coll,
'permissionOnDoc': permission_on_doc,
}
return entities

Expand All @@ -1382,60 +1359,61 @@ def __SetupEntities(client):
connection_policy=CRUDTests.connectionPolicy)
# setup entities
entities = __SetupEntities(client)
resource_tokens = {}
resource_tokens[entities['coll1'].id] = (
entities['permissionOnColl1'].properties['_token'])
resource_tokens[entities['doc1']['id']]= (
entities['permissionOnColl1'].properties['_token'])
col1_client = cosmos_client.CosmosClient(
resource_tokens = {"dbs/" + entities['db'].id + "/colls/" + entities['coll'].id:
entities['permissionOnColl'].properties['_token']}
col_client = cosmos_client.CosmosClient(
CRUDTests.host, resource_tokens,"Session", connection_policy=CRUDTests.connectionPolicy)
db = entities['db']

old_client_connection = db.client_connection
db.client_connection = col1_client.client_connection
# 1. Success-- Use Col1 Permission to Read
success_coll1 = db.get_container_client(container=entities['coll1'])
# 2. Failure-- Use Col1 Permission to delete
db.client_connection = col_client.client_connection
# 1. Success-- Use Col Permission to Read
success_coll = db.get_container_client(container=entities['coll'])
# 2. Failure-- Use Col Permission to delete
self.__AssertHTTPFailureWithStatus(StatusCodes.FORBIDDEN,
db.delete_container,
success_coll1)
# 3. Success-- Use Col1 Permission to Read All Docs
success_documents = list(success_coll1.read_all_items())
success_coll)
# 3. Success-- Use Col Permission to Read All Docs
success_documents = list(success_coll.read_all_items())
self.assertTrue(success_documents != None,
'error reading documents')
self.assertEqual(len(success_documents),
2,
'Expected 2 Documents to be succesfully read')
# 4. Success-- Use Col1 Permission to Read Col1Doc1
success_doc = success_coll1.read_item(
item=entities['doc1']['id'],
partition_key=entities['doc1']['id']
1,
'Expected 1 Document to be succesfully read')
# 4. Success-- Use Col Permission to Read Doc

docId = entities['doc']['id']
success_doc = success_coll.read_item(
item=docId,
partition_key=docId
)
self.assertTrue(success_doc != None, 'error reading document')
self.assertEqual(
success_doc['id'],
entities['doc1']['id'],
entities['doc']['id'],
'Expected to read children using parent permissions')
col2_client = cosmos_client.CosmosClient(
CRUDTests.host,
[entities['permissionOnColl2'].properties],
"Session",
connection_policy=CRUDTests.connectionPolicy)
doc = {
'CustomProperty1': 'BBBBBB',
'customProperty2': 1000,
'id': entities['doc2']['id']
}
entities['coll2'].client_connection = col2_client.client_connection
success_doc = entities['coll2'].create_item(body=doc)
self.assertTrue(success_doc != None, 'error creating document')
self.assertEqual(success_doc['CustomProperty1'],
doc['CustomProperty1'],
'document should have been created successfully')

#5. Failure-- Use Col Permission to Delete Doc
self.__AssertHTTPFailureWithStatus(StatusCodes.FORBIDDEN,
success_coll.delete_item,
docId, docId)

resource_tokens = {"dbs/" + entities['db'].id + "/colls/" + entities['coll'].id + "/docs/" + docId :
entities['permissionOnDoc'].properties['_token']}

doc_client = cosmos_client.CosmosClient(
CRUDTests.host, resource_tokens,"Session", connection_policy=CRUDTests.connectionPolicy)

#6. Success-- Use Doc permission to read doc
read_doc = doc_client.get_database_client(db.id).get_container_client(success_coll.id).read_item(docId, docId)
self.assertEqual(read_doc["id"], docId)

#6. Success-- Use Doc permission to delete doc
doc_client.get_database_client(db.id).get_container_client(success_coll.id).delete_item(docId, docId)
self.assertEqual(read_doc["id"], docId)

db.client_connection = old_client_connection
db.delete_container(entities['coll1'])
db.delete_container(entities['coll2'])
db.delete_container(entities['coll'])

def test_trigger_crud(self):
# create database
Expand Down