Skip to content

Fix: (Azure AD) Check for valid profile picture response before converting to base64#3656

Merged
balazsorban44 merged 6 commits into
nextauthjs:mainfrom
davidchalifoux:main
Jan 20, 2022
Merged

Fix: (Azure AD) Check for valid profile picture response before converting to base64#3656
balazsorban44 merged 6 commits into
nextauthjs:mainfrom
davidchalifoux:main

Conversation

@davidchalifoux
Copy link
Copy Markdown
Contributor

@davidchalifoux davidchalifoux commented Jan 17, 2022

Reasoning 💡

Currently, the profile() function for Azure AD takes a response from the Microsoft Graph API and converts it to base64 for profile pictures. Unfortunately, this has caused bugs for my project because it doesn't make sure that the response from Microsoft is valid before converting. Users without a photo set on their account will not recieve a valid response, but an invalid "image" will still be returned.

This PR adds a small check to the profile photo response to make sure the HTTP-status is 200-299. If the status is outside of that range, the profile image is left as an empty string.

Checklist 🧢

  • Documentation
  • Tests
  • Ready to be merged

Affected issues 🎟

None

@github-actions github-actions Bot added core Refers to `@auth/core` providers labels Jan 17, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 17, 2022

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 13.01%. Comparing base (a4d831d) to head (d16354a).
⚠️ Report is 2669 commits behind head on main.

Files with missing lines Patch % Lines
src/providers/azure-ad.ts 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3656      +/-   ##
==========================================
- Coverage   13.04%   13.01%   -0.03%     
==========================================
  Files          92       92              
  Lines        1449     1452       +3     
  Branches      385      387       +2     
==========================================
  Hits          189      189              
- Misses       1246     1249       +3     
  Partials       14       14              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added a small suggestion.

Comment thread src/providers/azure-ad.ts Outdated
@balazsorban44 balazsorban44 merged commit 4824f8c into nextauthjs:main Jan 20, 2022
mnphpexpert added a commit to mnphpexpert/next-auth that referenced this pull request Sep 2, 2024
…rting to base64 (nextauthjs#3656)

* Fix: Add OpenID to authorization scope

* Fix: Check for valid profile picture response before converting to base64

* Update src/providers/azure-ad.ts

Co-authored-by: Balázs Orbán <info@balazsorban.com>

* Confirm that profile photo was returned

Co-authored-by: Balázs Orbán <info@balazsorban.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Refers to `@auth/core` providers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants