Introduce Related Resource Auth Override #248
Merged
Conversation
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
mbarackman-coursera
force-pushed
the
MPB-add-relation-auth-override
branch
2 times, most recently
from
ed8fc7d
to
2b61e7a
Compare
cliu587
reviewed
May 2, 2018
| @@ -268,4 +296,10 @@ class NaptimeResolver extends DeferredResolver[SangriaGraphQlContext] with Stric | |||
| } | |||
|
|
|||
|
|
|||
| private[this] def getInternalRequestContext(context: SangriaGraphQlContext): SangriaGraphQlContext = { | |||
| val originalRequestHeader = context.requestHeader | |||
| val newHeaders = context.requestHeader.headers.remove("X-Coursera-User").add("X-Coursera-Source" -> "internal") | |||
There was a problem hiding this comment.
is it possible to move this to assembler's RemoteFetcher? that way implementation detail of how coursera handles auth isn't leaked onto the Naptime project and I believe that is the place where we have the final headers and instantiate the client so it feels like a more natural place to do this conditional logic.
There was a problem hiding this comment.
yeah I think that's doable and that all sounds reasonable. let me see
mbarackman-coursera
force-pushed
the
MPB-add-relation-auth-override
branch
from
2b61e7a
to
4c2861b
Compare
cliu587
approved these changes
May 3, 2018
| @@ -20,4 +20,12 @@ record ReverseRelationAnnotation { | |||
| */ | |||
| relationType: enum RelationType { FINDER, MULTI_GET, GET, SINGLE_ELEMENT_FINDER } | |||
|
|
|||
| /** | |||
| * This is an optional field which will override the auth headers from the | |||
There was a problem hiding this comment.
let's fix up this comment to not refer to headers. indicating that a different auth strategy from the default behavior I think is fine.
| // Handle MultiGet and Non-Multigets differently, since multigets can be batched | ||
| val forwardRelations = | ||
| fetchForwardRelations(forwardRequests, resourceName, ctx) | ||
| // partition forward requests by auth type |
There was a problem hiding this comment.
nit: the comment should reflect that we need to make 1 call of fetchForwardRelations for each auth override type.
- This change allows API developers to create related resources that are fetched with “internal” auths. - This is done by adding an authOverride value to the reverse relation definition on the primary resource. - The purpose of this is to give developers the option to do authentication in only one place, the entry/parent resource, and use internal auth for all the related/child resources. This allows the child resources to be a bit more re-usable in that they can be attached to multiple parent resources that may all have different auth schemes.
mbarackman-coursera
force-pushed
the
MPB-add-relation-auth-override
branch
from
3ccf982
to
d142d80
Compare
amory-coursera
pushed a commit
that referenced
this pull request
May 3, 2018
* Introduce Related Resource Auth Override - This change allows API developers to create related resources that are fetched with “internal” auths. - This is done by adding an authOverride value to the reverse relation definition on the primary resource. - The purpose of this is to give developers the option to do authentication in only one place, the entry/parent resource, and use internal auth for all the related/child resources. This allows the child resources to be a bit more re-usable in that they can be attached to multiple parent resources that may all have different auth schemes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This change allows API developers to create related resources that are fetched with “internal” auths.
This is done by adding an authOverride value to the reverse relation definition on the primary resource. This will show up in the naptime resource schema and will be interpreted by assembler in order to request with the proper auth request headers.
The purpose of this is to give developers the option to do authentication in only one place, the entry/parent resource, and use internal auth for all the related/child resources. This allows the child resources to be a bit more re-usable in that they can be attached to multiple parent resources that may all have different auth schemes.