turcsanyip commented on PR #10482: URL: https://github.com/apache/nifi/pull/10482#issuecomment-3732886010
@pvillard31 On a more detailed review, I would suggest the following changes: 1. Using `ClientAssertionCredential`: `StandardAzureIdentityFederationTokenProvider` performs direct HTTP requests to exchange the client assertion token for an access token. The Azure client lib provides a higher level API via [ClientAssertionCredential](https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/identity/azure-identity/src/main/java/com/azure/identity/ClientAssertionCredential.java) (similar to [IdentityPoolCredentials](https://github.com/apache/nifi/blob/98b80108439ca9dabeb2343f3c50817d2ecee2f6/nifi-extension-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/credentials/factory/strategies/WorkloadIdentityFederationCredentialsStrategy.java#L65-L70) in GCP) which is more convenient and robust to achieve the same functionality. 2. `AzureIdentityFederationTokenProvider` should not extend `OAuth2AccessTokenProvider`: Being an `OAuth2AccessTokenProvider` implementation, the controller service could be referenced in any processor that has OAuth dependency (e.g. `QuerySalesforceObject`) where the Azure-specific service is not appropriate. This relates to point (1) as well because if the service is refactored to use the credential object instead of the raw access token, then its interface will be changed and does not follow `OAuth2AccessTokenProvider` anymore. 3. Due to the changes in `AzureIdentityFederationTokenProvider` functionality and weight, a simple util class might be enough for building the credential object from component properties. No strong opinions here though. 4. Separating the Identity Federation and the Access Token options both on the UI and in the code: The components in the current implementation refer the new authentication strategy with different names: `Identity Federation`, `OAuth2 Access Token` or `Access Token`. Using a consistent name like `Identity Federation` or `Workload Identity Federation` would provide a better UX. I would not combine it with Access Token at code level either, mostly in the light of point (1) which changes the raw access token to a credential object. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
