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.

Sign up for GitHub

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

Jump to bottom

feat: Token Exchange (RFC 8693) #255

Merged
merged 7 commits into from
Feb 19, 2023
Merged

feat: Token Exchange (RFC 8693) #255

merged 7 commits into from
Feb 19, 2023

Conversation

lefelys
Copy link
Contributor

@lefelys lefelys commented Dec 20, 2022 •
edited
Loading

This PR implements OAuth2 Token Exchange in OP according to RFC 8693 (and client code)

Some implementation details:

  • OP parses and verifies subject/actor tokens natively if they were issued by OP
  • Third-party tokens verification is also possible by implementing additional storage interface
  • Token exchange can issue only OP's native tokens (id_token, access_token and refresh_token) with static issuer

@fforootd
Copy link
Member

👋 Hi There

Thank you for this PR, this looks awesome.

Please give us some time to schedule the review of this. Since the holiday season comes up our resources are a little tight 😁

Question: @livio-a would this be something where also @stebenz could assist?

@codecov
Copy link

codecov bot commented Dec 20, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (next@9291ca9). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             next     #255   +/-   ##
=======================================
  Coverage        ?   40.95%           
=======================================
  Files           ?       78           
  Lines           ?     6641           
  Branches        ?        0           
=======================================
  Hits            ?     2720           
  Misses          ?     3642           
  Partials        ?      279

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@livio-a
Copy link
Member

livio-a commented Jan 10, 2023

Hey @lefelys

First of all, thank you very much for this PR. Unfortunately, i did not have time to have a close look at it.
I'll try later this week of at least the beginning of next.

@muhlemmer muhlemmer changed the base branch from main to next February 8, 2023 11:55
@muhlemmer muhlemmer self-assigned this Feb 8, 2023
muhlemmer
muhlemmer requested changes Feb 8, 2023
View reviewed changes
Copy link
Collaborator

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

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

Once again thanks for this great PR. I reviewed your changes and left some comments. I'm new at the zitadel firm, so I'll also try to get @livio-a to go over my review asap as well.

I've modified the the PR to target the next branch, as I see that some interfaces carry additional methods which implies breaking changes. Can you please be so kind to rebase against the next branch so that the Go 1.20 test get triggered properly (merge requirement), and resolve the merge conflict.

pkg/op/token_request.go Outdated Show resolved Hide resolved
pkg/op/storage.go Outdated Show resolved Hide resolved
pkg/op/storage.go Outdated Show resolved Hide resolved
pkg/oidc/token.go Show resolved Hide resolved
pkg/oidc/token_request.go Outdated Show resolved Hide resolved
pkg/client/rs/resource_server.go Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/op/token.go Outdated Show resolved Hide resolved
pkg/op/token.go Outdated Show resolved Hide resolved
example/server/storage/storage.go Show resolved Hide resolved
muhlemmer
muhlemmer reviewed Feb 16, 2023
View reviewed changes
pkg/op/token.go Outdated Show resolved Hide resolved
@muhlemmer
Copy link
Collaborator

I've updated the comments, after going though them with my colleague. Irrelevant ones are put on resolved. For the ones that remain, a kind request to solve them.

I took the liberty to merge next into your branch and resolve the merge conflicts, as there were a lot of changes on our side in the last weeks.

@muhlemmer muhlemmer merged commit 8e29879 into zitadel:next Feb 19, 2023
@muhlemmer
Copy link
Collaborator

awesome, thanks! 🙏

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

🎉 This PR is included in version 2.0.0-next.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

muhlemmer added a commit that referenced this pull request Mar 16, 2023
@muhlemmer
chore: update features in readme
b218484 
- rotated features table for better rendering
- add links to specifications in feature table
- remove redundant links from the resources section
- changed "Token Exhange" feature to full yes (PR #255)
- add "Device Authorization" with full yes (PR #285)
@muhlemmer muhlemmer mentioned this pull request Mar 16, 2023
Merged
muhlemmer added a commit that referenced this pull request Mar 16, 2023
@muhlemmer
chore: update features in readme
62caf5d 
- rotated features table for better rendering
- add links to specifications in feature table
- remove redundant links from the resources section
- changed "Token Exhange" feature to full yes (PR #255)
- add "Device Authorization" with full yes (PR #285)
@muhlemmer muhlemmer mentioned this pull request Mar 21, 2023
Closed
@lefelys lefelys deleted the token-exchange branch July 13, 2023 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muhlemmer muhlemmer muhlemmer approved these changes

Assignees

@muhlemmer muhlemmer

Labels
released on @next released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lefelys @fforootd @livio-a @muhlemmer