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

feat: add client authentication strategy #565

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

narg95
Copy link
Contributor

@narg95 narg95 commented Feb 18, 2021

Related issue

#564

Proposed changes

  • Add a ClientAuthenticationStrategy type to provide an extension point for custom client authentication methods
  • Add Fosite.DefaultClientAuthentication as fosite's default and backward compatible client authentication strategy. Its content is 100% the current fosite's client authentication logic
  • Add method Fosite.WithDefaultClientAuthentication to use fosite's default client authentication strategy

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • [ x] I have added tests that prove my fix is effective or that my feature
    works.
  • [x ] I have added necessary documentation within the code base (if
    appropriate).

@narg95 narg95 changed the title add client authentication strategy feat: add client authentication strategy Feb 18, 2021
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you, this makes sense to me!

However, I think it would make sense to have a small helper function like f.authenticateClient() which returns the default strategy unless another strategy was set. This will prevent potential panics for people who forget to call WithDefaultClientAuthentication

@narg95
Copy link
Contributor Author

narg95 commented Feb 18, 2021

However, I think it would make sense to have a small helper function like f.authenticateClient() which returns the default strategy unless another strategy was set. This will prevent potential panics for people who forget to call WithDefaultClientAuthentication

great idea, it's done! Actually it made the pull request is even smaller

@narg95 narg95 requested a review from aeneasr February 20, 2021 02:23
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! 🎉 Your contribution makes Ory better :)

@aeneasr
Copy link
Member

aeneasr commented Feb 23, 2021

Oh, unfortunately, for some reason, the CircleCI tests are not running. Do you maybe have a fork on CircleCI of ory/fosite that you follow? If so, you need to unsubscribe / unwatch that fork. Then, you need to make another push to your branch using:

$ git commit --amend --allow-empty
$ git push --force

That should get the CI running! Thank you :)

@narg95
Copy link
Contributor Author

narg95 commented Feb 23, 2021

It works now. I did nothing with CircleCI. I unsubscribed my branch from Travis-CI and amended the commit. One of the two made it work :)

@aeneasr
Copy link
Member

aeneasr commented Feb 23, 2021

Nice, thank you! :)

@aeneasr aeneasr merged commit ec0bec2 into ory:master Feb 23, 2021
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.

2 participants