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

fix(auth): fix noninteractive grant creation #2003

Merged
merged 6 commits into from
Oct 19, 2023
Merged

fix(auth): fix noninteractive grant creation #2003

merged 6 commits into from
Oct 19, 2023

Conversation

njlie
Copy link
Contributor

@njlie njlie commented Oct 6, 2023

Changes proposed in this pull request

  • Refactored canSkipInteraction as a utility function.
  • Pulls in canSkipInteraction in the Grant service so its create function can properly create a noninteractive grant.

Context

Fixes #1968.

Grant creation was handling noninteractive grant creation by checking if there is an interact property in the grant request. But that property may be present in a request whose requested access does not necessitate interaction.

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Postman collection updated

@netlify
Copy link

netlify bot commented Oct 6, 2023 •
edited
Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 54b6d7f
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65310b1754e09d00082942da

@github-actions github-actions bot added type: tests Testing related type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels Oct 6, 2023
raducristianpopa
raducristianpopa reviewed Oct 9, 2023
View reviewed changes
Copy link
Member

@raducristianpopa raducristianpopa left a comment

Choose a reason for hiding this comment

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

LGTM. You need to run pnpm format. One question I have:

packages/auth/src/grant/service.test.ts
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth adding a test scenario where we provide all three accesses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a good idea, will add.

mkurapov
mkurapov previously approved these changes Oct 19, 2023
View reviewed changes
packages/auth/src/grant/service.test.ts Outdated Show resolved Hide resolved
packages/auth/src/grant/service.test.ts Outdated Show resolved Hide resolved
@sabineschaller sabineschaller merged commit a3a5353 into main Oct 19, 2023
21 of 22 checks passed
@sabineschaller sabineschaller deleted the nl/1968/noninteractive-grant branch October 19, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkurapov mkurapov mkurapov approved these changes

@raducristianpopa raducristianpopa raducristianpopa left review comments

@BlairCurrey BlairCurrey Awaiting requested review from BlairCurrey

@sabineschaller sabineschaller Awaiting requested review from sabineschaller

Assignees
No one assigned
Labels
pkg: auth Changes in the GNAP auth package. type: source Changes business logic type: tests Testing related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix non-interactive grant creation
4 participants
@njlie @mkurapov @sabineschaller @raducristianpopa