Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(oauth): Send id_token of the authorized user instead of Guest #17266

Merged
merged 2 commits into from Jun 22, 2022

Conversation

adityahase
Copy link
Member

@adityahase adityahase commented Jun 21, 2022

This only affects OAuth clients that use id_token obtained from frappe.integrations.oauth2.get_token.

Doesn't affect OAuth clients that ignore id_token and explicitly use frappe.integrations.oauth2.openid_profile endpoint for getting user details. e.g. Frappe OAuth client.

A simple way to replicate this is to setup Frappe-Frappe OAuth client-server pair and use login_via_oauth2_id_token instead of login_via_oauth2 in login_via_frappe.

FAIL  test_login_using_authorization_code (frappe.tests.test_oauth20.TestOAuth20)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/frappe-bench/apps/frappe/frappe/tests/test_oauth20.py", line 96, in test_login_using_authorization_code
    self.assertEqual(decoded_token["email"], "test@example.com")
    auth_code = '8xDxDU8wFZ6Yj8ZHApaiXzbJpvm8HF'
    bearer_token = {'access_token': 'BCkfz3SIxlxA1m17JwBYwzwBs1NUqT', 'expires_in': 3600, 'token_type': 'Bearer', 'scope': 'all openid', 'refresh_token': 'bzdPyQ1sCiitmOpOAfqPeuhCVoBlS3', 'id_token': '***'}
    decoded_token = {'aud': 'fc0171f657', 'iat': 1655827639, 'at_hash': 'rEU6GMvyNXUdRDahttMtDg', 'iss': 'http://test_site:8000', 'sub': None, 'name': 'Guest', 'given_name': 'Guest', 'family_name': None, 'email': 'guest@example.com', 'picture': None, 'roles': ['Guest']}
    query = {'code': ['8xDxDU8wFZ6Yj8ZHApaiXzbJpvm8HF']}
    redirect_destination = 'http://localhost?code=8xDxDU8wFZ6Yj8ZHApaiXzbJpvm8HF'
    self = <frappe.tests.test_oauth20.TestOAuth20 testMethod=test_login_using_authorization_code>
    session = <requests.sessions.Session object at 0x7fe7d90c6040>
    token_response = <Response [200]>
AssertionError: 'guest@example.com' != 'test@example.com'
- guest@example.com
? ^^       -
+ test@example.com
? ^

Reference: https://github.com/frappe/frappe/runs/6988384120?check_suite_focus=true#step:14:1019 In case the test results get lost.

This came up while setting up OAuth for Sentry.
Before.mp4
After.mp4
But mere mortals probably don't have Sentry instances lying around. Replicating with frappe works too.
Frappe.-.Before.mp4
Frappe.-.After.mp4

@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #17266 (93320ce) into develop (b04bffe) will decrease coverage by 1.39%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop   #17266      +/-   ##
===========================================
- Coverage    56.38%   54.98%   -1.40%     
===========================================
  Files          760      760              
  Lines        68398    68400       +2     
  Branches      5901     5901              
===========================================
- Hits         38564    37611     -953     
- Misses       26123    27078     +955     
  Partials      3711     3711              
Flag Coverage Δ
server 61.02% <0.00%> (-2.19%) ⬇️

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

This only affects OAuth clients that use `id_token` obtained from `frappe.integrations.oauth2.get_token`.

Doesn't affect OAuth clients that ignore id_token and explicitly use `frappe.integrations.oauth2.openid_profile` endpoint for getting user details. e.g. Frappe OAuth client.

A simple way to replicate this is to setup Frappe-Frappe OAuth client-server pair and use `login_via_oauth2_id_token` instead of `login_via_oauth2` in `login_via_frappe`.
This only affects OAuth clients that use `id_token` obtained from `frappe.integrations.oauth2.get_token`.

Doesn't affect OAuth clients that ignore id_token and explicitly use `frappe.integrations.oauth2.openid_profile` endpoint for getting user details. e.g. Frappe OAuth client.

A simple way to replicate this is to setup Frappe-Frappe OAuth client-server pair and use `login_via_oauth2_id_token` instead of `login_via_oauth2` in `login_via_frappe`.
@adityahase adityahase marked this pull request as ready for review June 21, 2022 17:01
@adityahase adityahase requested a review from a team as a code owner June 21, 2022 17:01
@adityahase adityahase requested review from phot0n and removed request for a team June 21, 2022 17:01
@mergify mergify bot merged commit c6ada3d into frappe:develop Jun 22, 2022
@ankush
Copy link
Member

ankush commented Sep 1, 2022

mergify ded? 😢

@ankush
Copy link
Member

ankush commented Sep 1, 2022

@Mergifyio backport version-13-hotfix

ankush added a commit that referenced this pull request Sep 1, 2022
…ckport #17266) (#18007)

* test(oauth): Send id_token of the authorized user instead of Guest

This only affects OAuth clients that use `id_token` obtained from `frappe.integrations.oauth2.get_token`.

Doesn't affect OAuth clients that ignore id_token and explicitly use `frappe.integrations.oauth2.openid_profile` endpoint for getting user details. e.g. Frappe OAuth client.

A simple way to replicate this is to setup Frappe-Frappe OAuth client-server pair and use `login_via_oauth2_id_token` instead of `login_via_oauth2` in `login_via_frappe`.

(cherry picked from commit 23cad54)

# Conflicts:
#	frappe/tests/test_oauth20.py

* fix(oauth): Send id_token of the authorized user instead of Guest

This only affects OAuth clients that use `id_token` obtained from `frappe.integrations.oauth2.get_token`.

Doesn't affect OAuth clients that ignore id_token and explicitly use `frappe.integrations.oauth2.openid_profile` endpoint for getting user details. e.g. Frappe OAuth client.

A simple way to replicate this is to setup Frappe-Frappe OAuth client-server pair and use `login_via_oauth2_id_token` instead of `login_via_oauth2` in `login_via_frappe`.

(cherry picked from commit 93320ce)

* chore: conflicts

Co-authored-by: Aditya Hase <aditya@adityahase.com>
Co-authored-by: Ankush Menat <ankush@frappe.io>
frappe-pr-bot pushed a commit that referenced this pull request Sep 6, 2022
# [13.41.0](v13.40.0...v13.41.0) (2022-09-06)

### Bug Fixes

* comma looks like a decimal in report summary widget ([#18014](#18014)) ([#18016](#18016)) ([af2598e](af2598e))
* **Data Import:** make warnings translatable (backport [#17924](#17924)) ([#17930](#17930)) ([1eb7248](1eb7248))
* **oauth:** Send id_token of the authorized user instead of Guest (backport [#17266](#17266)) ([#18007](#18007)) ([415f1f1](415f1f1))

### Features

* add lang to safe globals ([f51780a](f51780a))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants