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

fix(Core): support identityClaim "sub" #794

Merged
merged 2 commits into from
Oct 5, 2020
Merged

fix(Core): support identityClaim "sub" #794

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08b10ee
fix(Core): support identityClaim "sub"
wooj2 Oct 1, 2020
e4968aa
updating per pr
wooj2 Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Amplify.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@
6B33896823AAACC900561E5B /* ReachabilityUpdate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B33896723AAACC900561E5B /* ReachabilityUpdate.swift */; };
6B767FB723AC092800C683ED /* AmplifyAPICategory+ReachabilityBehavior.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B767FB623AC092800C683ED /* AmplifyAPICategory+ReachabilityBehavior.swift */; };
6B767FB923AC0A0D00C683ED /* APICategoryReachabilityBehavior.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B767FB823AC0A0D00C683ED /* APICategoryReachabilityBehavior.swift */; };
6B9F7C5225267E1500F1F71C /* MockAWSAuthUser.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B9F7C5125267E1500F1F71C /* MockAWSAuthUser.swift */; };
6B9F7C552526864800F1F71C /* ScenarioATest6Post+Schema.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B9F7C532526864800F1F71C /* ScenarioATest6Post+Schema.swift */; };
6B9F7C562526864800F1F71C /* ScenarioATest6Post.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B9F7C542526864800F1F71C /* ScenarioATest6Post.swift */; };
6B9F7C59252687CC00F1F71C /* GraphQLRequestAuthIdentityClaimTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6B9F7C572526875A00F1F71C /* GraphQLRequestAuthIdentityClaimTests.swift */; };
6BB7441023A9954900B0EB6C /* DispatchSource+MakeOneOff.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6BB7440F23A9954900B0EB6C /* DispatchSource+MakeOneOff.swift */; };
6BBECD7123ADA7E100C8DFBE /* AmplifyAWSServiceConfiguration.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6BBECD7023ADA7E100C8DFBE /* AmplifyAWSServiceConfiguration.swift */; };
6BBECD7423ADA9D100C8DFBE /* AmplifyAWSServiceConfigurationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6BBECD7323ADA9D100C8DFBE /* AmplifyAWSServiceConfigurationTests.swift */; };
Expand Down Expand Up @@ -845,6 +849,10 @@
6B33896723AAACC900561E5B /* ReachabilityUpdate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ReachabilityUpdate.swift; sourceTree = "<group>"; };
6B767FB623AC092800C683ED /* AmplifyAPICategory+ReachabilityBehavior.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "AmplifyAPICategory+ReachabilityBehavior.swift"; sourceTree = "<group>"; };
6B767FB823AC0A0D00C683ED /* APICategoryReachabilityBehavior.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = APICategoryReachabilityBehavior.swift; sourceTree = "<group>"; };
6B9F7C5125267E1500F1F71C /* MockAWSAuthUser.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockAWSAuthUser.swift; sourceTree = "<group>"; };
6B9F7C532526864800F1F71C /* ScenarioATest6Post+Schema.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "ScenarioATest6Post+Schema.swift"; sourceTree = "<group>"; };
6B9F7C542526864800F1F71C /* ScenarioATest6Post.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ScenarioATest6Post.swift; sourceTree = "<group>"; };
6B9F7C572526875A00F1F71C /* GraphQLRequestAuthIdentityClaimTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLRequestAuthIdentityClaimTests.swift; sourceTree = "<group>"; };
6BAC32194A15ACB56F07DC87 /* Pods-AWSS3StoragePlugin.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AWSS3StoragePlugin.debug.xcconfig"; path = "Target Support Files/Pods-AWSS3StoragePlugin/Pods-AWSS3StoragePlugin.debug.xcconfig"; sourceTree = "<group>"; };
6BB7440F23A9954900B0EB6C /* DispatchSource+MakeOneOff.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "DispatchSource+MakeOneOff.swift"; sourceTree = "<group>"; };
6BBECD7023ADA7E100C8DFBE /* AmplifyAWSServiceConfiguration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AmplifyAWSServiceConfiguration.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1574,9 +1582,11 @@
children = (
219A888623EB89C200BBC5F2 /* GraphQLRequestAnyModelWithSyncTests.swift */,
21A3FDB8246494CD00E76120 /* GraphQLRequestAuthRuleTests.swift */,
6B9F7C572526875A00F1F71C /* GraphQLRequestAuthIdentityClaimTests.swift */,
2129BE322394828B006363A1 /* GraphQLRequestModelTests.swift */,
216E45F0248E971E0035E3CE /* GraphQLRequestNonModelTests.swift */,
214F497D2486DA5000DA616C /* GraphQLRequestOptionalAssociationTests.swift */,
6B9F7C5125267E1500F1F71C /* MockAWSAuthUser.swift */,
);
path = GraphQLRequest;
sourceTree = "<group>";
Expand Down Expand Up @@ -2308,6 +2318,8 @@
2129BE002394627B006363A1 /* PostCommentModelRegistration.swift */,
B9AA09F02473CA29000E6FBB /* PostStatus.swift */,
B952182F237E21B900F53237 /* schema.graphql */,
6B9F7C542526864800F1F71C /* ScenarioATest6Post.swift */,
6B9F7C532526864800F1F71C /* ScenarioATest6Post+Schema.swift */,
214F49742486D8A200DA616C /* User.swift */,
214F49752486D8A200DA616C /* User+Schema.swift */,
214F49762486D8A200DA616C /* UserFollowers.swift */,
Expand Down Expand Up @@ -4086,6 +4098,7 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
6B9F7C59252687CC00F1F71C /* GraphQLRequestAuthIdentityClaimTests.swift in Sources */,
2183A56923EA4A9100232880 /* GraphQLUpdateMutationTests.swift in Sources */,
21A3FDB9246494CD00E76120 /* GraphQLRequestAuthRuleTests.swift in Sources */,
21A3FDB62464590600E76120 /* ModelMultipleOwnerAuthRuleTests.swift in Sources */,
Expand All @@ -4103,6 +4116,7 @@
219A888723EB89C200BBC5F2 /* GraphQLRequestAnyModelWithSyncTests.swift in Sources */,
2183A56423EA4A7F00232880 /* GraphQLGetQueryTests.swift in Sources */,
6BBECD7423ADA9D100C8DFBE /* AmplifyAWSServiceConfigurationTests.swift in Sources */,
6B9F7C5225267E1500F1F71C /* MockAWSAuthUser.swift in Sources */,
21AD425A249C0D910016FE95 /* AnyModelTester.swift in Sources */,
2129BE3C2394828B006363A1 /* GraphQLRequestModelTests.swift in Sources */,
2183A56523EA4A8400232880 /* GraphQLListQueryTests.swift in Sources */,
Expand Down Expand Up @@ -4566,6 +4580,7 @@
21F40A3E23A295390074678E /* AWSMobileClient+Message.swift in Sources */,
FAA2E8BC239FFC7700E420EA /* MockModels.swift in Sources */,
B9FAA11E23879B9F009414B4 /* Book+Schema.swift in Sources */,
6B9F7C552526864800F1F71C /* ScenarioATest6Post+Schema.swift in Sources */,
FACA361B2327FC74000E74F6 /* MockLoggingCategoryPlugin.swift in Sources */,
FACF52042329633500646E10 /* TestExtensions.swift in Sources */,
216E460824913D430035E3CE /* Color.swift in Sources */,
Expand All @@ -4584,6 +4599,7 @@
B9FAA11023878C5E009414B4 /* UserProfile.swift in Sources */,
B9AA09F12473CA29000E6FBB /* PostStatus.swift in Sources */,
214F497B2486D8A200DA616C /* User+Schema.swift in Sources */,
6B9F7C562526864800F1F71C /* ScenarioATest6Post.swift in Sources */,
B9FAA12023879BD0009414B4 /* BookAuthor+Schema.swift in Sources */,
21F40A4023A295470074678E /* TestCommonConstants.swift in Sources */,
214F497C2486D8A200DA616C /* UserFollowers.swift in Sources */,
Expand Down
21 changes: 17 additions & 4 deletions AmplifyPlugins/Core/AWSPluginsCore/Model/Decorator/AuthRuleDecorator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ import Foundation
import Amplify

public enum AuthRuleDecoratorInput {
public typealias OwnerId = String
case subscription(GraphQLSubscriptionType, OwnerId)
case subscription(GraphQLSubscriptionType, AuthUser)
case mutation
case query
}
Expand Down Expand Up @@ -60,13 +59,14 @@ public struct AuthRuleDecorator: ModelBasedGraphQLDocumentDecorator {
let ownerField = authRule.getOwnerFieldOrDefault()
selectionSet = appendOwnerFieldToSelectionSetIfNeeded(selectionSet: selectionSet, ownerField: ownerField)

guard case let .subscription(_, ownerId) = input else {
guard case let .subscription(_, authUser) = input else {
return document.copy(selectionSet: selectionSet)
}

if isOwnerInputRequiredOnSubscription(authRule) {
var inputs = document.inputs
inputs[ownerField] = GraphQLDocumentInput(type: "String!", value: .scalar(ownerId))
let identityClaimValue = resolveIdentityClaimValue(authRule, authUser)
inputs[ownerField] = GraphQLDocumentInput(type: "String!", value: .scalar(identityClaimValue))
return document.copy(inputs: inputs, selectionSet: selectionSet)
}
return document.copy(selectionSet: selectionSet)
Expand All @@ -76,6 +76,19 @@ public struct AuthRuleDecorator: ModelBasedGraphQLDocumentDecorator {
return authRule.allow == .owner && authRule.getModelOperationsOrDefault().contains(.read)
}

private func resolveIdentityClaimValue(_ authRule: AuthRule, _ authUser: AuthUser) -> String {
guard let identityClaim = authRule.identityClaim else {
return authUser.username
}
if identityClaim == "username" {
return authUser.username
} else if identityClaim == "sub" {
return authUser.userId
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i think these might be "cognito:username" https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-codegen-appsync-model-plugin/src/__tests__/visitors/appsync-swift-visitor.test.ts#L1012

and "sub" looks correct: https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-codegen-appsync-model-plugin/src/__tests__/visitors/appsync-swift-visitor.test.ts#L1120

Copy link

Choose a reason for hiding this comment

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

This differs from the current Android logic, here. My understanding is that Android gets the username from the decrypted authToken instead of from getUsername, which is necessary to support email/phone sign in.

@TrekSoft can you confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested w/ schema of:

type ScenarioATest6Post @model @auth(rules: [{ allow: owner, identityClaim: "username" }]) {
  id: ID!
  title: String!
}

and

type ScenarioATest6Post @model @auth(rules: [{ allow: owner}]) {
  id: ID!
  title: String!
}

in both cases, in our model generated schema, we see:

    
    model.authRules = [
      rule(allow: .owner, ownerField: "owner", identityClaim: "cognito:username", operations: [.create, .update, .delete, .read])
    ]

Copy link
Contributor

Choose a reason for hiding this comment

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

change looks good as incremental improvement as it would support sub and username (iff user uses username for sign in). Considering Android's current implementation, we should probably keep #485 open to address the username as email and phone sign in to do a similar JWT parse to extract the username out

Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder, if the auth plugin's get username behaves differently on Android and iOS such that on iOS we can rely on getUsername

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't rely on getUsername - Cognito is comparing what is sent to what is in the token and it simplifies the code as well to just do it like Android is where we simply pull the right value from the token. With custom identity claims, we can pull whatever other value is specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TrekSoft & @richardmcclellan -- thanks for calling this out.. Looked into implementing the parsing of the JWT token, and it's a bit more work than expecting. For this reason, we are going to push this code in and view it as an incremental improvement, while keeping #485 open to ultimately fix it in the manner you have suggested. Thanks again for extra eyes on this.

//TODO: Resolve other identityClaim types
return authUser.username
}

/// First finds the first `model` SelectionSet. Then, only append it when the `ownerField` does not exist.
private func appendOwnerFieldToSelectionSetIfNeeded(selectionSet: SelectionSet,
ownerField: String) -> SelectionSet {
Expand Down
6 changes: 3 additions & 3 deletions ...ifyPlugins/Core/AWSPluginsCore/Model/GraphQLRequest/GraphQLRequest+AnyModelWithSync.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ protocol ModelSyncGraphQLRequestFactory {

static func subscription(to modelType: Model.Type,
subscriptionType: GraphQLSubscriptionType,
ownerId: String) -> GraphQLRequest<MutationSyncResult>
authUser: AuthUser) -> GraphQLRequest<MutationSyncResult>

static func syncQuery(modelType: Model.Type,
where predicate: QueryPredicate?,
Expand Down Expand Up @@ -109,12 +109,12 @@ extension GraphQLRequest: ModelSyncGraphQLRequestFactory {

public static func subscription(to modelType: Model.Type,
subscriptionType: GraphQLSubscriptionType,
ownerId: String) -> GraphQLRequest<MutationSyncResult> {
authUser: AuthUser) -> GraphQLRequest<MutationSyncResult> {

var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelType: modelType, operationType: .subscription)
documentBuilder.add(decorator: DirectiveNameDecorator(type: subscriptionType))
documentBuilder.add(decorator: ConflictResolutionDecorator())
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(subscriptionType, ownerId)))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(subscriptionType, authUser)))
let document = documentBuilder.build()

return GraphQLRequest<MutationSyncResult>(document: document.stringValue,
Expand Down
17 changes: 10 additions & 7 deletions ...SPluginsCoreTests/Model/Decorator/AuthRuleDecorator/ModelMultipleOwnerAuthRuleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,11 @@ class ModelMultipleOwnerAuthRuleTests: XCTestCase {

// Only the 'owner' inherently has `.create` operation, requiring the subscription operation to contain the input
func testModelMultipleOwner_OnCreateSubscription() {
let authUser = MockAWSAuthUser(username: "user1", userId: "123e4567-dead-beef-a456-426614174000")
var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelType: ModelMultipleOwner.self,
operationType: .subscription)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .onCreate))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onCreate, "111")))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onCreate, authUser)))
let document = documentBuilder.build()
let expectedQueryDocument = """
subscription OnCreateModelMultipleOwner($editors: String!, $owner: String!) {
Expand All @@ -236,15 +237,16 @@ class ModelMultipleOwnerAuthRuleTests: XCTestCase {
XCTFail("The document doesn't contain variables")
return
}
XCTAssertEqual(variables["owner"] as? String, "111")
XCTAssertEqual(variables["owner"] as? String, "user1")
}

// Each owner with `.update` operation requires the ownerField on the corresponding subscription operation
func testModelMultipleOwner_OnUpdateSubscription() {
let authUser = MockAWSAuthUser(username: "user1", userId: "123e4567-dead-beef-a456-426614174000")
var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelType: ModelMultipleOwner.self,
operationType: .subscription)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .onUpdate))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onUpdate, "111")))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onUpdate, authUser)))
let document = documentBuilder.build()
let expectedQueryDocument = """
subscription OnUpdateModelMultipleOwner($editors: String!, $owner: String!) {
Expand All @@ -263,16 +265,17 @@ class ModelMultipleOwnerAuthRuleTests: XCTestCase {
XCTFail("The document doesn't contain variables")
return
}
XCTAssertEqual(variables["owner"] as? String, "111")
XCTAssertEqual(variables["editors"] as? String, "111")
XCTAssertEqual(variables["owner"] as? String, "user1")
XCTAssertEqual(variables["editors"] as? String, "user1")
}

// Only the 'owner' inherently has `.delete` operation, requiring the subscription operation to contain the input
func testModelMultipleOwner_OnDeleteSubscription() {
let authUser = MockAWSAuthUser(username: "user1", userId: "123e4567-dead-beef-a456-426614174000")
var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelType: ModelMultipleOwner.self,
operationType: .subscription)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .onDelete))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onDelete, "111")))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onDelete, authUser)))
let document = documentBuilder.build()
let expectedQueryDocument = """
subscription OnDeleteModelMultipleOwner($editors: String!, $owner: String!) {
Expand All @@ -291,6 +294,6 @@ class ModelMultipleOwnerAuthRuleTests: XCTestCase {
XCTFail("The document doesn't contain variables")
return
}
XCTAssertEqual(variables["owner"] as? String, "111")
XCTAssertEqual(variables["owner"] as? String, "user1")
}
}
9 changes: 6 additions & 3 deletions .../AWSPluginsCoreTests/Model/Decorator/AuthRuleDecorator/ModelReadUpdateAuthRuleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,11 @@ class ModelReadUpdateAuthRuleTests: XCTestCase {

// The owner auth rule contains `.create` operation, requiring the subscription operation to contain the input
func testModelReadUpdateField_OnCreateSubscription() {
let authUser = MockAWSAuthUser(username: "user1", userId: "123e4567-dead-beef-a456-426614174000")
var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelType: ModelReadUpdateField.self,
operationType: .subscription)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .onCreate))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onCreate, "111")))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onCreate, authUser)))
let document = documentBuilder.build()
let expectedQueryDocument = """
subscription OnCreateModelReadUpdateField {
Expand All @@ -221,10 +222,11 @@ class ModelReadUpdateAuthRuleTests: XCTestCase {

// Others can `.update` this model, which means the update subscription does not require owner input
func testModelReadUpdateField_OnUpdateSubscription() {
let authUser = MockAWSAuthUser(username: "user1", userId: "123e4567-dead-beef-a456-426614174000")
var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelType: ModelReadUpdateField.self,
operationType: .subscription)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .onUpdate))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onUpdate, "111")))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onUpdate, authUser)))
let document = documentBuilder.build()
let expectedQueryDocument = """
subscription OnUpdateModelReadUpdateField {
Expand All @@ -242,10 +244,11 @@ class ModelReadUpdateAuthRuleTests: XCTestCase {

// The owner auth rule contains `.delete` operation, requiring the subscription operation to contain the input
func testModelReadUpdateField_OnDeleteSubscription() {
let authUser = MockAWSAuthUser(username: "user1", userId: "123e4567-dead-beef-a456-426614174000")
var documentBuilder = ModelBasedGraphQLDocumentBuilder(modelType: ModelReadUpdateField.self,
operationType: .subscription)
documentBuilder.add(decorator: DirectiveNameDecorator(type: .onDelete))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onDelete, "111")))
documentBuilder.add(decorator: AuthRuleDecorator(.subscription(.onDelete, authUser)))
let document = documentBuilder.build()
let expectedQueryDocument = """
subscription OnDeleteModelReadUpdateField {
Expand Down
Loading