Added notification when starting up to an invalid credential, or when selecting an invalid credential #635

awschristou · 2018-11-28T23:58:50Z

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Description

Added STS getCallerIdentity validation

If user tries to switch to a credential profile that is not valid:
- they get a notification indicating the credential appears invalid
- the credential profile is not changed
If toolkit starts up and the previously used profile is not valid:
- they get a notification indicating the credential appears invalid
- the toolkit's active credential profile is not set

Motivation and Context

reduce the amount of downstream errors due to invalid credentials
inform users that something does not seem right and requires action

Related Issue(s)

#578

Testing

Unit Tests
Loading toolkit and switching to an invalid credential
Exiting toolkit on a valid credential profile, then making that profile invalid locally, then restarting toolkit

Screenshots (if appropriate)

Checklist

I have read the README document
I have read the CONTRIBUTING document
Local run of gradlew check succeeds
My code follows the code style of this project
I have added tests to cover my changes
All new and existing tests passed
A short description of the change has been added to the CHANGELOG

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

… selecting an invalid credential

codecov-io · 2018-11-29T00:18:02Z

Codecov Report

Merging #635 into master will decrease coverage by 6.77%.
The diff coverage is 49.01%.

@@             Coverage Diff             @@
##             master    #635      +/-   ##
===========================================
- Coverage     52.07%   45.3%   -6.78%     
+ Complexity      486     446      -40     
===========================================
  Files           138     138              
  Lines          4280    4322      +42     
  Branches        623     629       +6     
===========================================
- Hits           2229    1958     -271     
- Misses         1800    2136     +336     
+ Partials        251     228      -23

Flag	Coverage Δ	Complexity Δ
#integtest	`?`	`?`
#unittest	`45.3% <49.01%> (-0.08%)`	`446 <0> (ø)`

Impacted Files	Coverage Δ	Complexity Δ
...ftware/aws/toolkits/core/credentials/AwsProfile.kt	`83.16% <ø> (ø)`	`0 <0> (ø)`	⬇️
...its/core/credentials/ToolkitCredentialsProvider.kt	`32% <0%> (-18%)`	`3 <0> (ø)`
...re/aws/toolkits/jetbrains/core/AwsSettingsPanel.kt	`42.85% <0%> (-3.03%)`	`3 <0> (ø)`
...its/jetbrains/core/credentials/CredentialWriter.kt	`77.61% <100%> (+0.33%)`	`0 <0> (ø)`	⬇️
.../aws/toolkits/jetbrains/utils/NotificationUtils.kt	`23.68% <56.25%> (-1.32%)`	`0 <0> (ø)`
.../core/credentials/ProjectAccountSettingsManager.kt	`94.44% <83.33%> (-3.77%)`	`0 <0> (ø)`
...e/src/software/aws/toolkits/core/s3/BucketUtils.kt	`0% <0%> (-100%)`	`0% <0%> (ø)`
...s/ui/wizard/java/SamInitRuntimeSelectionPanel.java	`0% <0%> (-94.24%)`	`0% <0%> (-12%)`
...ns/services/lambda/python/PythonSamDebugSupport.kt	`0% <0%> (-92.11%)`	`0% <0%> (-3%)`
...s/services/lambda/execution/sam/SamRunningState.kt	`5.66% <0%> (-88.68%)`	`2% <0%> (-10%)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13887cb...ac706de. Read the comment docs.

zhangzhx · 2018-11-30T19:04:24Z

core/src/software/aws/toolkits/core/credentials/ToolkitCredentialsProvider.kt

@@ -31,6 +33,20 @@ abstract class ToolkitCredentialsProvider : AwsCredentialsProvider {
    override fun hashCode(): Int = id.hashCode()

    override fun toString(): String = "${this::class.simpleName}(id='$id')"
+
+    open fun isValid(sdkHttpClient: SdkHttpClient): Boolean {


Can we remove the parameter and just use AwsSdkClient.getInstance().sdkHttpClient? I don't see the reason for using other SDK client.

This is in core, which doesn't have a reference to jetbrains-core, where AwsSdkClient resides

zhangzhx · 2018-11-30T19:07:17Z

jetbrains-core/src/software/aws/toolkits/jetbrains/core/AwsSettingsPanel.kt

+                    title = message("credentials.invalid.title"),
+                    content = message("credentials.invalid.notification", credentialsProvider.displayName),
+                    listener = NotificationListener { notification, _ ->
+                        ActionManager.getInstance().getAction("aws.settings.upsertCredentials").actionPerformed(e)


I think it would be better to just add this Action to the notification using Notification.addAction. That way, you don't need the listener.

We can use NotificationAction as a wrapper of the CreateOrUpdateCredentialProfilesAction

zhangzhx · 2018-11-30T19:08:54Z

jetbrains-core/src/software/aws/toolkits/jetbrains/core/credentials/CredentialWriter.kt

@@ -56,6 +57,9 @@ class CreateOrUpdateCredentialProfilesAction @TestOnly constructor(
        localFileSystem.refreshFiles(listOf(virtualFile), false, false) {
            fileEditorManager.openTextEditor(OpenFileDescriptor(project, virtualFile), true)
                ?: throw RuntimeException(message("credentials.could_not_open", file))
+
+            // TODO : remove message (and localized string) when credentials auto-refreshing is supported
+            notifyInfo("", message("credentials.notification.restart.ide"))


Do we need to provide a title for this info?

I don't feel that it is necessary

From this message, it's not clear to tell what the credentials are, although it could be inferred from the context. Maybe we should change the credentials to AWS credentials to be more clear?

AWS credential

How about After modifying AWS credentials, you will need to restart the IDE for the toolkit to use your changes. ?

abrooksv · 2018-12-03T20:21:34Z

...s-core/src/software/aws/toolkits/jetbrains/core/credentials/ProjectAccountSettingsManager.kt

@@ -107,7 +110,9 @@ class DefaultProjectAccountSettingsManager(private val project: Project) :
        @Throws(CredentialProviderNotFound::class)
        get() = activeProfileInternal
            ?: tryOrNull {
-                getCredentialProviderOrNull("${ProfileToolkitCredentialsProviderFactory.TYPE}:default")
+                getCredentialProviderOrNull(ProfileToolkitCredentialsProviderFactory.DEFAULT_PROFILE_DISPLAY_NAME)
+            }?.takeIf {


This will trigger MFA for loading of certain UIs that dont need it yet such as editing a run config

Does my change trigger MFA where the existing code does not?

abrooksv · 2018-12-03T20:23:39Z

...s-core/src/software/aws/toolkits/jetbrains/core/credentials/ProjectAccountSettingsManager.kt

@@ -130,7 +135,22 @@ class DefaultProjectAccountSettingsManager(private val project: Project) :

    override fun loadState(state: AccountState) {
        activeRegionInternal = regionProvider.lookupRegionById(state.activeRegion)
-        activeProfileInternal = state.activeProfile?.let { getCredentialProviderOrNull(it) }
+        state.activeProfile?.let { getCredentialProviderOrNull(it) }?.run {
+            if (this.isValid(AwsSdkClient.getInstance().sdkHttpClient)) {


This also triggers MFA on IDE start...maybe thats okay?

zhangzhx · 2018-12-03T22:22:01Z

jetbrains-core/src/software/aws/toolkits/jetbrains/utils/NotificationUtils.kt

+            this.message ?: "${this::class.java.name}${this.stackTrace?.joinToString("\n", prefix = "\n")}",
+            NotificationType.ERROR
+        ), project
+    )


Minor: can we move these to the util file as well, so that all the notify warn can share the same behavior.

Added notification when starting up to an invalid credential, or when…

9729d3f

… selecting an invalid credential

awschristou requested a review from abrooksv November 28, 2018 23:58

Fix build warning

9c91723

Merge branch 'master' into awschristou/invalid-credentials

925a9b1

zhangzhx reviewed Nov 30, 2018

View reviewed changes

abrooksv reviewed Dec 3, 2018

View reviewed changes

Feedback change

5f94181

zhangzhx reviewed Dec 3, 2018

View reviewed changes

awschristou and others added 4 commits December 4, 2018 14:32

Feedback change

2e12b09

Merge branch 'master' into awschristou/invalid-credentials

7e02bb2

Fix build failure

64bf2a4

Added changelog entry

ac706de

zhangzhx approved these changes Dec 5, 2018

View reviewed changes

awschristou merged commit a54d3aa into master Dec 5, 2018

awschristou deleted the awschristou/invalid-credentials branch December 5, 2018 21:32

rli mentioned this pull request Jan 17, 2019

UX: Credential errors should not cause an IDE error #528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added notification when starting up to an invalid credential, or when selecting an invalid credential #635

Added notification when starting up to an invalid credential, or when selecting an invalid credential #635

awschristou commented Nov 28, 2018

codecov-io commented Nov 29, 2018 •

edited

Loading

zhangzhx Nov 30, 2018

awschristou Dec 3, 2018

zhangzhx Dec 3, 2018

zhangzhx Nov 30, 2018 •

edited

Loading

zhangzhx Dec 3, 2018

zhangzhx Nov 30, 2018

awschristou Dec 3, 2018

zhangzhx Dec 3, 2018

abrooksv Dec 3, 2018

awschristou Dec 3, 2018

abrooksv Dec 3, 2018

awschristou Dec 3, 2018

abrooksv Dec 3, 2018

zhangzhx Dec 3, 2018

Added notification when starting up to an invalid credential, or when selecting an invalid credential #635

Added notification when starting up to an invalid credential, or when selecting an invalid credential #635

Conversation

awschristou commented Nov 28, 2018

Types of changes

Description

Motivation and Context

Related Issue(s)

Testing

Screenshots (if appropriate)

Checklist

License

codecov-io commented Nov 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangzhx Nov 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 29, 2018 •

edited

Loading

zhangzhx Nov 30, 2018 •

edited

Loading