chore: LinkedIn custom OAuth#40594
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds optional ChangesLinkedIn OAuth with Configurable PKCE
Sequence DiagramsequenceDiagram
participant Startup as Meteor Startup
participant ImportPackages as importPackages.ts
participant LinkedInIndex as app/linkedin/server/index.ts
participant LinkedInLib as app/linkedin/server/lib.ts
participant SettingsWatcher as settings.watchMultiple
participant OAuthRegistrar as addPassportCustomOAuth
Startup->>ImportPackages: side-effect import `../app/linkedin/server`
ImportPackages->>LinkedInIndex: load linkedin server index
LinkedInIndex->>LinkedInLib: execute configureLinkedInOAuth
LinkedInLib->>OAuthRegistrar: register LinkedIn provider (pkce: false)
LinkedInLib->>SettingsWatcher: register watchMultiple for enable/credentials
SettingsWatcher->>LinkedInLib: trigger on setting change
LinkedInLib->>OAuthRegistrar: re-register LinkedIn provider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
2 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/phishing-resistant-mfa #40594 +/- ##
===============================================================
+ Coverage 69.71% 69.76% +0.04%
===============================================================
Files 3304 3297 -7
Lines 121731 120967 -764
Branches 21561 21549 -12
===============================================================
- Hits 84866 84391 -475
+ Misses 33602 33310 -292
- Partials 3263 3266 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
111904b to
6f9b1b9
Compare
ricardogarim
left a comment
There was a problem hiding this comment.
note to update the Accounts_OAuth_Linkedin_callback_url default to the new Passport callback URL: oauth/linkedin/callback.
also, nit: the previous implementation used to fetch the user profile. do you think it’s worth keeping that in the new implementation too? it would be something like adding avatarField: 'picture' to the config.
6e42e87
into
feat/phishing-resistant-mfa
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
PRM-36
Summary by CodeRabbit
New Features
Bug Fixes