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

Jump to bottom

Bypass for InResponseTo #87

Merged
merged 39 commits into from
Jun 16, 2022
Merged

Bypass for InResponseTo #87

merged 39 commits into from
Jun 16, 2022

Conversation

afurlane
Copy link
Contributor

Description

This should adress correctly the case of zero or more subjectconfirmations. If any is valid should go trough check for in-responseto or a fail condition is if there are some and none is vaid for any reason.

Checklist:

Alessandro Furlanetto and others added 30 commits March 8, 2022 09:41
Path to fix issue node-saml#339 with multiple subject confirmations
71483ba
Missing test
232f1c9
Missing package lock.
1493f34
Audit fix & package binary
187ecc1
@cjbarth
Merge branch 'master' into pr/43
ce9734b 
# Conflicts:
#	package-lock.json
#	package.json
#	test/tests.spec.ts
@cjbarth
Lint
38428ac
@afurlane
Fix to type declaration in code
18df38d
@afurlane
Test support for multiple subjectconfirmations in one assertion
02e645f
@cjbarth
Merge branch 'master' into pr/43
1fbe535
@cjbarth
Lint
1ac4719
missing package lock.
0d2c959
Correct test for multiple subject confirmation
8acccd6
Reverted package json
19543f1
prettify and test run
2791b37
@afurlane
Removed wrong import from test
d8f5965
@afurlane
The correct workflow suggested *is*. When you have multiple SC do sea…
381515d 
…rch and take one that is valid (checkTimestampsValidityError). But what if we don't have any valid sc? Take the first one; it will fail later in the flow.
@cjbarth
Merge remote-tracking branch 'origin/master' into pr/43
cf286a9
@cjbarth
Fix some test issues
2c6071b
@afurlane
Moved test in correct section
0a688d3
@afurlane
Code optimization and removal of wrong condition. If any subjectconfi…
06a003b 
…rmation is found then the firs one in the resulting array of valid confirmations is to be used, if none is found undefined is returned.
@cjbarth
Handle arrays correctly
a6bf700
@afurlane
Removed useless check on subjectconfirmation extracting confirmdata. …
771ed84 
…Add one more test to check if second subjectconfirmation is valid since the first one is expired.
@cjbarth
Merge branch 'master' into pr/43
e033e87
@cjbarth
Fix test for second SubjectConfirmationData
97a519f
@cjbarth
Simplify tests data
c3d1c18
@cjbarth
make pretty
4c03f94
@cjbarth
Cleanup unneeded branches
b3c2d7c
@cjbarth
Add test for no SubjectConfirmation
fd48f81
@cjbarth
Attempt to cover empty array case
7ff315f
@cjbarth
Remove pointless branch
ba65a9c
@cjbarth cjbarth changed the title Bypass for inresponseto Bypass for InResponseTo May 18, 2022
@cjbarth
Copy link
Collaborator

cjbarth commented May 18, 2022

I was just about to review this code when I noted that there are merge conflicts. Can you please resolve these before I review the code?

@codecov
Copy link

codecov bot commented May 18, 2022 •
edited

Codecov Report

Merging #87 (b2c55ee) into master (5b9962f) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   79.04%   79.14%   +0.10%     
==========================================
  Files          12       12              
  Lines         792      796       +4     
  Branches      238      238              
==========================================
+ Hits          626      630       +4     
  Misses         73       73              
  Partials       93       93
Impacted Files Coverage Δ
src/saml.ts 75.97% <100.00%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b9962f...b2c55ee. Read the comment docs.

@afurlane
Copy link
Contributor Author

I was just about to review this code when I noted that there are merge conflicts. Can you please resolve these before I review the code?

Sorry for the delay; I miss my duties on doing this merge sorry Chris. If I could do something more let me know; Really, I'm sorry about my fault.

@afurlane
Copy link
Contributor Author

@cjbarth sorry, maybe I forgot to mention you. Could you please take a look on this?

@cjbarth cjbarth merged commit c58d868 into node-saml:master Jun 16, 2022
@cjbarth cjbarth mentioned this pull request Jun 16, 2022
Closed
@cjbarth cjbarth added the bug Something isn't working label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjbarth cjbarth cjbarth approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@afurlane @cjbarth