[GitHub] gonzalad commented on issue #20: FEDIZ-212: fix logout when no httpSession present

[git]

gonzalad commented on issue #20: FEDIZ-212: fix logout when no httpSession 
present
URL: https://github.com/apache/cxf-fediz/pull/20#issuecomment-335427742
 
 
   Hi Sergey, 
   The important thing about this PR is more the fact that doing a logout with 
a null principal (ie oidc httpSession expired, user already clicked on logout 
button on another OIDC Client app, ...) behaves in the same way (from the end 
user perspective) as a logout request with a non-null principal :
   - it doesn't raise an exception 
- and redirects to idpLogoutUri 
- and ultimately redirects back to the post_logout_redirect_url configured 
for the configured OIDC Client.
   
   >  should we move the session invalidation inside the branch with the 
non-null principal ?
   
   The call mc.getHttpServletRequest().getSession().invalidate() can be moved 
to the 
   ```
   if (mc.getSecurityContext().getUserPrincipal() != null) {
   ```
   block or be kept outside.
   
   I've just tested both way, it works both way.
   
   Thanks !
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Swagger2Feature bug caused by removal of isScan() check

[Sergey Beryozkin]

I've introduced the property but it is set to false in 3.2.x

Sergey
On 07/10/17 17:17, Sergey Beryozkin wrote:

Hi Dennis
Thanks very much, my fault really as I did not see a point in keeping 
this property in 3.2.x and indeed, as you imply, all tests were/are 
still passing, but I guess more preparation should've gone into it.


Setting a scan to true did never work in OSGI as far as I know, may be 
in some basic cases only, so in OSGI case, one nearly always has to set 
it to false anyway. I recall James Carman also suggesting to disable it 
by default. Then I also looked at


https://github.com/swagger-api/swagger-core/blob/master/modules/swagger-jaxrs/src/main/java/io/swagger/jaxrs/config/BeanConfig.java#L220 



and could not see (and still do not see) how this property, if set to 
false, can have any effect on disabling the scan, the only thing I found 
that the actual setScan call was still needed.


But the problem came from the other angle :-).

I guess the easiest way around it is to return a setScan setter to 
3.2.x, given that in some (I'd say - rare) cases uses depend on 
BeanConfig's implementation, but in 3.2.x keep this value by default to 
'false' so that in most other cases people do not have do setScan(false).


Cheers, Sergey


On 07/10/17 13:47, Dennis Kieselhorst wrote:

Hi Taylor!

This causes the subsequent code (line 196):

BeanConfig beanConfig = appInfo == null
 ? new BeanConfig()
 : new ApplicationBeanConfig(appInfo.getProvider());

…to always instantiate an ApplicationBeanConfig object. Our 
production code relied on the BeanConfig’s implementation of its 
classes() method to grab our ReaderListener classes annotated with 
@SwaggerDefinition. Because a BeanConfig will never be instantiated 
this way anymore, we would no longer be able to use our current 
implementations of the ReaderListener interface.


Is this a known bug, or is there a workaround that I’m missing?


Can you create a PR with a unittest for your scenario?

Unfortunately there are many ways of using Swagger2Feature and obviously
we missed that one.

Regards
Dennis




--
Sergey Beryozkin

Talend Community Coders
http://coders.talend.com/