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: device authorization RFC 8628 #5646

Merged
merged 24 commits into from
Apr 19, 2023
Merged

feat: device authorization RFC 8628 #5646

merged 24 commits into from
Apr 19, 2023

Conversation

muhlemmer
Copy link
Contributor

@muhlemmer muhlemmer commented Apr 10, 2023

This adds support for RFC 8628, device authorization grant. The flow is build to work on top of the current login (Go UI):

  1. Client device initiates a request:
    a. It receives a User Code and URL that are printed to the user. (Also a complete link is sent)
    b. The device keeps polling the token endpoint until the device authorization grant is completed (Allowed, Denied or Exipired)
  2. User navigates their browser to the provided URL and enters the User Code.
  3. The user is then forwarded into the Login UI.
  4. When login is completed, the user is redirected to a confirmation page and given the choice to Allow or Deny the device authorization.
  5. After the user allows the device, the device will receive a token on the next poll.

Definition of Ready

  • I am happy with the code
  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • No debug or dead code
  • Critical parts are tested automatically
  • Where possible E2E tests are implemented
  • Documentation/examples are up-to-date
  • All non-functional requirements are met
  • Functionality of the acceptance criteria is checked manually on the dev system.

@vercel
Copy link

vercel bot commented Apr 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2023 8:32am

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #5646 (fd6645a) into main (991a563) will increase coverage by 0.00%.
The diff coverage is 58.41%.

@@           Coverage Diff            @@
##             main    #5646    +/-   ##
========================================
  Coverage   48.42%   48.42%            
========================================
  Files         693      699     +6     
  Lines       72237    72734   +497     
========================================
+ Hits        34981    35222   +241     
- Misses      35620    35857   +237     
- Partials     1636     1655    +19     
Impacted Files Coverage Δ
internal/api/grpc/admin/export.go 0.00% <0.00%> (ø)
internal/api/grpc/admin/server.go 0.00% <0.00%> (ø)
internal/api/grpc/management/idp_converter.go 9.39% <0.00%> (-0.06%) ⬇️
...i/grpc/management/project_application_converter.go 0.00% <0.00%> (ø)
internal/api/grpc/management/server.go 0.00% <0.00%> (ø)
internal/api/grpc/server/gateway.go 3.84% <0.00%> (-5.53%) ⬇️
.../api/grpc/server/middleware/service_interceptor.go 0.00% <0.00%> (ø)
internal/api/grpc/server/server.go 0.00% <0.00%> (ø)
internal/api/grpc/system/server.go 0.00% <0.00%> (ø)
internal/command/project_application_saml_model.go 61.79% <0.00%> (+0.34%) ⬆️
... and 31 more

... and 4 files with indirect coverage changes

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

@muhlemmer
Copy link
Contributor Author

I'm mostly done, this still needs to be clarified (already posted in chat):

I rechecked the acceptance criteria and I should add the Device Authorization Grant also to the App / Client config. I've added OIDC_GRANT_TYPE_DEVICE_CODE to app.proto, regenerated and implemented the various type converters.

The question: should I check and enforce if the grant type is configured for the client and reject the request if it is not?

I'm asking because when I text/reference search for the other grant types, I don't see any enforcements on them either. Just oidc compliance checks.

I will also create a follow-up issue for console changes.

@muhlemmer
Copy link
Contributor Author

Enforcement of the grant type is now implemented:

// TODO(muhlemmer): Remove the following code block with oidc v3
// https://github.com/zitadel/oidc/issues/370
client, err := o.GetClientByClientID(ctx, clientID)
if err != nil {
return err
}
if !op.ValidateGrantType(client, oidc.GrantTypeDeviceCode) {
return errors.ThrowPermissionDeniedf(nil, "OIDC-et1Ae", "grant type %q not allowed for client", oidc.GrantTypeDeviceCode)
}

Copy link
Member

@adlerhurst adlerhurst left a comment

Choose a reason for hiding this comment

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

reviewed code

internal/api/oidc/device_auth.go Outdated Show resolved Hide resolved
internal/api/oidc/device_auth.go Outdated Show resolved Hide resolved
internal/api/oidc/device_auth.go Show resolved Hide resolved
internal/api/oidc/device_auth.go Outdated Show resolved Hide resolved
internal/api/oidc/device_auth.go Outdated Show resolved Hide resolved
internal/api/api.go Outdated Show resolved Hide resolved
internal/api/ui/login/renderer.go Outdated Show resolved Hide resolved
internal/api/ui/login/device_auth.go Outdated Show resolved Hide resolved
internal/domain/device_auth.go Outdated Show resolved Hide resolved
internal/repository/deviceauth/aggregate.go Show resolved Hide resolved
internal/api/oidc/device_auth.go Show resolved Hide resolved
internal/api/oidc/device_auth.go Show resolved Hide resolved
@muhlemmer muhlemmer merged commit 5819924 into main Apr 19, 2023
6 checks passed
@muhlemmer muhlemmer deleted the feat-device-authz branch April 19, 2023 08:46
@github-actions
Copy link

🎉 This PR is included in version 2.24.0-ignore-me2.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

Device Authorization Grant
2 participants