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

Error reporting for diagnostics #98

Merged
merged 5 commits into from
Oct 9, 2014
Merged

Error reporting for diagnostics #98

merged 5 commits into from
Oct 9, 2014

Conversation

christianbpedersen
Copy link

Status

READY

Description

This adds some error output.

Todos

  • Write tests

Deploy Notes

Steps to Test or Reproduce

bundle exec rake

Impacted Areas in Application

@Lordnibbler
Copy link
Contributor

@christianbpedersen can you please rebase this branch off master and add tests for new code?

@pwnetrationguru
Copy link
Contributor

I've rebased off current master and will look into tests, @Lordnibbler.

@pwnetrationguru
Copy link
Contributor

@pitbulk, @luisvm, @Lordnibbler, rebased and tests added.

@pwnetrationguru
Copy link
Contributor

Oops, look like I missed two cases. Adding now...

@Lordnibbler
Copy link
Contributor

@pwnetrationguru can you please remove the version bump from this branch?

@@ -1,5 +1,5 @@
module OneLogin
module RubySaml
VERSION = '0.8.1'
VERSION = '0.8.2'
Copy link
Contributor

Choose a reason for hiding this comment

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

@pwnetrationguru as per @Lordnibbler, could you please remove the version bump?

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 9, 2014

@pwnetrationguru , we need that you remove version bump, also we have the #147 that includes a new error, can you mix this PR (branch issuer) with your code and add its test?

@pwnetrationguru
Copy link
Contributor

hey all, yep, i'll plan on updating this PR tomorrow! 👍

@pwnetrationguru
Copy link
Contributor

@Lordnibbler, @luisvm and @pitbulk, version bump removed, test added, should be GTG 🐥

@pwnetrationguru
Copy link
Contributor

Oops, looking at #147 now and will add that error

@pwnetrationguru
Copy link
Contributor

@Lordnibbler, @luisvm and @pitbulk, re #147. I think the best approach is merge this PR into master and then comment on #147 to add the errors to @errors

Otherwise, I'm merging in changes from a PR that hasn't been merged into master yet,etc.

Thoughts?

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 9, 2014

@pwnetrationguru You can download the https://github.com/onelogin/ruby-saml/pull/147.patch do a:

git apply 147.patch

and handle with the error issue, then you can close the #147 referencing this PR, then merge this PR and at the end, delete the issuer branch

@pwnetrationguru
Copy link
Contributor

That doesn't seem to follow git best practices. What makes the most sense, to me, is get this merged into master. Then I can checkout the branch associated with #147 and add the @errors object. My suggestion would be:

  1. Merge this PR into master
  2. Rob opens PR against Fix LogoutResponse issuer validation and implement SAML Response issuer validation. Related to Pull Request 116 #147 to add @errors
  3. Merge Fix LogoutResponse issuer validation and implement SAML Response issuer validation. Related to Pull Request 116 #147 into master

@Lordnibbler
Copy link
Contributor

👍, id recommend @pwnetrationguru's steps for git but @pitbulk's will also work

pwnetrationguru pushed a commit that referenced this pull request Oct 9, 2014
Error reporting for diagnostics
@pwnetrationguru pwnetrationguru merged commit 7488f28 into master Oct 9, 2014
@pwnetrationguru pwnetrationguru deleted the error_messages branch October 9, 2014 21:02
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.

None yet

5 participants