IdentityPython / oidc-op

An implementation of an OIDC Provider (OP)
Apache License 2.0
64 stars 26 forks source link

Refactor client authentication #187

Closed nsklikas closed 2 years ago

nsklikas commented 2 years ago

This PR introduces various changes to the client authentication flow. I believe that with these changes the flow is cleaner. This PR changes:

TODO:

rohe commented 2 years ago

I don't see the need for using @classmethod. Please enlighten me. Unless used in a factory context (which this isn't) I think using classmethod is more of a optimisation. Something we usually don't regard as a main objective. We also doesn't use this pattern in other places so adding it here will just confuse developers. I think we'd better use the pattern that are present in oidcmsg and not introduce classmethod when not needed.

nsklikas commented 2 years ago

@rohe I used classmethod to avoid having to initialize the class and use it as a singleton. I removed client_auth_setup for this reason, if you think that it would cause confusion then I shall change it back.

rohe commented 2 years ago

@nsklikas I think it will. So please do !

nsklikas commented 2 years ago

I would also like your opinion on another big change happening in this PR. In the past the none authn method would allow requests without a client_id. Now this changes.

Do you think there is a use case were we should allow such requests? The only one I can think of is for the registration endpoint. Still if we want to support this behavior I think the following change is needed: Separate the logic of the none authn method by introducing another authn method, let's call it public. The difference between none and public is that, in public, client_id will be mandatory. This approach would have the upside of not introducing any breaking changes (i.e. changing the behavior of none).

What do you think @rohe @peppelinux?

rohe commented 2 years ago

The none authn method was added for testing purposes and should never ever be used in production. Having said that I know that people uses whatever is provided in whatever way they find reasonable. That's life :-/

Anyway, I'm perfectly OK with adding public beside none as per your proposal.