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

Allow impersonation with spring #309

Merged
merged 1 commit into from Jan 17, 2016
Merged

Conversation

sergey-podolsky
Copy link
Contributor

No description provided.

@hazendaz
Copy link
Member

Can you squash the formatting change, add some unit tests, and configuration to use this in one of our samples?

Sent from Outlook Mobile

On Sun, Dec 27, 2015 at 6:12 AM -0800, "Sergey Podolsky" notifications@github.com wrote:

You can view, comment on, or merge this pull request online at:

#309

-- Commit Summary --

  • Allow impersonation in spring4
  • Don not dispose windowsIdentity if impersonated
  • Reformat code
  • Allow impersonation in spring3

-- File Changes --

M Source/JNA/waffle-spring-security3/src/main/java/waffle/spring/NegotiateSecurityFilter.java (47)
M Source/JNA/waffle-spring-security4/src/main/java/waffle/spring/NegotiateSecurityFilter.java (47)

-- Patch Links --

https://github.com/dblock/waffle/pull/309.patch
https://github.com/dblock/waffle/pull/309.diff


Reply to this email directly or view it on GitHub:
#309

@@ -59,6 +61,9 @@
/** The allow guest login. */
private boolean allowGuestLogin = true;

/** The impersonate. */
private boolean impersonate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default this to false?

@dblock
Copy link
Collaborator

dblock commented Dec 28, 2015

👍 Also update CHANGELOG.

@sergey-podolsky
Copy link
Contributor Author

Done.

@@ -18,3 +18,4 @@ Waffle.sln.cache
Waffle.suo
*.csproj.user
.tern-project
*.iml
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a module file created by IntelliJ IDEA

Copy link
Member

Choose a reason for hiding this comment

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

I realize a comment was made to explicitly set impersonate to false. Jvm does that by default so I'd remove that. I'll get a better overall look tonight.

Sent from Outlook Mobile

On Wed, Dec 30, 2015 at 4:23 AM -0800, "Sergey Podolsky" notifications@github.com wrote:

@@ -18,3 +18,4 @@ Waffle.sln.cache
Waffle.suo
.csproj.user
.tern-project
+
.iml

a module file created by IntelliJ IDEA


Reply to this email directly or view it on GitHub:
https://github.com/dblock/waffle/pull/309/files#r48601291

@dblock
Copy link
Collaborator

dblock commented Dec 30, 2015

Squash your commits please.

@@ -7,6 +7,7 @@

* Rework .net build to be mostly automatic using nuget
* Change .net target to more modern .net 4.0 framework
* [#309](https://github.com/dblock/waffle/pull/309): Added impersonation support on spring-security filters.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add your Github username. Thanks for contributing!

@hazendaz
Copy link
Member

ok - here is what you need to do. In order to properly squash things, you will need to perform interactive rebase (git rebase -i HEADXX where XX is all commits you want to deal with). Then squash where it makes sense. Another option is to reset softly all the comments then apply them again as they make sense (git reset --soft HEADXX where XX is all commits you want to deal with).

When you get that completed, perform a rewrite using plus operator -> git push origin +master

That will have this come in much cleaner to the master.

Only other note is to use assertj instead of hamcrest matchers. I'm not sure if we have assertj in waffle at the moment but if we don't, just add it in the parent near where junit is pulled in.

As soon as that is completed, I'll pull this in.

Question for you...when this comes in where you looking for a time table for a release? I really don't have much more to add at this time. We have one other pull request outstanding but waiting on user to get back with us in performing some of the same things you just did. I'd like to get his changes as well. However, if you are wanting fairly fast turn around and he isn't responding, I'll move forwards. If you are willing to wait, I'll try to get his response or ultimately I'll address his changes directly so we can close that out.

Anyway, thanks for the changes. I can see real value in this now with setup I use at my day job.

@sergey-podolsky
Copy link
Contributor Author

This is not really urgent for me, so I am happy to wait as much as needed. Anyway, I will be on vacation till 11 Jan, so will address all your suggestions as soon as I come back and when I have some spare time.

@hazendaz
Copy link
Member

hazendaz commented Jan 1, 2016

Sounds good. Enjoy your vacation

Sent from Outlook Mobile

On Fri, Jan 1, 2016 at 1:27 PM -0800, "Sergey Podolsky" notifications@github.com wrote:

This is not really urgent for me, so I am happy to wait as much as needed. Anyway, I will be on vacation till 11 Jan, so will address all your suggestions as soon as I come back and when I have some spare time.


Reply to this email directly or view it on GitHub:
#309 (comment)

@hazendaz
Copy link
Member

@sergey-podolsky When you get some time, if you want to button this guy up, I'll pull it in.

@sergey-podolsky
Copy link
Contributor Author

  • Replaced junit assertions with assertj
  • Removed wildcard (*) imports
  • Squashed all commits

@hazendaz
Copy link
Member

thanks @sergey-podolsky

hazendaz added a commit that referenced this pull request Jan 17, 2016
Allow impersonation with spring
@hazendaz hazendaz merged commit 66a4fe5 into Waffle:master Jan 17, 2016
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

3 participants