-
Notifications
You must be signed in to change notification settings - Fork 150
Moved attestation factory back to static method on attestation class … #3682
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.
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
Changes from all commits
2c1a53e
938cd4b
4edf95a
51a8541
db81845
016fd6f
b327f04
8043d27
2db50e7
723c241
027654d
c579e4f
16490c8
3459711
862692f
adb9370
4e6f598
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,17 +2,12 @@ | |||||||||||||||||||
|
|
||||||||||||||||||||
| ## 1.0.0-beta.3 (Unreleased) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ### Features Added | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ### Breaking Changes | ||||||||||||||||||||
| - `ValueToSend` field in `TpmAttestationOptions` becomes `Payload`. | ||||||||||||||||||||
| - `AddIsolatedModeCertificatesOptions` becomes `AddIsolatedModeCertificateOptions` | ||||||||||||||||||||
| - `RemoveIsolatedModeCertificatesOptions` becomes `RemoveIsolatedModeCertificateOptions` | ||||||||||||||||||||
| - Renamed `AttestEnclaveOptions` to `AttestSgxEnclaveOptions` and `AttestOpenEnclaveOptions`. | ||||||||||||||||||||
| - Split out `AttestationClient::Create` into its own factory class `AttestationClientFactory`. | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't we also remove
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in the previous CR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean? If it was added and removed during this release, then ignore my comment :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like it was always private. We might want to update the docs on public API that reference the method still, because the end user can't call it: Line 334 in 0fd0267
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was public for 1.0.0.beta-1, and was removed for either 1.0.0.beta-2 or 1.0.0.beta-3 (not sure which). Could you show me the public API documentation for this method? I thought I had searched for all of them and removed them, but it's possible I missed a couple.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a couple places (I linked the GitHub search of the codebase above), here's an example: Lines 98 to 105 in ebe084b
|
||||||||||||||||||||
| - Note that the `AttestationClientFactory::Create` method returns a `std::unique_ptr` to the client object. | ||||||||||||||||||||
| - Split out `AttestationAdministrationClient::Create` into its own factory class `AttestationAdministrationClientFactory`. | ||||||||||||||||||||
| - Note that the `AttestationAdministrationClientFactory::Create` method returns a `std::unique_ptr` to the client object. | ||||||||||||||||||||
| - `AttestationClient` and `AttestationAdministrationClient` creation is now done using the factory method `AttestationClient::Create()` and `AttestationAdministrationClient::Create()`. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ### Bugs Fixed | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,9 +44,23 @@ namespace Azure { namespace Security { namespace Attestation { | |
| * | ||
| */ | ||
| class AttestationAdministrationClient final { | ||
| friend class AttestationAdministrationClientFactory; | ||
|
|
||
| public: | ||
| /** | ||
| * @brief Construct a new Attestation Administration Client object. | ||
| * | ||
| * @param endpoint The URL address where the client will send the requests to. | ||
| * @param credential The authentication token to use. | ||
| * @param options The options to customize the client behavior. | ||
| * @return The newly created client. | ||
| */ | ||
| static AttestationAdministrationClient Create( | ||
LarryOsterman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| std::string const& endpoint, | ||
| std::shared_ptr<Core::Credentials::TokenCredential const> credential, | ||
| AttestationAdministrationClientOptions const& options | ||
| = AttestationAdministrationClientOptions{}, | ||
| Azure::Core::Context const& context = Azure::Core::Context{}); | ||
|
|
||
| /** | ||
| * @brief Construct a new Attestation Administration Client object from another attestation | ||
| * administration client. | ||
|
|
@@ -56,7 +70,8 @@ namespace Azure { namespace Security { namespace Attestation { | |
| AttestationAdministrationClient(AttestationAdministrationClient const& attestationClient) | ||
| : m_endpoint(attestationClient.m_endpoint), m_apiVersion(attestationClient.m_apiVersion), | ||
| m_pipeline(attestationClient.m_pipeline), | ||
| m_tokenValidationOptions(attestationClient.m_tokenValidationOptions){}; | ||
| m_tokenValidationOptions(attestationClient.m_tokenValidationOptions), | ||
| m_attestationSigners(attestationClient.m_attestationSigners){}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breaking change? If so, add CL entry.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All this does is copy a member variable in the copy constructor, is that breaking?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this adding a new mandatory parameter, that wasn't there before? |
||
|
|
||
| /** | ||
| * @brief Destructor. | ||
|
|
@@ -255,6 +270,36 @@ namespace Azure { namespace Security { namespace Attestation { | |
|
|
||
| std::vector<Models::AttestationSigner> m_attestationSigners; | ||
|
|
||
| /** | ||
| * @brief Construct a new Attestation Administration Client object. | ||
| * | ||
| * @param endpoint The URL address where the client will send the requests to. | ||
| * @param credential The authentication token to use. | ||
| * @param options The options to customize the client behavior. | ||
| * @return The newly created client. | ||
| */ | ||
| static AttestationAdministrationClient CreateConcrete( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove these if no longer needed. |
||
| std::string const& endpoint, | ||
| std::shared_ptr<Core::Credentials::TokenCredential const> credential, | ||
| AttestationAdministrationClientOptions const& options | ||
| = AttestationAdministrationClientOptions{}, | ||
| Azure::Core::Context const& context = Azure::Core::Context{}); | ||
|
|
||
| /** | ||
| * @brief Construct a new Attestation Administration Client object. | ||
| * | ||
| * @param endpoint The URL address where the client will send the requests to. | ||
| * @param credential The authentication token to use. | ||
| * @param options The options to customize the client behavior. | ||
| * @return The newly created client. | ||
| */ | ||
| static std::unique_ptr<AttestationAdministrationClient> CreatePointer( | ||
| std::string const& endpoint, | ||
| std::shared_ptr<Core::Credentials::TokenCredential const> credential, | ||
| AttestationAdministrationClientOptions const& options | ||
| = AttestationAdministrationClientOptions{}, | ||
| Azure::Core::Context const& context = Azure::Core::Context{}); | ||
|
|
||
| /** | ||
| * @brief Construct a new Attestation Administration Client object. | ||
| * | ||
|
|
@@ -289,29 +334,4 @@ namespace Azure { namespace Security { namespace Attestation { | |
| void RetrieveResponseValidationCollateral( | ||
| Azure::Core::Context const& context = Azure::Core::Context{}); | ||
| }; | ||
|
|
||
| /** @brief Construct a new AttestationAdministrationClient object. | ||
| * | ||
| * The AttestationAdministrationClientFactory class is a factory class for instantiating new | ||
| * AttestationAdministrationClient objects. | ||
| * | ||
| */ | ||
| class AttestationAdministrationClientFactory final { | ||
| public: | ||
| /** | ||
| * @brief Construct a new Attestation Administration Client object. | ||
| * | ||
| * @param endpoint The URL address where the client will send the requests to. | ||
| * @param credential The authentication token to use. | ||
| * @param options The options to customize the client behavior. | ||
| * @return std::unique_ptr<AttestationAdministrationClient> The newly created client. | ||
| */ | ||
| static std::unique_ptr<AttestationAdministrationClient> Create( | ||
| std::string const& endpoint, | ||
| std::shared_ptr<Core::Credentials::TokenCredential const> credential, | ||
| AttestationAdministrationClientOptions const& options | ||
| = AttestationAdministrationClientOptions{}, | ||
| Azure::Core::Context const& context = Azure::Core::Context{}); | ||
| }; | ||
|
|
||
| }}} // namespace Azure::Security::Attestation | ||
Uh oh!
There was an error while loading. Please reload this page.