Azure / azure-sdk-for-cpp

This repository is for active development of the Azure SDK for C++. For consumers of the SDK we recommend visiting our versioned developer docs at https://azure.github.io/azure-sdk-for-cpp.
MIT License
181 stars 126 forks source link

Resolve the difference in Account Sas Permissions enums between C++ and languages, only keep the values needed by Tables, and make the order consistent #6170

Closed ahsonkhan closed 1 week ago

ahsonkhan commented 3 weeks ago

Here's the current AccountSasPermissions enum values in C++: https://github.com/Azure/azure-sdk-for-cpp/blob/d835e1a3b1970e53ceda94ef1c6d0596cd103d02/sdk/tables/azure-data-tables/inc/azure/data/tables/account_sas_builder.hpp#L92-L128

1) The order in which the enum values are sent to the REST API matter, and should be consistent with other languages https://learn.microsoft.com/en-us/rest/api/storageservices/create-account-sas#table-service Under the covers, both .NET and C++ rightly use the order rwdlau. Therefore, the minimal change needed here, is to reorder List and Add in the publicly exposed enum, making List = 8, and Add = 16 (consistent with implementation detail ordering expected by the service, and .NET) 2) Some of the other langauges add other values such as Create, Process, and Query because the same enum object is used across the storage and tables service. C++ has separate enum values, so we likely don't need to include those values here. We should only expose the values needed by the Tables service. Verify that rwdlau are all that's needed, and none of those need to be removed.

Here's how we append the permissions ordering: https://github.com/Azure/azure-sdk-for-cpp/blob/cfd1a433ea812320bb454f57fda2c088b33e9d52/sdk/tables/azure-data-tables/src/account_sas_builder.cpp#L19-L43

It aligns with how .NET constructs the SAS string, in the correct order: https://github.com/Azure/azure-sdk-for-net/blob/42e72d78e79c7e2568a987cef287439768d40fb4/sdk/tables/Azure.Data.Tables/src/Sas/TableAccountSasPermissions.cs#L70-L95

From @annatisch:

In Python - the AccountSasPermissions object is shared between all the storage services - so there's probably permissions there that don't specifically apply to the Tables service. And in fact in the docs for that object, create is only for blobs and files, add is only for append blobs and queues, update and process are only for queues. Although order is important, so if you're not making all of these permission options public in tables because they're not all applicable, the internal order still needs to be maintained.

From @christothes:

the values or the enum are not important, just the ordering - they are ultimately translated to one or more of these char values in the SAS token URL https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#permissions-for-a-table

Reference links for comparison, here are the values exposed by the other language SDKs: .NET - https://learn.microsoft.com/en-us/dotnet/api/azure.data.tables.sas.tableaccountsaspermissions?view=azure-dotnet#fields Java - https://learn.microsoft.com/en-us/java/api/com.azure.data.tables.sas.tableaccountsaspermission?view=azure-java-stable Python - https://github.com/Azure/azure-sdk-for-python/blob/dc2e258971f009c92c3f5a872d8f759bbdcdca47/sdk/tables/azure-data-tables/azure/data/tables/_models.py#L593-L600 JS - https://github.com/Azure/azure-sdk-for-js/blob/d74b28cf794bc85ac95142867d4cc49f95982d3e/sdk/tables/data-tables/src/sas/accountSasPermissions.ts#L82-L112 GoLang - https://github.com/Azure/azure-sdk-for-go/blob/c4e56191456c45f561be3f20d1e92e9e2291ed1b/sdk/data/aztables/sas_account.go#L86-L95

LarryOsterman commented 3 weeks ago

I thought that during the discussion with the other language partners, the actual numeric value of the enumerations didn't matter, the ONLY thing that mattered was the order that the various letters appeared in the SaS token string.

So the "List" permission doesn't have to have a numeric value of 32 - it be any power of two, as long as when the permissions string is generated, the 'l' value appears between 'd' and 'a' in the permission string.

gearama commented 1 week ago

ti is done