Skip to content

Conversation

@lekkimworld
Copy link

I used this repo when learning about SAML in general and configuring it with Salesforce Communities for single sign on. In that I updated SAMLValidator.java to allow it to read the DEBUG setting from an environment variable and show a bit or more debug debug on validation for learning.

Thank you for sharing the repo. Merge if you like or discard :)

Copy link
Contributor

@davidjbrossard davidjbrossard left a comment

Choose a reason for hiding this comment

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

Thanks for the neat suggestions. Here are some comments

  • What's the point of the CSPFilter?
  • In SAMLValidator.java, you use System.err.println()... Perhaps you should use either of System.out.println() or a logging framework e.g. log4j.

Copy link
Contributor

@davidjbrossard davidjbrossard left a comment

Choose a reason for hiding this comment

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

Are these Eclipse files instead of VSCode? Generally, we try to avoid any IDE-specific files and stick to industry-adopted techniques (e.g. Maven)

@lekkimworld
Copy link
Author

Hey - the reason why I used System.err instead of System.out is that the project used that stream already - also for regular logging. I agree that System.out is better for this kind of thing and I'll happily change it.

As to the CSPFilter and the IDE specific files then it's probably because I thought pull requests were against a specific commit and not a branch. The CSPFilter was one I added yesterday when playing around with Canvas API apps in Salesforce as they need a CSP header to show the app in an iframe.

Maybe it makes the most sense if you reject this pull request, I isolate the proposed changes in a new branch and resubmit the pull request?

Thanks

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.

2 participants