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: allow skipping the consent flow #3451

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Conversation

hperl
Copy link
Contributor

@hperl hperl commented Feb 27, 2023

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • 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 the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@hperl hperl self-assigned this Feb 27, 2023
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #3451 (9973b2a) into master (50bc1b4) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 9973b2a differs from pull request most recent head 23e08bc. Consider uploading reports for the commit 23e08bc to get more accurate results

@@            Coverage Diff             @@
##           master    #3451      +/-   ##
==========================================
+ Coverage   76.81%   76.83%   +0.02%     
==========================================
  Files         123      123              
  Lines        9069     9069              
==========================================
+ Hits         6966     6968       +2     
+ Misses       1660     1659       -1     
+ Partials      443      442       -1     
Impacted Files Coverage Δ
oauth2/handler.go 66.50% <0.00%> (ø)
persistence/sql/persister_oauth2.go 82.88% <0.00%> (+0.76%) ⬆️

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

@hperl hperl marked this pull request as ready for review February 28, 2023 14:50
@hperl hperl requested a review from aeneasr as a code owner February 28, 2023 14:50
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.

Nice! Some nits

consent/handler.go Outdated Show resolved Hide resolved
client/handler_test.go Show resolved Hide resolved
@hperl hperl force-pushed the hperl/skip-consent-flow branch 3 times, most recently from 71072a3 to 8183092 Compare February 28, 2023 17:01
@hperl hperl linked an issue Mar 1, 2023 that may be closed by this pull request
6 tasks
This adds a new boolean parameter `skip_consent` to the admin APIs of
the OAuth clients. This parameter will be forwarded to the consent app
as `client.skip_consent`.

It is up to the consent app to act on this parameter, but the canonical
implementation accepts the consent on the user's behalf, similar to
when `skip` is set.
Copy link
Contributor

@kmherrmann kmherrmann left a comment

Choose a reason for hiding this comment

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

Amazing, thanks!
My review is obviously superficial wrt. to implementation details

Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

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

tiny nits

@@ -12,7 +12,7 @@ import (
"github.com/gobuffalo/pop/v6"
"github.com/gofrs/uuid"

jose "gopkg.in/square/go-jose.v2" // Naming the dependency jose is important for go-swagger to work, see https://github.com/go-swagger/go-swagger/issues/1587
"gopkg.in/square/go-jose.v2" // Naming the dependency jose is important for go-swagger to work, see https://github.com/go-swagger/go-swagger/issues/1587
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems to conflict with the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea IDK, my IDE (Goland) does this automatically. I think this is not needed anyways (because the package is named jose already), but we can remove the comment. OK?

Comment on lines +1376 to +1377
var ret bool
return ret
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var ret bool
return ret
return false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is generated code.

@aeneasr aeneasr merged commit 4f65365 into master Mar 2, 2023
@aeneasr aeneasr deleted the hperl/skip-consent-flow branch March 2, 2023 11:47
@bulolo
Copy link

bulolo commented Apr 9, 2023

how to use skip_consent?

@kmherrmann
Copy link
Contributor

harnash pushed a commit to Wikia/ory-hydra that referenced this pull request Apr 12, 2023
This adds a new boolean parameter `skip_consent` to the admin APIs of
the OAuth clients. This parameter will be forwarded to the consent app
as `client.skip_consent`.

It is up to the consent app to act on this parameter, but the canonical
implementation accepts the consent on the user's behalf, similar to
when `skip` is set.
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.

Skip consent flow in default Ory Network UI
5 participants