Custom instances of GenericPrincipal in WaffleAuthenticatorBase #571
Conversation
|
|
||
| this.log.debug("roles: {}", windowsPrincipal.getRolesString()); | ||
| this.log.debug("roles: {}", String.join(", ", genericPrincipal.getRoles())); |
There was a problem hiding this comment.
Please revert the change above. getRolesString() already does the string joiner.
There was a problem hiding this comment.
Using the toString-Method in log output would give the possibility to use more generic interfaces
this.log.debug("roles: {}", String.join(", ", genericPrincipal.getRoles()));
->
this.log.debug("principal: {}", principal.toString());
|
@Snap252 Can you also make the same change to all other negotiateAuthenticators? That way it is supported throughout. Thanks in advance. |
|
This method should go into the parent class I believe and be invoked from https://github.com/Waffle/waffle/blob/master/Source/JNA/waffle-tomcat7/src/main/java/waffle/apache/WaffleAuthenticatorBase.java#L246 as well. It also needs tests. |
|
Thx for the fast feedback.
If you tell me that this is the right way I will write some testing code and enhance code coverage. |
|
|
||
| this.log.debug("roles: {}", windowsPrincipal.getRolesString()); | ||
| this.log.debug("roles: {}", String.join(", ", genericPrincipal.getRoles())); |
There was a problem hiding this comment.
Minor: one space too many.
There was a problem hiding this comment.
Run a quick build, that will flush out that space or any others.
There was a problem hiding this comment.
@Snap252 There isn't really much difference between the various integrations. We probably could use an overall refactoring to make things better. For now though, as new builds become available, because we brand the version, we supply a new copy. So tomcat 9 is really just tomcat 8.5 with the minor variance you saw. Spring 3, 4, and 5 copies I believe are completely identical. Just haven't had time to really go in and look how to make it more streamlined.
There was a problem hiding this comment.
Run a quick build, that will flush out that space or any others.
- hazendaz
I already tried a maven test / maven install - but this kills me all the formatting (in existing and untouched files) - Am I doing something wrong, or is there anything I needed to know to keep formatting?
|
@Snap252 Just for reference of when this might get released. I'm waiting on spring security 5 to be officially released. Once that is done, I plan to push out 1.9.0 version of waffle. I'll include this work with it. Thanks. |
Waffle#571 +- replaced double space by single space
+ test case for custom principal
|
@Snap252 Can you add the same test case to tomcat7,8,9? Spring has now released spring security 5 so now just wating on that piece from you to merge this then a few rounds of testing and this will be released. Thanks. |
GenericPrincipal in WaffleAuthenticatorBase Waffle#571 @Snap252 Can you add the same test case to tomcat7,8,9? Spring has now released spring security 5 so now just wating on that piece from you to merge this then a few rounds of testing and this will be released. Thanks.
|
I was not sure if these tests are enough - and before copying it to the other subprojects I was waiting for your feedback 👍 |
|
I think it's good enough for what I can see here. I'll run some coverage after merging. I'm merging now. Thanks for getting this squared away. |
Custom instances of GenericPrincipal in WaffleAuthenticatorBase (#571)
|
I feel a pretty guilty here,
I thought it is done. Can you tell me, what is still to do here? Kind regards. |
|
Just need to go through each tomcat and compare them to make sure all are in sync. I've been short on time so haven't gotten back to this. If you are able, great. If not that is ok too but may be a few weeks before I can get back on this.
Get Outlook for Android<https://aka.ms/ghei36>
…________________________________
From: Snap252 <notifications@github.com>
Sent: Monday, February 5, 2018 2:04:01 AM
To: Waffle/waffle
Cc: Jeremy Landis; State change
Subject: Re: [Waffle/waffle] Custom instances of GenericPrincipal in WaffleAuthenticatorBase (#571)
I feel a pretty guilty here,
I think it's good enough for what I can see here.
I thought it is done. Can you tell me, what is still to do here?
Kind regards.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub<#571 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AA7ho2w9VUQHmFBhx685I_-2ZnyZpjUeks5tRqfhgaJpZM4QekyN>.
|
++ synchronized tomcat8 + tomcat7
refering:
#561
my main target is it to get the roles not directly by the windows roles but by a further mechanism that gets some roles from a database query using the current windowsIdentity as a query parameter