In Account class instantiation code the credentials parameter is typed incorrectly and fails static typechecking

[profMelo]

Based on sanity checking in Connection instantiation code here (line 445: or len(credentials) != 1 )

python-o365/O365/connection.py

Lines 437 to 450 in 6716030

	if auth_flow_type in (
	"public",
	"password",
	): # allow client id only for public or password flow
	if isinstance(credentials, str):
	credentials = (credentials,)
	if (
	not isinstance(credentials, tuple)
	or len(credentials) != 1
	or (not credentials[0])
	):
	raise ValueError(
	"Provide client id only for public or password flow credentials"
	)

for "public" authorization work flow, account credentials are Required to be of type Tuple[str] which is in direct contradiction to what is expected by the Account instantiation code here where credentials are typed as Tuple[str,str]

python-o365/O365/account.py

Line 11 in 6716030

def __init__(self, credentials: Tuple[str, str], *,

So doing something like this

credentials = (CLIENT_ID)
scopes = ['Mail.Send']
tokenBackEnd = EnvTokenBackend()
account = Account(credentials, auth_flow_type='public', tenant_id=TENANT_ID, token_backend=tokenBackEnd)

is the correct way to create a connection for public auth flow, and works as intended but it fails static type checking with an error like "Argument of type "Literal['CLIENT_ID']" cannot be assigned to parameter "credentials" of type "Tuple[str, str]" in function "init"
  "Literal['CLIENT_ID']" is not assignable to "Tuple[str, str]""

Do we need the validation of credentials being a length of one for public auth flow? In the public auth flow code only the first string is used for client_id, if a second string for client_secret is present, it is simply ignored. See here:

python-o365/O365/connection.py

Lines 611 to 619 in 6716030

	if self.auth_flow_type in ("public", "password"):
	client = PublicClientApplication(
	client_id=self.auth[0],
	authority=self._msal_authority,
	token_cache=self.token_backend,
	proxies=self.proxy,
	verify=self.verify_ssl,
	timeout=self.timeout
	)

I don't see anywhere that accepting a Tuple[str,str] for public auth flow would break the code, so this validation check seems unnecessary to me.

I propose two potential solutions

1: Modify validation checking to allow client_secret to be an empty string for public auth flow
In this case to keep backwards compatibility for pre-existing code that uses a credential variable like Tuple[str], just change the validation code to check for a length of one or two but enforce length two requiring empty string for second variable.
This provides backwards compatibility of the code because it still allows length of one, but also introduces ability to supply a Tuple of size 2 so that moving forward someone who cares about static type checking can set the credentials client_secret string to an empty string like this

credentials = (CLIENT_ID, '')
scopes = ['Mail.Send']
tokenBackEnd = EnvTokenBackend()
account = Account(credentials, auth_flow_type='public', tenant_id=TENANT_ID, token_backend=tokenBackEnd)

This could be implemented something like this (untested... I think the logic makes sense, but would require testing)

if (
    not isinstance(credentials, tuple)
    or not (len(credentials) == 1 or (len(credentials) ==2 and credentials[1] =='')) 
    or (not credentials[0])
  ):
    raise ValueError("Provide client id only for public or password flow credentials")

2: Leave validation checking as is, but just change the credential type in Account class instantiation to allow for a credential tuple of size 1 by changing this:

python-o365/O365/account.py

Line 11 in 6716030

def __init__(self, credentials: Tuple[str, str], *,

to this:
credentials: Tuple[str, str] | Tuple[str]

[alejcas]

To me the validation is useful to let know the user that only one value is expected (the client id) as with authorisation where 2 values need to be present.

The problem comes with the credentials in init not being properly annotated: Union[str, tuple[str, str]].

Edit: or doing what you said: credentials: Tuple[str, str] | Tuple[str] but adding str as a possibility

Type annotation in the library is mostly not done.
So maybe just changing that type annotation such suffice.

Do you agree?

[profMelo]

Yes that would be fine, actually I was thinking of Tuple[str] since credentials is normally a tuple, but you are correct if it is just the client ID and no secret, maybe just provide a string, no use to provide a tuple with a single string. It makes more sense to use just a string instead of a Tuple unless you want to keep things standard and always have credentials represented by Tuple[str, str] but allow client secret to be empty string (which requires changing the validation code as I indicated in option 1).

If you prefer option 2, just leave the validation alone, and simply change the type annotation of Account instantiation, then you can either user Union or | depending on what version of python is supported. Version 3.10+ supports | which is the modern standard and looks cleaner, but if supporting previous versions of python then use Union.

def __init__(self, credentials: str | Tuple[str, str] , *,
Or
def __init__(self, credentials: Union[str, Tuple[str, str]], *,

[alejcas]

Done, thanks!