rombert commented on issue #51: SAML2 Service Provider Pull Request
URL: https://github.com/apache/sling-whiteboard/pull/51#issuecomment-611539465
 
 
   I've been thinking some more about how to make sure the reviews are 
productive and reduce the time needed to get this into the whiteboard.
   
   The idea I came up with takes two complementary approaches:
   
   1. Simplify testing
   1. Reduce the amount of submitted code
   
   For item 1 I suggest that you provide a docker script or docker-compose 
setup that launches a SAML-enable identity provider. One idea would be 
[Keycloak with 
Docker](https://www.keycloak.org/getting-started/getting-started-docker), but I 
admit I'm not at all familiar with identity providers to offer an informed 
suggestion.
   
   This would allow you to drop ~400 LOC in the `idp` package and maybe some 
other parts that are only used from there.
   
   For item 2 item 1 already helps :-) I think you can start with submitting 
the minimal functionality that works - and I get that is the 
AuthenticationHandler.
   
   I see code for the ExternalIdentityProvider and LoginModule as well. If that 
is not needed for the basic login flow, I would suggest submitting them as 
follow-up PRs once we merge the initial one.
   
   I also see a potential of dropping some code with the TokenStore class. You 
mention it's derived from the class in the `o.a.s.auth.form` bundle. Maybe 
that's something we can export, or you can inline the class in this bundle. The 
code looks quite complex and is large, I think it's a good idea to keep the 
duplication out of the bundle.
   
   Would that work for you?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

Reply via email to