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: add RequestedAuthnContext Comparison Type parameter #360

Merged
merged 1 commit into from Mar 26, 2019

Conversation

osan15
Copy link
Contributor

@osan15 osan15 commented Mar 5, 2019

No description provided.

@osan15 osan15 changed the title ⭐ feat: add RequestedAuthnContext Comparison Type parameter feat: add RequestedAuthnContext Comparison Type parameter Mar 5, 2019
Copy link
Contributor

@markstos markstos left a comment

Choose a reason for hiding this comment

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

I'm interested in merging this PR with refinements:

  • Document all the supported values
  • Validate that only "exact", "better", "minimum" and "maximum" are accepted
  • Include automate test coverage to illustrate it works as expected. You can refactor related code to make it easier to unit-test this if needed.
  • When done, rebase and squash the work into a single commit.

I reviewed the spec and found this feature and the supported value documented here confirming that this chang e would improve our spec compliance.

Copy link
Contributor

@markstos markstos left a comment

Choose a reason for hiding this comment

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

I'm interested in merging this PR with refinements:

  • Document all the supported values
  • Validate that only "exact", "better", "minimum" and "maximum" are accepted
  • Include automate test coverage to illustrate it works as expected. You can refactor related code to make it easier to unit-test this if needed.
  • When done, rebase and squash the work into a single commit.

I reviewed the spec and found this feature and the supported value documented here confirming that this chang e would improve our spec compliance.

Copy link
Contributor

@markstos markstos left a comment

Choose a reason for hiding this comment

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

I'm interested in merging this PR with refinements:

  • Document all the supported values
  • Validate that only "exact", "better", "minimum" and "maximum" are accepted
  • Include automate test coverage to illustrate it works as expected. You can refactor related code to make it easier to unit-test this if needed.
  • When done, rebase and squash the work into a single commit.

I reviewed the spec and found this feature and the supported value documented here confirming that this chang e would improve our spec compliance.

test: add test for check the option comparisonType
@osan15
Copy link
Contributor Author

osan15 commented Mar 19, 2019

The modifications have been made

@markstos markstos merged commit a466139 into node-saml:master Mar 26, 2019
@markstos
Copy link
Contributor

I've merged this, but renamed comparisonType to RACComparison, as comparisonType is too generic. I also documented in the main README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants