This repository has been archived by the owner on Jun 13, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Breaking changes to SmartAuthProvider #170
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the
client_credientials
grant flow, are we banking ontokenParams
having a scope property?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - if it has it, we will use it. But it doesn't need it.
In most OAuth 2.0 libraries, including simple-oauth2, you get to specify the "default" scope you want to use for a given Authorization Code grant flow request. When you want to support different grant flows with the same config, things get a little trickier because
scope
is also used in the Credentials Grant flow.How do they each work?
In the Authorization Code grant flow the
scope
value is used during the authorization URL creation step (the place we redirect the users to first).scope
is commonly used as a global config value that is used for every single authorization URL, but there's no reason that you can't change it per request.This might make sense for example when you have an app that flexibly lets the user select which scopes they want to give to the app. You could in advance ask the user what they want to give you, before redirecting them to the authorization URL.
The param
scope
also happens to be used by the Client Credentials grant flow but it's very rarely ever going to make sense to have the same default values. Making a Client Credentials grant flow with scopes that are the same as a user is sort of disjoint from the point of the CC grant flow: to give you application instead of user-level access to some resources. Worse, SMART App Launch as a spec says absolutely nothing about Client Credentials.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if you set up a SmartAuthProvider with the Client Credentials grant flow, we're going to listen to anything you want to specify in
tokenParams
but no other defaults.