identity_vscode_credential

[xiangyan99]

No description provided.

[chlowell]

Please add this to DefaultAzureCredential, before AzureCliCredential and optionally disabled with keyword argument "exclude_visual_studio_code_credential".

[chlowell]

Shouldn't this go before trying to get a refresh token?

[xiangyan99]

If we fail to get a refresh token (which means the user may have logged out), why we should still return a valid token if there is one stored?

[chlowell]

The point is perhaps moot because get_token is usually called by an auth policy which caches access tokens itself. There will typically be a period of minutes--during which authorization will succeed--between a user signing out and the next get_token call.

Still, you ask a good question. Here's a couple things to think about in answering it:

1. Access tokens aren't connected to refresh tokens. They're valid for their (short) lifetimes even after the refresh token used to acquire them expires or is revoked.
2. In this scenario the user has signed out but left a program running. The program has tried to send a service request. Which is more surprising to the user: the request succeeding, or failing?

There's another interesting case here. The user can change the signed in identity between two calls to get_token. That suggests an argument for having get_token always redeem the refresh token, if any, for a new access token, although I don't think users should expect changing identities midstream to pass without error. Redeeming the refresh token every time also raises another issue: when we redeem a refresh token, AAD may return us a new one and invalidate the one we sent. We don't contribute this new refresh token back to VS Code. That means our credential can invalidate the Azure Account extension's refresh token but itself continue to succeed and I guess we then recurse on "what happens if the user signs out of the extension?" 😸

I'm trying to get all the food for thought here down on this page, sorry for all the words. Personally I now lean toward:

1. forget caching in this credential; leave it up to the auth policy
2. if the user signs out or changes the signed in identity, we make no promises: undefined behavior ahead
3. iterate following feedback

Anyone else have thoughts?

[xiangyan99]

In this scenario the user has signed out but left a program running. The program has tried to send a service request. Which is more surprising to the user: the request succeeding, or failing?

I think the choices are failing immediately or failing in a few minutes. I lean toward failing immediately because

1. It is deterministic
2. It is easier to diagnose.

[annatisch]

It seems this is used for service name and account name - service name is probably okay, but is there any change that an account name might not be utf8?

[xiangyan99]

This is a good question and I don't have an answer. I opened #11135 to track it.

[lmazuel]

I assume that would be utf8

[xiangyan99]

Yes. I discussed with Anna offline. We assume this is utf8. We will try if we can find a way to test it.

[lmazuel]

Do we want a message if the lib is not available, to do something like sudo apt-get install libsecret-1-0 or something?

[xiangyan99]

In fact, this credential is usually used in DefaultCredential which is a chain of credentials. If one does not work, it will try next one. So if libsecret-1-0 is not installed, we prefer just skip this one and go to next credential rather than failing.

[xiangyan99]

In other words, if libsecret-1-0 is not installed, the feature is disabled silently.

[lmazuel]

Re-thinking this, I guess if they have vscode the lib is available, or maybe vscode embed it to save their credentials?

[xiangyan99]

Azure connectors (e.g. azure-account, azure-storage, etc.) are vs code extensions. User can use VS code w/o them. Given this case, user uses vs code w/o Azure extensions. (and libsecret-1-0 is not installed). We should just not do anything, right?

[lmazuel]

I got my answer:

$ dpkg -I /mnt/c/Users/lmazuel/Downloads/code_1.44.2-1587059832_amd64.deb
 new Debian package, version 2.0.
 size 62335842 bytes: control archive=2325 bytes.
     705 bytes,    14 lines      control
    2909 bytes,    73 lines   *  postinst             #!/usr/bin/env
     189 bytes,     5 lines   *  postrm               #!/bin/bash
     274 bytes,     6 lines   *  prerm                #!/usr/bin/env
 Package: code
 Version: 1.44.2-1587059832
 Section: devel
 Depends: libnotify4, libnss3 (>= 2:3.26), gnupg, apt, libxkbfile1, libsecret-1-0, libgtk-3-0 (>= 3.10.0), libxss1
 Priority: optional
 Architecture: amd64
 Maintainer: Microsoft Corporation <[email protected]>
 Homepage: https://code.visualstudio.com/
 Installed-Size: 259118
 Provides: visual-studio-code
 Conflicts: visual-studio-code
 Replaces: visual-studio-code
 Description: Code editing. Redefined.
  Visual Studio Code is a new choice of tool that combines the simplicity of a code editor with what developers need for the core edit-build-debug cycle. See https://code.visualstudio.com/docs/setup/linux for installation instructions and FAQ.

This means libsecret will be installed if you have vscode

[lmazuel]

You're passing None for the third parameter, you're not checking the error. From the testbase of libsecret

static void
test_lookup_no_name (Test *test,
                     gconstpointer used)
{
	GError *error = NULL;
	gchar *password;

	/* should return null, because nothing with mock schema and 5 */
	password = secret_password_lookup_sync (&MOCK_SCHEMA, NULL, &error,
	                                        "number", 5,
	                                        NULL);
	g_assert_no_error (error);
	g_assert (password == NULL);

	/* should return an item, because we have a prime schema with 5, and flags not to match name */
	password = secret_password_lookup_sync (&NO_NAME_SCHEMA, NULL, &error,
	                                        "number", 5,
	                                        NULL);

	g_assert_no_error (error);
	g_assert_cmpstr (password, ==, "555");

	secret_password_free (password);
}

[lmazuel]

I'm keen to see the coredump on 2.7 ;)

[xiangyan99]

Added.

[lmazuel]

Is this an actual password that could be utf8, or an ascii token?

[xiangyan99]

We tried some real ones and they were utf8.

[chlowell]

Could you please format this with black -l 120?