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.

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(authorize-request): idp scoping provider #428

Merged
merged 2 commits into from
Nov 2, 2020

Conversation

rob-gijsens
Copy link
Contributor

Hi everybody,

In my case, the configured IDP in passport-saml is a proxying IDP. This IDP manages the connection with several underlying IDP's.
In some usecases, you want to target a specific IDP from within the application logic. This can be accomplished by targeting a IDP by IDPScoping. I added the option 'idpScopingProviderId' to the additonalSamlBehavior options that adds a node to the SAML AuthnRequest.

It would be very helpfull if this (relative small) feature could be merged in!

Greetings,
Rob

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I've tested this behavior with my setup and this works as expected.

@rob-gijsens
Copy link
Contributor Author

Any update on this? Really needing this feature to make passport-saml a suitable package for my project.

@rob-gijsens
Copy link
Contributor Author

@bergie Any update on when pending PR's are getting processed?

@rob-gijsens
Copy link
Contributor Author

I still really needs this feature for my project. Would be nice if someone has some time to have a look at this PR.
Is this project still actively maintained, or should I should look for an alternative.

@cjbarth I noticed you are one of the collaborators which has been recently active on this project. Could you maybe explain to me what the status is of this project/my PR?

@cjbarth
Copy link
Collaborator

cjbarth commented Oct 28, 2020

@rob-gijsens This is still an active project. This looks like a good feature to add. We just ask two things for features being added:

  1. Please include a link to the relevant part of the SAML spec in this PR for reference.
  2. Please include a minimal test case to make sure that this newly added feature isn't accidentally reverted or corrupted in the future.

@markstos
Copy link
Contributor

@rob-gijsens The project has recently become more actively maintained, with multiple committers and people with publish rights added. It's actively being rewritten in TypeScript and other moderization efforts are underway. Some people who were working on their own forks are now collaborating on this instead.

@rob-gijsens
Copy link
Contributor Author

@cjbarth @markstos Thank you for your explanation, and good to hear the project is still actively maintained.
I updated my PR. My feature is now added in the (new) Typescript files and I added a testcase to the test file.

@cjbarth As an answer to your questions:

1: The SAML2.0 specification for the 'Scoping' can be found in the following document: Page 51, Paragraph 3.4.1.2 Element
2: See the comment above and the latest commit to my PR.

@markstos
Copy link
Contributor

@rob-gijsens Thanks for the work.

Github is reporting conflicts. Could you rebase and squash your work into a single commit and force push that to this branch?

@cjbarth
Copy link
Collaborator

cjbarth commented Oct 30, 2020

@rob-gijsens As I read over the spec, it seems that your change, while fitting the spec, is very limited. It doesn't even seem to leave much room to gracefully grow support to the rest of the <scoping> spec.

@markstos , what do you think? Would it make more sense to have an object be provided as a config that can more easily be expanded later to support the rest of the <scoping> spec later on without breaking changes?

@markstos
Copy link
Contributor

@cjbarth I agree. The current proposal will make it awkard to support of the rest of the <Scoping> spec later.

@rob-gijsens Are you willing to update the contribution to support the list of the Scoping spec, including ProxyCount, IDPList and RequestorId ?

@rob-gijsens
Copy link
Contributor Author

@markstos @cjbarth
Good suggestion,

I updated the funtionality to support the full Scope element from the SAML2.0 spec.
I also updated the documentation for this updated feature and added some extra tests.

@cjbarth cjbarth added the semver-minor This PR requires a semver-minor version bump label Nov 2, 2020
@markstos
Copy link
Contributor

markstos commented Nov 2, 2020

QA Log

  • Reviewed diff
  • Reviewed related spec
  • Reviewed test coverage

@markstos markstos merged commit a11ad61 into node-saml:master Nov 2, 2020
@cjbarth cjbarth mentioned this pull request May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature semver-minor This PR requires a semver-minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants