Modify quick pulse code with authentication enabled#1678
Conversation
|
|
||
| public static void init(AuthenticationType authenticationType, String clientId, String keePassDatabasePath, | ||
| String tenantId, String clientSecret, String authorityHost) { | ||
| AadAuthentication.instance = new AadAuthentication(authenticationType, clientId, keePassDatabasePath, tenantId, |
There was a problem hiding this comment.
i know this will get called only once.. do we still need to prevent user from creating a new instance of AadAuthentication each time when init is called?
| package com.microsoft.applicationinsights.internal.authentication; | ||
|
|
||
| public enum AuthenticationType { | ||
| UAMI, SAMI, INTELLIJ, VSCODE, CLIENTSECRET |
There was a problem hiding this comment.
Can we document what each type is?
There was a problem hiding this comment.
link to the azure doc will be helpful.
|
This pull request introduces 1 alert when merging 503582d into 83b96b6 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 9c17075 into 83b96b6 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging d123841 into 83b96b6 - view on LGTM.com new alerts:
|
|
|
||
| // handle AAD authentication | ||
| // TODO handle authentication exceptions | ||
| HttpPipelinePolicy authenticationPolicy = AadAuthentication.getInstance().getAuthenticationPolicy(); |
There was a problem hiding this comment.
won't this fail auth not configured? getInstance() throws exception?
There was a problem hiding this comment.
Yes if we call init() only when authentication is enabled. for now I removed the auth enabled check to create the authentication object. But we will end up with object with nullable variables.
|
This pull request introduces 1 alert when merging 8602098 into 1ffad2b - view on LGTM.com new alerts:
|
trask
left a comment
There was a problem hiding this comment.
did u mean to update the submodule in this PR?
|
This pull request introduces 1 alert when merging 857c29f into 1ffad2b - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 36d6e7b into 1ffad2b - view on LGTM.com new alerts:
|
|
@kryalama can you fix the lgtm bot alert above? |
Yes will do. Also looking at the failing smoke test. |
…t/applicationinsights/smoketest/SpringBootAutoTest.java
| try { | ||
| delegate = init(); | ||
| } catch (RuntimeException e) { | ||
| initException = e; |
There was a problem hiding this comment.
i am curious to know why not throw here directly and wait for second called of this method then throw at line 42?
Modify quick pulse code to use azure core httppipeline and have aad authentication enabled