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: list consent sessions by session id #2853

Merged
merged 2 commits into from Dec 7, 2022

Conversation

aarmam
Copy link
Contributor

@aarmam aarmam commented Nov 12, 2021

This pull request introduces feature to list subject consent sessions by session id.

Use case:
When authentication is initiated without prompt parameter from multiple devices, we would like to distinguish in what session was the consent given.

Current situation:
GET /oauth2/auth/sessions/consent returns single consent and it references to the session id, where the consent was initially given. Furthermore if logout is performed from device initially gave the consent, the reference to login session id is cleared.
If prompt=consent were to be used, separate consents with separate session id would be returned and you would not have same problems. So this endpoint behaves differently in relation to how login session id is referenced.

Proposed solution:
Add additional query parameter login_session_id for GET /oauth2/auth/sessions/consent to return consents related to requested session id.
This solution does not change how login session id is referenced in result.

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 green light (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

Tests and documentation will be commited after inital acceptance of the proposed feature.

@aarmam aarmam requested a review from aeneasr as a code owner November 12, 2021 17:23
@aarmam aarmam marked this pull request as draft November 12, 2021 17: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.

The idea of this PR is to only return a subset of the user’s consents?

From implementation standpoint it looks good :)

@aarmam aarmam changed the title featl: list consent sessions by session id feat: list consent sessions by session id Nov 15, 2021
@aeneasr
Copy link
Member

aeneasr commented Nov 22, 2021

Let me know if ready for 👀 by clicking "ready for review"!

@aarmam aarmam force-pushed the feature/list-consent-sessions branch from b70391d to 26a6230 Compare November 24, 2021 08:30
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #2853 (20d5bbc) into master (316b582) will increase coverage by 0.13%.
The diff coverage is 94.20%.

@@            Coverage Diff             @@
##           master    #2853      +/-   ##
==========================================
+ Coverage   76.82%   76.95%   +0.13%     
==========================================
  Files         123      123              
  Lines        8976     9044      +68     
==========================================
+ Hits         6896     6960      +64     
- Misses       1652     1655       +3     
- Partials      428      429       +1     
Impacted Files Coverage Δ
consent/manager.go 100.00% <ø> (ø)
persistence/sql/persister_consent.go 87.41% <84.61%> (-0.18%) ⬇️
consent/handler.go 66.01% <100.00%> (+0.67%) ⬆️
consent/manager_test_helpers.go 97.93% <100.00%> (+0.10%) ⬆️

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

@aarmam aarmam force-pushed the feature/list-consent-sessions branch from 26a6230 to 0cc8f84 Compare December 8, 2021 09:52
@aarmam aarmam force-pushed the feature/list-consent-sessions branch 4 times, most recently from f3fee3a to b918410 Compare March 21, 2022 14:39
@aarmam aarmam force-pushed the feature/list-consent-sessions branch 2 times, most recently from e38a27a to 319ba6a Compare April 19, 2022 08:00
@aarmam aarmam force-pushed the feature/list-consent-sessions branch 4 times, most recently from c56c5f2 to 76cf185 Compare May 11, 2022 05:59
@aarmam aarmam marked this pull request as ready for review May 11, 2022 07:12
@aarmam aarmam marked this pull request as draft September 6, 2022 10:48
@aarmam aarmam force-pushed the feature/list-consent-sessions branch 3 times, most recently from db77826 to 5f9223b Compare September 14, 2022 07:53
@aarmam aarmam force-pushed the feature/list-consent-sessions branch 5 times, most recently from a8596e4 to e73baaf Compare September 15, 2022 09:43
@aarmam aarmam marked this pull request as ready for review September 15, 2022 10:17
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.

This looks very good! There is a small conflict with the v2 SDK naming but once that’s resolved I’ll merge this asap :)

@aarmam aarmam force-pushed the feature/list-consent-sessions branch from e73baaf to 4c29937 Compare November 1, 2022 14:50
@aarmam aarmam requested a review from aeneasr November 1, 2022 15:16
@aarmam aarmam force-pushed the feature/list-consent-sessions branch 3 times, most recently from 6298d9d to 1b307ba Compare November 4, 2022 10:22
@aeneasr aeneasr force-pushed the feature/list-consent-sessions branch from 1b307ba to 20d5bbc Compare December 6, 2022 17:43
@aeneasr aeneasr merged commit d275ad6 into ory:master Dec 7, 2022
harnash pushed a commit to Wikia/ory-hydra that referenced this pull request Apr 12, 2023
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.

None yet

2 participants