Skip to content
This repository was archived by the owner on Feb 26, 2026. It is now read-only.

PRO-8013: comprehensive solution to support atypical passport strategies#31

Merged
boutell merged 13 commits intomainfrom
pro-8013
Jul 7, 2025
Merged

PRO-8013: comprehensive solution to support atypical passport strategies#31
boutell merged 13 commits intomainfrom
pro-8013

Conversation

@boutell
Copy link
Copy Markdown
Member

@boutell boutell commented Jul 3, 2025

This has been tested:

  • For the new use case. See:

apostrophecms/starter-kit-assembly-essentials#130

  • For existing, more typical use cases, to ensure no regression. See:

apostrophecms/starter-kit-essentials#75

@boutell boutell requested a review from haroun July 3, 2025 19:23
@linear
Copy link
Copy Markdown

linear Bot commented Jul 3, 2025

Comment thread README.md
### Working with passport strategies that expect different arguments to the `verify` callback

All passport strategies expect us to provide a `verify` callback. By default, `@apostrophecms/passport-bridge` passes its `findOrCreateUser` function as the `verify` callback, which works for many strategies including `passport-oauth2`, `passport-github2` and `passport-gitlab2`. For other strategies, you can pass an explicit `verify` option which will remap the strategy `verify` method with `@apostrophecms/passport-bridge` `findOrCreateUser` method.
All passport strategies expect us to provide a `verify` callback. By default, `@apostrophecms/passport-bridge` passes its `findOrCreateUser` function as the `verify` callback, which works for many strategies including `passport-oauth2`, `passport-github2` and `passport-gitlab2`. For other strategies, you can pass an explicit `verify` option which will remap the strategy `verify` method to our `@apostrophecms/passport-bridge` `findOrCreateUser` method.
Copy link
Copy Markdown
Member Author

@boutell boutell Jul 3, 2025

Choose a reason for hiding this comment

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

I made some wording improvements regarding verify, I didn't change it.

Comment thread README.md

That function accepts the normal verify callback from the passport bridge module, and returns an alternative function which accepts `req`, plus the arguments that are typical for `passport-auth0` or the strategy of your choice, and then invokes the normal verify callback with the arguments it expects, returning the result.

### Working with `oidc-client` and other passport strategies that don't follow typical patterns
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here's the juicy new feature.

Comment thread index.js

for (const strategy of self.options.strategies) {
const spec = klona(strategy);
for (let spec of self.options.strategies) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

freeing up the variable name strategy for the actual strategy object.

Comment thread index.js
// sensibly. But we can do it by making a dummy strategy object now
const dummy = new Strategy({
callbackURL: 'https://dummy/test',
const dummy = await factory({
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this edge case will rarely be used with a factory function, but must be thorough

Comment thread index.js
self.strategies[spec.name] = strategy;
self.apos.login.passport.use(strategy);
if (strategy._oauth2) {
// This will only work with strategies that actually have an _oauth2 object
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not everything is oauth2 based. Oops. A bug fix.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Without this we get a crash with strategies that can't support automatic token refresh. Which isn't even important unless you're reusing the tokens for API calls.

Copy link
Copy Markdown
Contributor

@haroun haroun left a comment

Choose a reason for hiding this comment

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

There are tiny request changes in the doc, the rest is ok

Comment thread README.md Outdated
Comment thread README.md Outdated
boutell and others added 2 commits July 7, 2025 08:32
Co-authored-by: haroun <1765606+haroun@users.noreply.github.com>
Co-authored-by: haroun <1765606+haroun@users.noreply.github.com>
@boutell boutell merged commit 12123e1 into main Jul 7, 2025
9 checks passed
@boutell boutell deleted the pro-8013 branch July 7, 2025 12:33
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.

2 participants