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

Add decrypt support. #241

Merged
merged 9 commits into from
Jun 25, 2015
Merged

Add decrypt support. #241

merged 9 commits into from
Jun 25, 2015

Conversation

pitbulk
Copy link
Collaborator

@pitbulk pitbulk commented Jun 11, 2015

  • Be able to decrypt EncryptedAssertions

This was referenced Jun 11, 2015
@pitbulk
Copy link
Collaborator Author

pitbulk commented Jun 12, 2015

To end the PR, I need to add support for EncryptedID (Encrypted NameID).

meanwhile, @luisvm and @Lordnibbler can you review those commits?

@@ -51,6 +53,21 @@ def initialize(response, options = {})

@response = decode_raw_saml(response)
@document = XMLSecurity::SignedDocument.new(@response, @errors)

if assertion_encrypted?
Copy link
Contributor

Choose a reason for hiding this comment

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

return unless assertion_encrypted?
...

that will save you from indenting the following lines

@luisvm
Copy link
Contributor

luisvm commented Jun 17, 2015

👍 looks good.
could you please get more reviewers before merging?

assert_equal "true", spsso_descriptor.attribute("AuthnRequestsSigned").value
assert_equal ruby_saml_cert.to_der, cert.to_der
end

it "generates Service Provider Metadata with X509Certificate for sign and encrypt" do
assert_equal 2, key_descriptors.length

Choose a reason for hiding this comment

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

if there were 3... would that change anything in this spec? otherwise, we don't need the line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We testing that there are 2 and only 2 key descriptors.

@daniel-g
Copy link

@pitbulk please look at the conversation, some of them doesn't make sense, so please tell me if I'm thinking it wrong :)

@daniel-g
Copy link

👍

pitbulk added a commit that referenced this pull request Jun 25, 2015
@pitbulk pitbulk merged commit 04659e5 into master Jun 25, 2015
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.

3 participants