fix: Allow user to continue as Guest if API key validation fails

[gaurav08suri]

Allow user to continue as Guest if API key validation fails

Background for change:

• When trying to integrate Frappe as OAuth Provider with Apache Superset, it makes a call to get token from Frappe. During this call, it passes an Authorization Header based on the OAuth ClientId and ClientSecret Generated by Frappe and configured in superset.
• During this call, when trying to validate user via api key, it fails, because, though the Authorization Header is present, it corresponds to OAuth client and not a frappe user, what the code is currently expects.
• So, adding this change, would ignore any exception generated and allow to continue as Guest.

Additional context - https://discuss.erpnext.com/t/oauth-integration-of-frappe-with-apache-superset/83085/15

[ankush]

Add use-case and bit more information for reviewers. https://github.com/frappe/erpnext/wiki/Contribution-Guidelines

Mark "ready for review" when done.

[revant]

LGTM.
waiting for tests.

[codecov]

Codecov Report

Merging #15161 (63094ad) into develop (ba094e7) will decrease coverage by 8.76%.
The diff coverage is 0.00%.

❗ Current head 63094ad differs from pull request most recent head 6ab648e. Consider uploading reports for the commit 6ab648e to get more accurate results

@@             Coverage Diff             @@
##           develop   #15161      +/-   ##
===========================================
- Coverage    53.73%   44.97%   -8.77%     
===========================================
  Files          746      746              
  Lines        65176    65176              
  Branches      5423     5423              
===========================================
- Hits         35025    29311    -5714     
- Misses       26835    32549    +5714     
  Partials      3316     3316              

Flag	Coverage Δ
server	44.09% <0.00%> (-13.47%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[sagarvora]

I get what you're saying but I think for a person who is genuinely trying to login using an API key and secret, raising frappe.AuthenticationError gives a better picture of why they're not able to preform the desired action. Perhaps we should fallback to API key-secret auth only if Oauth fails?

[gaurav08suri]

Is there any way, we can skip this for OAuth ? I am not quite sure, if we can skip based on api paths ?

I get what you're saying but I think for a person who is genuinely trying to login using an API key and secret, raising frappe.AuthenticationError gives a better picture of why they're not able to preform the desired action. Perhaps we should fallback to API key-secret auth only if Oauth fails?

Makes sense. Any ideas, how we could proceed on this ? Or do we need to wait for some consensus for the route we need to take ?

[sagarvora]

Makes sense. Any ideas, how we could proceed on this ? Or do we need to wait for some consensus for the route we need to take?

Would something like this work? #15166

[sagarvora]

More context from https://discuss.erpnext.com/t/oauth-integration-of-frappe-with-apache-superset/83085/13:

This is what happened,

• I added validate_oauth(), it validates an header and throws error if invalid
• Someone else added similar function to validate secret and key.
• I refactored my code to not throw any errors or raise exceptions, let the user be set or let it remain Guest. Framework takes care of permissions and errors as per user.
• Refactor to code referenced from old validate_oauth() also needs refactor.

[sagarvora]

@revant Is there a way to identify from the request that it's OAuth and not API authentication?

[gaurav08suri]

I think, when the get_token api is called, even the OAuth Bearer token does not exist, so oauth will also not work.
This method would return the access_token, which the client would include in all subsequent requests, during which the oauth will work.

@revant - Is this understanding correct ?

[revant]

@revant Is there a way to identify from the request that it's OAuth and not API authentication?

not really.

• We cannot pass Authorization: Basic user:pass type headers to frappe framework.
• We cannot write a whitelisted endpoint where headers can be checked for Authorization: Basic user:pass, because all such requests will fail in api.py before reaching any method.

Any way to allow Basic Auth header and not fail at api.py? if #15166 does that, its fine too.
If it fails at api.py framework can't handle basic auth further in code.

I think, when the get_token api is called, even the OAuth Bearer token does not exist, so oauth will also not work.
This method would return the access_token, which the client would include in all subsequent requests, during which the oauth will work.

There are 2 general ways to get token,

1. POST body param passes client_secret
2. Basic Auth Header with base64(client_id:client_secret)

Frappe can accept 1st method. If apache superset has way to authenticate token endpoint using body param instead of header.
Frappe can't accept 2nd method because as soon as it detects "Authorization: Basic" it assumes it's api_key:api_secret or username:pass

[sagarvora]

What if we just ignore API authentication of any kind for that specific endpoint (get_token)?

@revant

[gaurav08suri]

Thank you all for the help.
Finally got this working using https://discuss.erpnext.com/t/oauth-integration-of-frappe-with-apache-superset/83085/18?u=gaurav08suri
Maybe, if we could include this in the nginx that is autogenerated by frappe, it would be helpful to the any future users.
Closing the PR.