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

Modify invalid subject tests to not expect an error immediately #200

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

bakejeyner
Copy link
Collaborator

@bakejeyner bakejeyner commented Sep 13, 2018

###Issue
#201

Description of the Change

Before, the invalid subject error tests use to expect the IdP to error out directly after sending the AuthnRequest. Now the tests will send the AuthnRequest, attempt the user login, then expect the error.

Verification Process

The invalid subject error tests should pass when running the CTK against this branch of DDF.

Checklist:

  • Unit Tests Added/Modified

@coyotesqrl
Copy link
Member

Is there an issue for this PR?

val samlResponseDom = response.getBindingVerifier().decodeAndVerifyError()
val finalHttpResponse =
TestCommon.getImplementation(IdpSSOResponder::class)
.getResponseForPostRequest(response)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'm okay with this change until we run into an IdP that acts differently, but I think our IdP needing more interaction before validating the Subject is implementation-dependent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that this is implementation dependent. I also agree that in order to fix this issue, a separate effort should be made to look into:

  1. How many of our tests are actually implementation dependent
  2. The best solution to solve this implementation dependence.

Implementation dependent example: IdP1 could take the AuthnRequest Subject and check it against its user database, sending an error response without having to log any user in. Whereas IdP2 (lookin' at you DDF) may log the user in, then compare that user to the Subject sent in the AuthnRequest.

@bakejeyner
Copy link
Collaborator Author

@coyotesqrl There was no issue at the time of your comment, however I have created one and made links from the PR to the issue and vice versa.

@bakejeyner
Copy link
Collaborator Author

build now

@cxbot
Copy link
Collaborator

cxbot commented Sep 13, 2018

Build success! See the job results in Jenkins UI or in Blue Ocean UI.

@blen-desta
Copy link
Collaborator

test ddf

@cxbot
Copy link
Collaborator

cxbot commented Sep 13, 2018

Internal DDF test has been started. Your results will be available at completion. See build progress in Jenkins UI or in Blue Ocean UI.

@cxbot
Copy link
Collaborator

cxbot commented Sep 13, 2018

Build failure. See the job results in Jenkins UI or in Blue Ocean UI.

@blen-desta
Copy link
Collaborator

Oh forgot that codice/ddf#3733 needs to be merged for that test to pass.
build now

@cxbot
Copy link
Collaborator

cxbot commented Sep 13, 2018

Build success! See the job results in Jenkins UI or in Blue Ocean UI.

@brjeter
Copy link
Collaborator

brjeter commented Sep 18, 2018

Hero successful ✅

@blen-desta
Copy link
Collaborator

test ddf

@cxbot
Copy link
Collaborator

cxbot commented Sep 26, 2018

Internal DDF test has been started. Your results will be available at completion. See build progress in Jenkins UI or in Blue Ocean UI.

@cxbot
Copy link
Collaborator

cxbot commented Sep 26, 2018

Build failure. See the job results in Jenkins UI or in Blue Ocean UI.

@blen-desta
Copy link
Collaborator

build now

@cxbot
Copy link
Collaborator

cxbot commented Oct 1, 2018

Build success! See the job results in Jenkins UI or in Blue Ocean UI.

@blen-desta blen-desta merged commit 1f5feed into codice:master Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants