Where is the issue?

• Other (please describe)

AadInstanceDiscovery uses a private global for it's cache but does not synchronize read and writes.

Expected behavior
Instance discovery cache is thread safe

Actual behavior
Instance discovery cache is not thread safe

**Other
sync.Once isn't really needed here either as the only thing it does is init a var, which can be done in it's declaration.

This is for for Desktop applications persisting to disk and doing so encrypted

• Extensions library
• shared token cache w/other dev tools

Per discussion in #68 (comment)

The Accounts() method returns a slice of accounts in a confidential client app however there will always only be one account.

There is no singular Account() method at present.

Establish a pattern for handling optional arguments. This should be established up front as the design might impact public surface area.

The idea is to provide a mechanism to avoid breaking changes in the event that we want to add a new, optional parameter to an API.

1. options struct to hold optional params
2. variadic configuration pattern
3. add new APIs with a suffix, e.g. WithFoo()

Option 1 and 2 are detailed here. Option 3 should be avoided.

There are types within internal/ that use pointers to basic types, such as:

type Account struct {
	HomeAccountID     *string `json:"home_account_id,omitempty"`
	Environment       *string `json:"environment,omitempty"`
	Realm             *string `json:"realm,omitempty"`
	LocalAccountID    *string `json:"local_account_id,omitempty"`
	AuthorityType     *string `json:"authority_type,omitempty"`
	PreferredUsername *string `json:"username,omitempty"`
	GivenName         *string `json:"given_name,omitempty"`
	FamilyName        *string `json:"family_name,omitempty"`
	MiddleName        *string `json:"middle_name,omitempty"`
	Name              *string `json:"name,omitempty"`
	AlternativeID     *string `json:"alternative_account_id,omitempty"`
	RawClientInfo     *string `json:"client_info,omitempty"`
	additionalFields  map[string]interface{}
}

This shouldn't be necessary for json encode/decode. This creates a lot of objects on the heap that cause GC slowdown.

Not that this is never done, but usually only if you need to tell the difference between a zero value and set value. Protocol Buffer version 2 does this. However, that method has been deprecated in version 3 because of these types of issues.

This will take a non-insignificant amount of work to do and if it is agreed this should be worked on, we should avoid colliding with another PR in internal/.

• Add CI build on either Azure Devops or Github actions that builds the code and runs unit tests for every pull request

Client credential grant will only look up tokens in the cache if used with acquire token silent. It should instead do a default cache lookup, with the option to skip.

Per discussion in #68 (comment)

Align with the other MSAL implementations regarding default claims.

E.g. ConfidentialClientApplication.AcquireTokenByClientCredential() should have a first param ctx context.Context.

This indicates to the caller that the API performs an I/O request and allows the caller to control the lifetime of the request.

https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-oauth2-on-behalf-of-flow

The JSON encoding inside is kinda funky. This looks to be the case in order to support a unique backend API. If the backend API sends an updated message with a field we do not have, we must store that field instead of dropping it. Because we will often need to alter that sent message and send back, replacing what's on the server, we cannot silently drop the field as Go's stdlib encoder/decoder does.

But we shouldn't have to give up easy to use struct decoding for map[string]interface{}, which is inefficient and hard to follow.

I propose either finding another third-party JSON library that has this function or vendoring our own version for use in the SDK with the additional functionality. The JSON library is feature frozen, so this isn't something we can push back upstream.

This would significantly reduce both the time it takes for encode/decode and reduce the code to be much easier to read and follow.

These are some of the pages that should be updated (there are more)

https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-web-app-sign-user-app-configuration?
https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-web-app-call-api-app-configuration?
https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-desktop-app-configuration?https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-daemon-app-configuration?

I'm not 100% sure this should exist, unless we are going to implement several of these. But if we are, then I am suggesting the following changes:

• StorageManager should become private or its args/return values should become public (it is internal, so it can only be implemented in this repo)
• Should be composed of multiple interfaces that allow use of slimmer interfaces and reduce naming size
• Use the namespace to make actions clear
• Use smaller interfaces when possible instead of StorageManager

Here is a proposal for new interfaces:

type StorageManager interface {
	Tokenser
	Accounter
	MetaDataer
	Serialize() ([]byte, error)
	Deserialize([]byte) error
}

type Tokenser interface {
	Tokens() Tokens
}

type Tokens interface {
	Access() AccessReadWrite
	Refresh() RefreshReadWrite
	ID() IDReadWrite
}

type AccessReadWrite interface{
	Read(homeAccountID string, envAliases []string, realm, clientID string, scopes []string) (accessTokenCacheItem, error)
	Write(accessToken accessTokenCacheItem) error
}

type RefreshReadWrite interface {
	Read(homeAccountID string, envAliases []string, familyID, clientID string) (refreshTokenCacheItem, error)
	Write(refreshToken refreshTokenCacheItem) error
}

type IDReadWrite interface{
	Read(homeAccountID string, envAliases []string, realm, clientID string) (idTokenCacheItem, error)
	Write(idToken *idTokenCacheItem) error
}

type Accounter interface{
	Accounts() Accounts
}

type Accounts interface{
	ReadAll() ([]msalbase.Account, error)
	Read(homeAccountID string, envAliases []string, realm string) (msalbase.Account, error)
	Write(account msalbase.Account) error
	Delete(homeAccountID string, envAliases []string) error
}

type MetaDataer interface{
	MetaData() MetaData
}

type MetaData interface{
	Read(envAliases []string, clientID string) (appMetadata, error)
	Write(appMetadata appMetadata) error
}

Now, this does look like more text, but in essence this becomes a really easy to access interface that is composed (so you can use a smaller surface area like Accounts when only those methods are required). This is the same model that base packages like io use.

Here's and example of use:

// Get an access token:
at, err := sm.Tokens().Access().Read(homeAccountID, envAliases, realm, clientID, scopes []string)
if err != nil {
  // Do something
}

// Read all accounts:
accounts, err := sm.Accounts().ReadAll()
if err != nil {
  // Do something
}

The library has support for pem encoded PKCS8 private key.

Investigate and add support for other types.

I tried using a PEM encoded RSA Private key in PKCS1 and it gives the following error:
x509: failed to parse private key (use ParsePKCS1PrivateKey instead for this key format)"

For a sample error response from the service like this:

"error":"invalid_client","error_description":"AADSTS7000218: The request body must contain the following parameter: 'client_assertion' or 'client_secret'.\r\nTrace ID: 86b167f1-2c86-470c-bdc7-04cddd81d401\r\nCorrelation ID: 1441968e-02a2-4280-adb9-8b8ded58a0a0\r\nTimestamp: 2020-11-09 09:13:06Z","error_codes":[7000218],"timestamp":"2020-11-09 09:13:06Z","trace_id":"86b167f1-2c86-470c-bdc7-04cddd81d401","correlation_id":"1441968e-02a2-4280-adb9-8b8ded58a0a0","error_uri":"https://login.microsoftonline.com/error?code=7000218"

the library just exposes 'invalid_client`
Change this to include the error object as this currently hides the error description and the rest of the fields.

We (the Go SDK team) have been burned in the past by poor-quality third-party dependencies. Review the current list to ensure they're required and provide value.

We should also avoid using any third-party type as an "exchange type" (a type exposed in public surface area).

A lot of operations take/return an interface with a "Provider" suffix. The naming is not idiomatic, and is a lot of cases the interface can likely be removed and a struct returned instead. A good example of the latter is DeviceCodeResultProvider.

Needs to follow Golang convention

Where is the issue?
Tests

What version of Go are you using (go version)?
Test default_storage_manager_test.TestReadRefreshToken failed on go 1.14, passed on build previously

See if test is flaky and needs refactor

Passing test: https://github.com/AzureAD/microsoft-authentication-library-for-go/pull/45/checks?check_run_id=1254682076
Failing test: https://github.com/AzureAD/microsoft-authentication-library-for-go/runs/1255127547?check_suite_focus=true

Where is the issue?

• Other (please describe)

github.com/google/uuid.New() returns a new uuid or panics, instead should use NewRandom() and handle the error appropriately

See: https://godoc.org/github.com/google/uuid#New

Internal packages are useful when you want to share, without exporting, code across several packages. With the current implementation, the code is needlessly complex and hard to follow.

The client polling stage of device code flow should tolerate network drift by some retry mechanism.
I find retryablehttp is extremely helpful and easy to adapt.

config.json doesn't need client secret & confidential.json doesn't need username/password for example.

Make sure to review what is being logged

Currently, the build function for authority asks user to not only pass in the authority endpoint, but also the token endpoint and so on.
While it seems once we get the authority endpoint, we can discover other endpoints via some endpoint, as shown here:

https://github.com/magodo/terraform-provider-outlook/blob/master/msauth/authority.go

More details to come.

Per rfc8628, client polling access token might get a expired_token response:

The "device_code" has expired, and the device authorization
session has concluded. The client MAY commence a new device
authorization request but SHOULD wait for user interaction before
restarting to avoid unnecessary polling.

So we might need to specifically handle this case, e.g. commence a new device authorization request and prompt a new message to user.

https://github.com/marketplace/actions/check-code-formatting-using-gofmt

Potentially things like MSALClientError or MSALServiceError.

See the conversation in #68 (comment) for more info.

https://github.com/Azure/azure-sdk-for-go/tree/master/sdk/azcore

Create a quick start, concept to be found here:

Javascript quickstart, notice how we are using tags for what goes to the portal and docs.
https://docs.microsoft.com/en-us/azure/active-directory/develop/quickstart-v2-javascript

The md file from docs and convert it into HTML based on the tags for it to surface in portal.
Detailed instructions shared internally.

ex. u/p w/common instead of organizations

Microsoft Docs ADFS documentation