Skip to content

feat: allow skipping the consent flow#3451

Merged
aeneasr merged 1 commit intomasterfrom
hperl/skip-consent-flow
Mar 2, 2023
Merged

feat: allow skipping the consent flow#3451
aeneasr merged 1 commit intomasterfrom
hperl/skip-consent-flow

Conversation

@hperl
Copy link
Copy Markdown
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
    [email protected]) 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
Copy Markdown

codecov bot commented Feb 27, 2023 •
edited
Loading

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 force-pushed the hperl/skip-consent-flow branch from 75796db to 87e9ad1 Compare February 28, 2023 14:50
@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
aeneasr
aeneasr reviewed Feb 28, 2023
View reviewed changes
Copy link
Copy Markdown
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

@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
Skip consent flow in default Ory Network UI ory/network#205
Closed
6 tasks
@hperl hperl force-pushed the hperl/skip-consent-flow branch from 8183092 to b383582 Compare March 1, 2023 14:25
@hperl
feat: allow skipping consent for trusted clients
23e08bc 
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.
@hperl hperl force-pushed the hperl/skip-consent-flow branch from b383582 to 23e08bc Compare March 1, 2023 14:35
@hperl hperl requested review from aeneasr and kmherrmann March 1, 2023 14:53
kmherrmann
kmherrmann approved these changes Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown
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

alnr
alnr requested changes Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown
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

client/client.go
"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
Copy Markdown
Contributor

@alnr alnr Mar 1, 2023

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
Copy Markdown
Contributor Author

@hperl hperl Mar 2, 2023

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?

internal/httpclient/model_o_auth2_client.go
Comment on lines +1376 to +1377
var ret bool
return ret
Copy link
Copy Markdown
Contributor

@alnr alnr Mar 1, 2023

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
Copy Markdown
Contributor Author

@hperl hperl Mar 2, 2023

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
Copy Markdown

bulolo commented Apr 9, 2023

how to use skip_consent?

@kmherrmann
Copy link
Copy Markdown
Contributor

kmherrmann commented Apr 11, 2023

Please see here: https://www.ory.sh/docs/oauth2-oidc/custom-login-consent/flow#skipping-consent-for-trusted-clients

harnash pushed a commit to Wikia/ory-hydra that referenced this pull request Apr 12, 2023
@hperl @harnash
feat: allow skipping consent for trusted clients (ory#3451)
698c748 
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

Reviewers

@alnr alnr alnr requested changes

@aeneasr aeneasr aeneasr approved these changes

+1 more reviewer

@kmherrmann kmherrmann kmherrmann approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hperl hperl

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Skip consent flow in default Ory Network UI

5 participants

@hperl @bulolo @kmherrmann @aeneasr @alnr