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.

Sign up for GitHub

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

feat: allow passing of object to auth strategy service #371

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: allow passing of object to auth strategy service #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/ServerConfiguration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ Here is an example config with all the currently supported options. See descript

#### `auth.strategy.service`

- **Type:** `string`
- **Description:** A relative path to a authentication service module. It currently must export a strategy property which is set to a valid [passport strategy](http://www.passportjs.org/packages/).
- **Type:** `string|object`
- **Description:** A relative path to an authentication service module, or the actual module itself (e.g. `require('path/to/service')`). It currently must export a strategy property which is set to a valid [passport strategy](http://www.passportjs.org/packages/).
- **Required:** false
- **Default:** undefined

Expand Down
3 changes: 2 additions & 1 deletion packages/node-fhir-server-core/src/server/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ class Server {

configurePassport() {
if (this.config.auth && this.config.auth.strategy) {
let { strategy } = require(path.resolve(this.config.auth.strategy.service));
const service = this.config.auth.strategy.service;
Copy link
Contributor

@ekdeveloper ekdeveloper Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

makes more sense to make it into a separate function

const Strategy = require('passport-http-bearer').Strategy;

configurePassportWithStrategy(strategy) {
  if(strategy instanceof passport.Strategy){
    passport.use(strategy);
    return true;
  }
  return false;
}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for checking, I will modify it, but I am not sure I understand entirely. Do you mean something like this? What to do with the return value?

configurePassport() {
  if (this.config.auth && this.config.auth.strategy) {
    const service = this.config.auth.strategy.service;
    let { strategy } = typeof service === 'string' ? require(path.resolve(service)) : service;
    this.configurePassportWithStrategy(strategy);
  }

  // return self for chaining
  return this;
}

configurePassportWithStrategy(strategy) {
  if (strategy instanceof passport.Strategy) {
    passport.use(strategy);
    return true;
  }
  return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

don't update configurePassport(). Just add the new configurePassportWithStrategy() and use that instead of configurePassport().

Copy link
Author

Choose a reason for hiding this comment

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

ok, but I think that will not be very useful - those who may need it will not be able to use the default initialize, but instead have to do custom initialization. In such case the users can just skip configurePassport() and call passport.use() directly.

I copied the code from verifyAndLoadProfiles which already supports both strings and objects, so this change doesn't break existing behavior, just adds more possibility and unification.

let { strategy } = typeof service === 'string' ? require(path.resolve(service)) : service;
passport.use(strategy);
}

Expand Down