Skip to content

✨ Added user lookup dependency injection to sso base adapter#29365

Merged
acburdine merged 1 commit into
mainfrom
feat/sso-refactor
Jul 16, 2026
Merged

✨ Added user lookup dependency injection to sso base adapter#29365
acburdine merged 1 commit into
mainfrom
feat/sso-refactor

Conversation

@acburdine

Copy link
Copy Markdown
Member

ref https://linear.app/ghost/issue/PLA-233/create-public-npm-package-for-sso-adapter-base

  • adds new UserRepository type to SSO Base adapter, allowing calling code to inject model layer references into the base adapter without needing core requires
  • add @tryghost/errors dep to base adapter

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

SSOBase now supports an injected UserRepository for email and owner lookups, with errors when no repository is configured. Session creation supplies model-backed lookup functions to the SSO adapter. SSO tests now use an SSOBase subclass, and adapter tests cover configured and missing repository behavior. Workspace dependency catalogs now provide @tryghost/errors.

Possibly related PRs

  • TryGhost/Ghost#29285: Introduced the adapter-base-sso package and initial SSOBase wiring extended by this repository-injection change.

Suggested reviewers: evanhahn, allouis, 9larsons

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: injecting user lookup into the SSO base adapter.
Description check ✅ Passed The description matches the changeset by describing the new UserRepository injection and the added @tryghost/errors dependency.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/sso-refactor

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nx-cloud

nx-cloud Bot commented Jul 15, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit b963d17

Command Status Duration Result
nx run @tryghost/admin-x-settings:test:acceptance ✅ Succeeded 8m 58s View ↗
nx run ghost:test:ci:integration ✅ Succeeded 2m 49s View ↗
nx run-many -t test:unit -p ghost,@tryghost/ada... ✅ Succeeded 8m 34s View ↗
nx run ghost:test:integration ✅ Succeeded 3m View ↗
nx run ghost:test:legacy ✅ Succeeded 3m 18s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded 19s View ↗
nx run-many -t lint -p ghost,@tryghost/adapter-... ✅ Succeeded 3m 52s View ↗
nx run ghost:test:e2e ✅ Succeeded 3m 8s View ↗
Additional runs (9) ✅ Succeeded ... View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-07-15 18:57:45 UTC

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.00%. Comparing base (a7c4b00) to head (84245f9).

Files with missing lines Patch % Lines
...st/core/core/server/services/auth/session/index.js 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #29365      +/-   ##
==========================================
- Coverage   74.01%   74.00%   -0.01%     
==========================================
  Files        1578     1578              
  Lines      137508   137524      +16     
  Branches    16650    16652       +2     
==========================================
+ Hits       101777   101780       +3     
- Misses      34684    34697      +13     
  Partials     1047     1047              
Flag Coverage Δ
admin-tests 55.32% <ø> (-0.03%) ⬇️
e2e-tests 76.09% <81.25%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 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.

@acburdine
acburdine force-pushed the feat/sso-refactor branch from b963d17 to 5c4398a Compare July 15, 2026 18:45
ref https://linear.app/ghost/issue/PLA-233/create-public-npm-package-for-sso-adapter-base
- adds new UserRepository type to SSO Base adapter, allowing calling code to inject model layer references into the base adapter without needing core requires
- add @tryghost/errors dep to base adapter
@acburdine
acburdine force-pushed the feat/sso-refactor branch from 5c4398a to 84245f9 Compare July 15, 2026 18:46

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
ghost/core/core/server/services/auth/session/index.js (1)

87-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer models.User.getOwner() for owner lookups.

Ghost's User model provides a dedicated getOwner() method. Using it is preferred over duplicating the internal role and status query parameters with findOne.

♻️ Proposed refactor
-        async getOwner() {
-            const owner = await models.User.findOne({role: 'Owner', status: 'all'});
-            return owner ? {id: owner.id, email: owner.get('email')} : null;
-        }
+        async getOwner() {
+            const owner = await models.User.getOwner();
+            return owner ? {id: owner.id, email: owner.get('email')} : null;
+        }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ghost/core/core/server/services/auth/session/index.js` around lines 87 - 90,
Update the getOwner() method to use the dedicated models.User.getOwner() lookup
instead of duplicating role and status criteria through models.User.findOne.
Preserve the existing return shape, including the owner id and email mapping or
null when no owner exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@ghost/core/core/server/services/auth/session/index.js`:
- Around line 87-90: Update the getOwner() method to use the dedicated
models.User.getOwner() lookup instead of duplicating role and status criteria
through models.User.findOne. Preserve the existing return shape, including the
owner id and email mapping or null when no owner exists.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 08ff3baa-7793-4875-aba8-0787b655e127

📥 Commits

Reviewing files that changed from the base of the PR and between b963d17 and 84245f9.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • ghost/core/core/server/services/auth/session/index.js
  • ghost/core/package.json
  • ghost/core/test/e2e-api/admin/sso.test.js
  • packages/adapters/sso-base/package.json
  • packages/adapters/sso-base/src/base.ts
  • packages/adapters/sso-base/test/index.test.ts
  • pnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
  • ghost/core/test/e2e-api/admin/sso.test.js
  • packages/adapters/sso-base/src/base.ts
  • packages/adapters/sso-base/test/index.test.ts

@acburdine
acburdine merged commit 01d4f25 into main Jul 16, 2026
51 checks passed
@acburdine
acburdine deleted the feat/sso-refactor branch July 16, 2026 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants