Support API key as password for excel feeds #35698
Conversation
The API key is being used in place of the password, not the other way around
dimagimon
added
the
Risk: High
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: corehq/apps/domain/decorators.py
Did you find this useful? React with a 👍 or 👎 |
This ensures the function signatures for all login methods used in the auth decorator map are consistent, but doesn't actually change any behavior since the only time require_domain is explicitly set to False is in our LoginAuthentication, which uses the regular auth anyway for basic auth type anyway.
| @@ -373,6 +375,7 @@ def get_multi_auth_decorator(default, allow_formplayer=False, oauth_scopes=None, | |||
| formplayer can not use the session cookie to auth. To allow formplayer access to the | |||
| endpoints we validate each formplayer request using a shared key. See the auth | |||
| function for more details. | |||
| :param allow_api_key_as_password: If True, allows API Key to be used in BASIC auth | |||
There was a problem hiding this comment.
Is this the thing that you were referring to with this comment?
my main concern is that we will start to use this on other endpoints, when in reality we should reserve this for exceptional use cases where it is our best, and possibly only option
If yes, maybe add that concern to this docstring to warn future devs not to use it in exceptional circumstances? Possibly even rename the parameter to something like allow_api_key_as_password_EXCEPTIONAL_USE_ONLY to force anyone who uses it to type that out and as a flag to reviewers?
gherceg
changed the title
| @@ -416,6 +425,20 @@ def api_auth(*, allow_creds_in_data=True, oauth_scopes=None): | |||
| default=DIGEST, | |||
| oauth_scopes=oauth_scopes, | |||
| allow_creds_in_data=allow_creds_in_data, | |||
| allow_api_key_as_password=False, # if supporting digest auth, you cannot use an api key as a password | |||
There was a problem hiding this comment.
What do you think of enforcing the condition in this comment with an assertion in get_multi_auth_decorator?
I've probably got my head too deep in the auth code and lost sight of it, but do we have an explanation of why this is the case? Might be nice to include that here or possibly in the docstring of this function.
| @@ -443,9 +467,16 @@ def get_auth_decorator_map( | |||
| 'require_domain': require_domain, | |||
| 'allow_sessions': allow_sessions, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
re:
the only place we set
require_domainto False is here, which we can safely say does not use thelogin_or_basic_or_api_key_exfunction sinceallow_api_key_as_passworddefaults to False.
Would it make sense to assert that here?
| assert not allow_api_key_as_password or require_domain, \ | |
| "It is unexpected to allow API key as password and not require domain" |
I assume it's not easy or very meaningful to add tests to verify that our code never does that, but if it's possible to verify with a test then that might make sense too.
| See determine_authtype_from_header for more details on how defaulting to BASIC drops DIGEST support | ||
| """ | ||
| return get_multi_auth_decorator( | ||
| default=BASIC, |
There was a problem hiding this comment.
I'm curious if defaulting to BASIC has any other side effects that may not be anticipated? I see we do this in other places such as mobile and odata auth decorators as well, so it seems safe.
| ) | ||
|
|
||
|
|
||
| def api_auth_no_digest(*, allow_creds_in_data=True, oauth_scopes=None, allow_api_key_as_password=False): |
There was a problem hiding this comment.
Is the allow_api_key_as_password=False parameter needed here? The only place it is ever False is in tests. What do you think of eliminating it and always passing allow_api_key_as_password=True to get_multi_auth_decorator below?
Since this function is about allowing API key as password, would it make sense to rename it to make that more clear and instead explain the part about "no digest" in the docstring? Maybe something like api_auth_allow_key_as_password?
Product Description
Adds support for 2FA/SSO users to configure their Excel dashboard feeds with basic auth, using the API key in place of the password.
We will need to update these docs to reflect this option for 2FA/SSO users.
Technical Summary
https://dimagi.atlassian.net/browse/SAAS-16493
We already have support for this in OData feeds (#24231), and this is attempting to achieve similar behavior for Excel dashboards. We've seen unreliable behavior with our Excel handles the
Authorizationheader that holds the username and API key to the point where Excel will remove this header from the request entirely before sending it. This obviously results in a 401, and our users are left frustrated because they followed our documentation for setting up a data feed in Excel.Honestly, my main concern is that we will start to use this on other endpoints, when in reality we should reserve this for exceptional use cases where it is our best, and possibly only option. The alternative in this case is to tell users to pass the credentials via URL parameters, but that seems even less ideal than what this PR does, and therefore I went with this approach.
Feature Flag
Safety Assurance
Safety story
Automated test coverage
Added test to ensure a 401 is still raised when the
allow_api_key_as_passwordis set to False, which is what it defaults to. It is only set to True on this one endpoint (at the moment).QA Plan
I think we ideally want to fast track this change, so I will test on staging with the help of those that have access to Excel, and see if QA is needed then.https://dimagi.atlassian.net/browse/QA-7440
Rollback instructions
Excel dashboard feeds are currently broken for 2FA/SSO users on newer versions of excel, and if we tell users to adopt a new format configuring feeds, and then revert it, that would put us in an even worse situation.
Labels & Review