Feat(GraphQL): This PR adds auth switch in GraphQL authorization header. #6779

JatinDev543 · 2020-10-22T10:54:17Z

Fixes GRAPHQL-713
This PR adds auth switch closedByDefault in graphql authorization header which can be turned on to apply jwt checking on types that don't have auth directive.

This change is

Docs Preview:

abhimanyusinghgaur

Reviewable status: 0 of 11 files reviewed, 17 unresolved discussions (waiting on @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/authorization/auth.go, line 136 at r1 (raw file):

	idx := authMetaRegex.FindAllStringSubmatchIndex(authInfo, -1)
	glog.Infof("%s", idx)

stale

graphql/authorization/auth.go, line 145 at r1 (raw file):

	meta.Namespace = authInfo[idx[0][6]:idx[0][7]]
	meta.Algo = authInfo[idx[0][8]:idx[0][9]]
	meta.VerificationKey = authInfo[idx[0][10]:idx[0][11]]

so, we are not providing support for this in the old format. One way to get users to move to the new format :)

graphql/e2e/auth/auth_test.go, line 128 at r1 (raw file):

closedByDefault bool

don't see it used here, should be removed?

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 44 at r1 (raw file):

Quoted 199 lines of code…

type Region struct {
	Id     string         `json:"id,omitempty"`
	Name   string         `json:"name,omitempty"`
	Users  []*common.User `json:"users,omitempty"`
	Global bool           `json:"global,omitempty"`
}
type Movie struct {
	Id               string    `json:"id,omitempty"`
	Content          string    `json:"content,omitempty"`
	Hidden           bool      `json:"hidden,omitempty"`
	RegionsAvailable []*Region `json:"regionsAvailable,omitempty"`
}
type Issue struct {
	Id    string       `json:"id,omitempty"`
	Msg   string       `json:"msg,omitempty"`
	Owner *common.User `json:"owner,omitempty"`
}
type Log struct {
	Id     string `json:"id,omitempty"`
	Logs   string `json:"logs,omitempty"`
	Random string `json:"random,omitempty"`
}
type ComplexLog struct {
	Id      string `json:"id,omitempty"`
	Logs    string `json:"logs,omitempty"`
	Visible bool   `json:"visible,omitempty"`
}
type Role struct {
	Id         string         `json:"id,omitempty"`
	Permission string         `json:"permission,omitempty"`
	AssignedTo []*common.User `json:"assignedTo,omitempty"`
}
type Ticket struct {
	Id         string         `json:"id,omitempty"`
	OnColumn   *Column        `json:"onColumn,omitempty"`
	Title      string         `json:"title,omitempty"`
	AssignedTo []*common.User `json:"assignedTo,omitempty"`
}
type Column struct {
	ColID     string    `json:"colID,omitempty"`
	InProject *Project  `json:"inProject,omitempty"`
	Name      string    `json:"name,omitempty"`
	Tickets   []*Ticket `json:"tickets,omitempty"`
}
type Project struct {
	ProjID  string    `json:"projID,omitempty"`
	Name    string    `json:"name,omitempty"`
	Roles   []*Role   `json:"roles,omitempty"`
	Columns []*Column `json:"columns,omitempty"`
}
type Student struct {
	Id    string `json:"id,omitempty"`
	Email string `json:"email,omitempty"`
}
type Task struct {
	Id          string            `json:"id,omitempty"`
	Name        string            `json:"name,omitempty"`
	Occurrences []*TaskOccurrence `json:"occurrences,omitempty"`
}
type TaskOccurrence struct {
	Id   string `json:"id,omitempty"`
	Due  string `json:"due,omitempty"`
	Comp string `json:"comp,omitempty"`
}
type TestCase struct {
	user            string
	role            string
	result          string
	name            string
	filter          map[string]interface{}
	variables       map[string]interface{}
	query           string
	closedByDefault bool
}
type uidResult struct {
	Query []struct {
		UID string
	}
}
type Tasks []Task
func (tasks Tasks) add(t *testing.T) {
	getParams := &common.GraphQLParams{
		Query: `
		mutation AddTask($tasks : [AddTaskInput!]!) {
		  addTask(input: $tasks) {
			numUids
		  }
		}
		`,
		Variables: map[string]interface{}{"tasks": tasks},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func (r *Region) add(t *testing.T, user, role string) {
	getParams := &common.GraphQLParams{
		Headers: common.GetJWT(t, user, role, metaInfo),
		Query: `
		mutation addRegion($region: AddRegionInput!) {
		  addRegion(input: [$region]) {
			numUids
		  }
		}
		`,
		Variables: map[string]interface{}{"region": r},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func (r *Region) delete(t *testing.T, user, role string) {
	getParams := &common.GraphQLParams{
		Headers: common.GetJWT(t, user, role, metaInfo),
		Query: `
		mutation deleteRegion($name: String) {
		  deleteRegion(filter:{name: { eq: $name}}) {
			msg
		  }
		}
		`,
		Variables: map[string]interface{}{"name": r.Name},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func TestOptimizedNestedAuthQuery(t *testing.T) {
	query := `
	query {
	  queryMovie {
		content
		 regionsAvailable {
		  name
		  global
		}
	  }
	}
	`
	user := "user1"
	role := "ADMIN"
	getUserParams := &common.GraphQLParams{
		Headers: common.GetJWT(t, user, role, metaInfo),
		Query:   query,
	}
	gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
	beforeTouchUids := gqlResponse.Extensions["touched_uids"]
	beforeResult := gqlResponse.Data
	// Previously, Auth queries would have touched all the new `Regions`. But after the optimization
	// we should only touch necessary `Regions` which are assigned to some `Movie`. Hence, adding
	// these extra `Regions` would not increase the `touched_uids`.
	var regions []Region
	for i := 0; i < 100; i++ {
		r := Region{
			Name:   fmt.Sprintf("Test_Region_%d", i),
			Global: true,
		}
		r.add(t, user, role)
		regions = append(regions, r)
	}
	gqlResponse = getUserParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
	afterTouchUids := gqlResponse.Extensions["touched_uids"]
	require.Equal(t, beforeTouchUids, afterTouchUids)
	require.Equal(t, beforeResult, gqlResponse.Data)
	// Clean up
	for _, region := range regions {
		region.delete(t, user, role)
	}
}
func (s Student) deleteByEmail(t *testing.T) {
	getParams := &common.GraphQLParams{
		Query: `
			mutation delStudent ($filter : StudentFilter!){
			  	deleteStudent (filter: $filter) {
					numUids
			  	}
			}
		`,
		Variables: map[string]interface{}{"filter": map[string]interface{}{
			"email": map[string]interface{}{"eq": s.Email},
		}},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func (s Student) add(t *testing.T) {
	mutation := &common.GraphQLParams{
		Query: `
		mutation addStudent($student : AddStudentInput!) {
			addStudent(input: [$student]) {
				numUids
			}
		}`,
		Variables: map[string]interface{}{"student": s},
	}
	result := `{"addStudent":{"numUids": 1}}`
	gqlResponse := mutation.ExecuteAsPost(t, graphqlURL)
	common.RequireNoGQLErrors(t, gqlResponse)
	require.JSONEq(t, result, string(gqlResponse.Data))
}

I don't think we need to copy-paste all of this.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 290 at r1 (raw file):

Quoted 17 lines of code…

		{
			name: "Invalid JWT - type with auth directive",
			query: `
	          	mutation addUser($user: AddUserSecretInput!) {
		        	addUserSecret(input: [$user]) {
				      userSecret {
					    aSecret
				      }
			       }
	        	}`,
			user:   "user1",
			result: `{"addUserSecret":{"usersecret":[{"aSecret":"secret1"}]}}`,
			variables: map[string]interface{}{"user": &common.UserSecret{
				ASecret: "secret1",
				OwnedBy: "user1",
			}},
		},

Ideally, we should only be testing for Missing JWT cases, is there a reason we are testing for invalid JWT too here? That would anyways be tested in e2e/auth/auth_test.go already.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 346 at r1 (raw file):

Quoted 26 lines of code…

	for _, tcase := range testCases {
		getUserParams := &common.GraphQLParams{
			Query:     tcase.query,
			Variables: tcase.variables,
		}
		testMissingJWT := strings.HasPrefix(tcase.name, "Missing JWT")
		testInvalidKey := strings.HasPrefix(tcase.name, "Invalid JWT")
		if testInvalidKey {
			getUserParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
			jwtVar := getUserParams.Headers.Get(metaInfo.Header)
			// Create a invalid JWT signature.
			jwtVar = jwtVar + "A"
			getUserParams.Headers.Set(metaInfo.Header, jwtVar)
		} else if !testMissingJWT {
			getUserParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
		}
		gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
		require.Equal(t, len(gqlResponse.Errors), 1)
		if testMissingJWT {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"Jwt is required when ClosedByDefault flag is true")
		} else if testInvalidKey {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"unable to parse jwt token:token signature is invalid")
		}
	}

if we are testing only for missing JWT, then we should modify this to consider only that scenario.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 402 at r1 (raw file):

Quoted 22 lines of code…

		{name: "Missing JWT - type with auth field",
			query: `
			query {
				queryProject {
					name
				}
			}`,
			user:   "user1",
			role:   "ADMIN",
			result: `{"queryProject":[]}`,
		},
		{name: "Missing JWT - type without auth field",
			query: `
			query {
				queryTodo {
					owner
				}
			}`,
			user:   "user1",
			role:   "ADMIN",
			result: `{"queryTodo":[]}`,
		},

I believe only these two test cases are enough, we shouldn't be needing the others in this test.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 455 at r1 (raw file):

Quoted 27 lines of code…

	for _, tcase := range testCases {
		queryParams := &common.GraphQLParams{
			Query: tcase.query,
		}
		testMissingJWT := strings.HasPrefix(tcase.name, "Missing JWT")
		testInvalidKey := strings.HasPrefix(tcase.name, "Invalid JWT")
		if testInvalidKey {
			queryParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
			jwtVar := queryParams.Headers.Get(metaInfo.Header)
			// Create a invalid JWT signature.
			jwtVar = jwtVar + "A"
			queryParams.Headers.Set(metaInfo.Header, jwtVar)
		} else if (tcase.user != "" || tcase.role != "") && (!testMissingJWT) {
			queryParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
		}
		gqlResponse := queryParams.ExecuteAsPost(t, graphqlURL)
		if testMissingJWT {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"Jwt is required when ClosedByDefault flag is true")
		} else if testInvalidKey {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"unable to parse jwt token:token signature is invalid")
		}
		if diff := cmp.Diff(tcase.result, string(gqlResponse.Data)); diff != "" {
			t.Errorf("Test: %s result mismatch (-want +got):\n%s", tcase.name, diff)
		}
	}

if we are testing only for missing JWT, then we should modify this to consider only that scenario.

graphql/e2e/common/common.go, line 199 at r1 (raw file):

Quoted 5 lines of code…

type Todo struct {
	Id    string `json:"id,omitempty"`
	Text  string `json:"text,omitempty"`
	Owner string `json:"owner,omitempty"`
}

do we need this here?
I don't see a need here.
This should go to auth_ClosedByDefault/auth_test.go because it is used only there.

graphql/resolve/auth_add_test.yaml, line 1052 at r1 (raw file):

Quoted 41 lines of code…

- name: "Query with missing jwt token - Add with top level not RBAC false."
  gqlquery: |
    mutation addComplexLog($log: AddComplexLogInput!) {
      addComplexLog(input: [$log]) {
        complexLog {
          id
        }
      }
    }
  jwtvar:
    ROLE: "USER"
    USER: "user1"
  variables: |
    { "log":
      { "logs": "log123",
        "visible": true
      }
    }
  uids: |
    { "ComplexLog1": "0x123" }
  error:
    { "message": "mutation failed because authorization failed because Jwt is required when ClosedByDefault flag is true"}
- name: "Query with missing jwt token - Type without Auth"
  gqlquery: |
    mutation addTodo($todo: AddTodoInput!) {
      addTodo(input: [$todo]) {
        todo {
          id
          owner
          text
        }
      }
    }
  variables: |
    { "todo":
      { "owner": "Alice",
        "text":  "Hi Graphql"
      }
    }
  error:
    { "message": "mutation failed because authorization failed because Jwt is required when ClosedByDefault flag is true"}

since these two cases are to be run only when closeByDefault=true, I believe instead of adding them here, we should just create test cases in the go file. So, they are not run for cases where closeByDefault=false. OR just have a separate yaml file to run these.

graphql/resolve/auth_add_test.yaml, line 1073 at r1 (raw file):

Quoted 19 lines of code…

- name: "Invalid JWT - Type without Auth"
  gqlquery: |
    mutation addTodo($todo: AddTodoInput!) {
      addTodo(input: [$todo]) {
        todo {
          id
          owner
          text
        }
      }
    }
  variables: |
    { "todo":
      { "owner": "Alice",
        "text":  "Hi Graphql"
      }
    }
  error:
    { "message": "mutation failed because authorization failed because unable to parse jwt token:token signature is invalid"}

I don't understand why do we need this.

graphql/resolve/auth_query_test.yaml, line 1065 at r1 (raw file):

Quoted 17 lines of code…

- name: "Invalid JWT - type without auth directive"
  gqlquery: |
    query {
      queryTodo {
        id
        owner
        text
      }
    }
  dgquery: |-
    query {
      queryRTodo(func: type(Todo)) {
        owner : Todo.comment
        text :  Todo.text
        dgraph.uid : uid
      }
    }

These Invalid JWT tests shouldn't be needed, right?

graphql/resolve/auth_test.go, line 388 at r1 (raw file):

Quoted 23 lines of code…

				if strings.HasPrefix(tcase.Name, "Invalid JWT") {
					authMeta.PublicKey = "invalidkey"
				}
				ctx, err = authMeta.AddClaimsToContext(ctx)
				require.NoError(t, err)
				authMeta.PublicKey = publicKeyTemp
			}
			dgQuery, err := testRewriter.Rewrite(ctx, gqlQuery)
			if ClosedByDefault {
				if strings.HasPrefix(tcase.Name, "Query with missing jwt token") {
					require.Equal(t, err.Error(), "Jwt is required when ClosedByDefault flag is true")
					require.Nil(t, dgQuery)
				} else if strings.HasPrefix(tcase.Name, "Invalid JWT") {
					require.Equal(t, err.Error(), "unable to parse jwt token:token signature is invalid")
					require.Nil(t, dgQuery)
				} else {
					require.Nil(t, err)
					require.Equal(t, tcase.DGQuery, dgraph.AsString(dgQuery))
				}
			} else {
				require.Nil(t, err)
				require.Equal(t, tcase.DGQuery, dgraph.AsString(dgQuery))
			}

we shouldn't have to do this on a case by case basis.

graphql/resolve/auth_test.go, line 732 at r1 (raw file):

Quoted 8 lines of code…

	if !strings.HasPrefix(tcase.Name, "Query with missing jwt token") {
		if strings.HasPrefix(tcase.Name, "Invalid JWT") {
			authMeta.PublicKey = "invalidkey"
		}
		ctx, err = authMeta.AddClaimsToContext(ctx)
		require.NoError(t, err)
		authMeta.PublicKey = publicKeyTemp
	}

we shouldn't be doing this

graphql/resolve/auth_test.go, line 767 at r1 (raw file):

Quoted 5 lines of code…

	if ClosedByDefault {
		if strings.HasPrefix(tcase.Name, "Query with missing jwt token") || strings.HasPrefix(tcase.Name, "Invalid JWT") {
			require.Equal(t, resolved.Err.Error(), tcase.Error.Error())
		}
	} else if tcase.Error != nil {

this shouldn't be needed

testutil/graphql.go, line 156 at r1 (raw file):

closedByDefault

do we need to add this? doesn't seem to be used anywhere. Also if it were meant to be used outside of this package then shouldn't it be PascalCase? like: ClosedByDefault?

testutil/graphql.go, line 208 at r1 (raw file):

 ClosedByDefault

and this should be closedByDefault.

JatinDev543

Reviewable status: 0 of 12 files reviewed, 17 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/authorization/auth.go, line 136 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

stale

removed.

graphql/authorization/auth.go, line 145 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

so, we are not providing support for this in the old format. One way to get users to move to the new format :)

yeah, audiance is also not there.

graphql/e2e/auth/auth_test.go, line 128 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

closedByDefault bool
don't see it used here, should be removed?

removed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 44 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

type Region struct {
	Id     string         `json:"id,omitempty"`
	Name   string         `json:"name,omitempty"`
	Users  []*common.User `json:"users,omitempty"`
	Global bool           `json:"global,omitempty"`
}
type Movie struct {
	Id               string    `json:"id,omitempty"`
	Content          string    `json:"content,omitempty"`
	Hidden           bool      `json:"hidden,omitempty"`
	RegionsAvailable []*Region `json:"regionsAvailable,omitempty"`
}
type Issue struct {
	Id    string       `json:"id,omitempty"`
	Msg   string       `json:"msg,omitempty"`
	Owner *common.User `json:"owner,omitempty"`
}
type Log struct {
	Id     string `json:"id,omitempty"`
	Logs   string `json:"logs,omitempty"`
	Random string `json:"random,omitempty"`
}
type ComplexLog struct {
	Id      string `json:"id,omitempty"`
	Logs    string `json:"logs,omitempty"`
	Visible bool   `json:"visible,omitempty"`
}
type Role struct {
	Id         string         `json:"id,omitempty"`
	Permission string         `json:"permission,omitempty"`
	AssignedTo []*common.User `json:"assignedTo,omitempty"`
}
type Ticket struct {
	Id         string         `json:"id,omitempty"`
	OnColumn   *Column        `json:"onColumn,omitempty"`
	Title      string         `json:"title,omitempty"`
	AssignedTo []*common.User `json:"assignedTo,omitempty"`
}
type Column struct {
	ColID     string    `json:"colID,omitempty"`
	InProject *Project  `json:"inProject,omitempty"`
	Name      string    `json:"name,omitempty"`
	Tickets   []*Ticket `json:"tickets,omitempty"`
}
type Project struct {
	ProjID  string    `json:"projID,omitempty"`
	Name    string    `json:"name,omitempty"`
	Roles   []*Role   `json:"roles,omitempty"`
	Columns []*Column `json:"columns,omitempty"`
}
type Student struct {
	Id    string `json:"id,omitempty"`
	Email string `json:"email,omitempty"`
}
type Task struct {
	Id          string            `json:"id,omitempty"`
	Name        string            `json:"name,omitempty"`
	Occurrences []*TaskOccurrence `json:"occurrences,omitempty"`
}
type TaskOccurrence struct {
	Id   string `json:"id,omitempty"`
	Due  string `json:"due,omitempty"`
	Comp string `json:"comp,omitempty"`
}
type TestCase struct {
	user            string
	role            string
	result          string
	name            string
	filter          map[string]interface{}
	variables       map[string]interface{}
	query           string
	closedByDefault bool
}
type uidResult struct {
	Query []struct {
		UID string
	}
}
type Tasks []Task
func (tasks Tasks) add(t *testing.T) {
	getParams := &common.GraphQLParams{
		Query: `
		mutation AddTask($tasks : [AddTaskInput!]!) {
		  addTask(input: $tasks) {
			numUids
		  }
		}
		`,
		Variables: map[string]interface{}{"tasks": tasks},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func (r *Region) add(t *testing.T, user, role string) {
	getParams := &common.GraphQLParams{
		Headers: common.GetJWT(t, user, role, metaInfo),
		Query: `
		mutation addRegion($region: AddRegionInput!) {
		  addRegion(input: [$region]) {
			numUids
		  }
		}
		`,
		Variables: map[string]interface{}{"region": r},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func (r *Region) delete(t *testing.T, user, role string) {
	getParams := &common.GraphQLParams{
		Headers: common.GetJWT(t, user, role, metaInfo),
		Query: `
		mutation deleteRegion($name: String) {
		  deleteRegion(filter:{name: { eq: $name}}) {
			msg
		  }
		}
		`,
		Variables: map[string]interface{}{"name": r.Name},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func TestOptimizedNestedAuthQuery(t *testing.T) {
	query := `
	query {
	  queryMovie {
		content
		 regionsAvailable {
		  name
		  global
		}
	  }
	}
	`
	user := "user1"
	role := "ADMIN"
	getUserParams := &common.GraphQLParams{
		Headers: common.GetJWT(t, user, role, metaInfo),
		Query:   query,
	}
	gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
	beforeTouchUids := gqlResponse.Extensions["touched_uids"]
	beforeResult := gqlResponse.Data
	// Previously, Auth queries would have touched all the new `Regions`. But after the optimization
	// we should only touch necessary `Regions` which are assigned to some `Movie`. Hence, adding
	// these extra `Regions` would not increase the `touched_uids`.
	var regions []Region
	for i := 0; i < 100; i++ {
		r := Region{
			Name:   fmt.Sprintf("Test_Region_%d", i),
			Global: true,
		}
		r.add(t, user, role)
		regions = append(regions, r)
	}
	gqlResponse = getUserParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
	afterTouchUids := gqlResponse.Extensions["touched_uids"]
	require.Equal(t, beforeTouchUids, afterTouchUids)
	require.Equal(t, beforeResult, gqlResponse.Data)
	// Clean up
	for _, region := range regions {
		region.delete(t, user, role)
	}
}
func (s Student) deleteByEmail(t *testing.T) {
	getParams := &common.GraphQLParams{
		Query: `
			mutation delStudent ($filter : StudentFilter!){
			  	deleteStudent (filter: $filter) {
					numUids
			  	}
			}
		`,
		Variables: map[string]interface{}{"filter": map[string]interface{}{
			"email": map[string]interface{}{"eq": s.Email},
		}},
	}
	gqlResponse := getParams.ExecuteAsPost(t, graphqlURL)
	require.Nil(t, gqlResponse.Errors)
}
func (s Student) add(t *testing.T) {
	mutation := &common.GraphQLParams{
		Query: `
		mutation addStudent($student : AddStudentInput!) {
			addStudent(input: [$student]) {
				numUids
			}
		}`,
		Variables: map[string]interface{}{"student": s},
	}
	result := `{"addStudent":{"numUids": 1}}`
	gqlResponse := mutation.ExecuteAsPost(t, graphqlURL)
	common.RequireNoGQLErrors(t, gqlResponse)
	require.JSONEq(t, result, string(gqlResponse.Data))
}

I don't think we need to copy-paste all of this.

yeah,removed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 290 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

		{
			name: "Invalid JWT - type with auth directive",
			query: `
	          	mutation addUser($user: AddUserSecretInput!) {
		        	addUserSecret(input: [$user]) {
				      userSecret {
					    aSecret
				      }
			       }
	        	}`,
			user:   "user1",
			result: `{"addUserSecret":{"usersecret":[{"aSecret":"secret1"}]}}`,
			variables: map[string]interface{}{"user": &common.UserSecret{
				ASecret: "secret1",
				OwnedBy: "user1",
			}},
		},

Ideally, we should only be testing for Missing JWT cases, is there a reason we are testing for invalid JWT too here? That would anyways be tested in e2e/auth/auth_test.go already.

yeah,removed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 346 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	for _, tcase := range testCases {
		getUserParams := &common.GraphQLParams{
			Query:     tcase.query,
			Variables: tcase.variables,
		}
		testMissingJWT := strings.HasPrefix(tcase.name, "Missing JWT")
		testInvalidKey := strings.HasPrefix(tcase.name, "Invalid JWT")
		if testInvalidKey {
			getUserParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
			jwtVar := getUserParams.Headers.Get(metaInfo.Header)
			// Create a invalid JWT signature.
			jwtVar = jwtVar + "A"
			getUserParams.Headers.Set(metaInfo.Header, jwtVar)
		} else if !testMissingJWT {
			getUserParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
		}
		gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
		require.Equal(t, len(gqlResponse.Errors), 1)
		if testMissingJWT {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"Jwt is required when ClosedByDefault flag is true")
		} else if testInvalidKey {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"unable to parse jwt token:token signature is invalid")
		}
	}

if we are testing only for missing JWT, then we should modify this to consider only that scenario.

removed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 402 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

		{name: "Missing JWT - type with auth field",
			query: `
			query {
				queryProject {
					name
				}
			}`,
			user:   "user1",
			role:   "ADMIN",
			result: `{"queryProject":[]}`,
		},
		{name: "Missing JWT - type without auth field",
			query: `
			query {
				queryTodo {
					owner
				}
			}`,
			user:   "user1",
			role:   "ADMIN",
			result: `{"queryTodo":[]}`,
		},

I believe only these two test cases are enough, we shouldn't be needing the others in this test.

removed , other tests.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 455 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	for _, tcase := range testCases {
		queryParams := &common.GraphQLParams{
			Query: tcase.query,
		}
		testMissingJWT := strings.HasPrefix(tcase.name, "Missing JWT")
		testInvalidKey := strings.HasPrefix(tcase.name, "Invalid JWT")
		if testInvalidKey {
			queryParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
			jwtVar := queryParams.Headers.Get(metaInfo.Header)
			// Create a invalid JWT signature.
			jwtVar = jwtVar + "A"
			queryParams.Headers.Set(metaInfo.Header, jwtVar)
		} else if (tcase.user != "" || tcase.role != "") && (!testMissingJWT) {
			queryParams.Headers = common.GetJWT(t, tcase.user, tcase.role, metaInfo)
		}
		gqlResponse := queryParams.ExecuteAsPost(t, graphqlURL)
		if testMissingJWT {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"Jwt is required when ClosedByDefault flag is true")
		} else if testInvalidKey {
			require.Contains(t, gqlResponse.Errors[0].Error(),
				"unable to parse jwt token:token signature is invalid")
		}
		if diff := cmp.Diff(tcase.result, string(gqlResponse.Data)); diff != "" {
			t.Errorf("Test: %s result mismatch (-want +got):\n%s", tcase.name, diff)
		}
	}

if we are testing only for missing JWT, then we should modify this to consider only that scenario.

changed.

graphql/e2e/common/common.go, line 199 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

type Todo struct {
	Id    string `json:"id,omitempty"`
	Text  string `json:"text,omitempty"`
	Owner string `json:"owner,omitempty"`
}
do we need this here?
I don't see a need here.
This should go to auth_ClosedByDefault/auth_test.go because it is used only there.

yeah, part of the previous code. Removed.

graphql/resolve/auth_add_test.yaml, line 1052 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

- name: "Query with missing jwt token - Add with top level not RBAC false."
  gqlquery: |
    mutation addComplexLog($log: AddComplexLogInput!) {
      addComplexLog(input: [$log]) {
        complexLog {
          id
        }
      }
    }
  jwtvar:
    ROLE: "USER"
    USER: "user1"
  variables: |
    { "log":
      { "logs": "log123",
        "visible": true
      }
    }
  uids: |
    { "ComplexLog1": "0x123" }
  error:
    { "message": "mutation failed because authorization failed because Jwt is required when ClosedByDefault flag is true"}
- name: "Query with missing jwt token - Type without Auth"
  gqlquery: |
    mutation addTodo($todo: AddTodoInput!) {
      addTodo(input: [$todo]) {
        todo {
          id
          owner
          text
        }
      }
    }
  variables: |
    { "todo":
      { "owner": "Alice",
        "text":  "Hi Graphql"
      }
    }
  error:
    { "message": "mutation failed because authorization failed because Jwt is required when ClosedByDefault flag is true"}

since these two cases are to be run only when closeByDefault=true, I believe instead of adding them here, we should just create test cases in the go file. So, they are not run for cases where closeByDefault=false. OR just have a separate yaml file to run these.

created different yml.

graphql/resolve/auth_add_test.yaml, line 1073 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

- name: "Invalid JWT - Type without Auth"
  gqlquery: |
    mutation addTodo($todo: AddTodoInput!) {
      addTodo(input: [$todo]) {
        todo {
          id
          owner
          text
        }
      }
    }
  variables: |
    { "todo":
      { "owner": "Alice",
        "text":  "Hi Graphql"
      }
    }
  error:
    { "message": "mutation failed because authorization failed because unable to parse jwt token:token signature is invalid"}

I don't understand why do we need this.

removed.

graphql/resolve/auth_query_test.yaml, line 1065 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

- name: "Invalid JWT - type without auth directive"
  gqlquery: |
    query {
      queryTodo {
        id
        owner
        text
      }
    }
  dgquery: |-
    query {
      queryRTodo(func: type(Todo)) {
        owner : Todo.comment
        text :  Todo.text
        dgraph.uid : uid
      }
    }

These Invalid JWT tests shouldn't be needed, right?

yeah,removed.

graphql/resolve/auth_test.go, line 388 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

				if strings.HasPrefix(tcase.Name, "Invalid JWT") {
					authMeta.PublicKey = "invalidkey"
				}
				ctx, err = authMeta.AddClaimsToContext(ctx)
				require.NoError(t, err)
				authMeta.PublicKey = publicKeyTemp
			}
			dgQuery, err := testRewriter.Rewrite(ctx, gqlQuery)
			if ClosedByDefault {
				if strings.HasPrefix(tcase.Name, "Query with missing jwt token") {
					require.Equal(t, err.Error(), "Jwt is required when ClosedByDefault flag is true")
					require.Nil(t, dgQuery)
				} else if strings.HasPrefix(tcase.Name, "Invalid JWT") {
					require.Equal(t, err.Error(), "unable to parse jwt token:token signature is invalid")
					require.Nil(t, dgQuery)
				} else {
					require.Nil(t, err)
					require.Equal(t, tcase.DGQuery, dgraph.AsString(dgQuery))
				}
			} else {
				require.Nil(t, err)
				require.Equal(t, tcase.DGQuery, dgraph.AsString(dgQuery))
			}

we shouldn't have to do this on a case by case basis.

yeah,removed.

graphql/resolve/auth_test.go, line 732 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	if !strings.HasPrefix(tcase.Name, "Query with missing jwt token") {
		if strings.HasPrefix(tcase.Name, "Invalid JWT") {
			authMeta.PublicKey = "invalidkey"
		}
		ctx, err = authMeta.AddClaimsToContext(ctx)
		require.NoError(t, err)
		authMeta.PublicKey = publicKeyTemp
	}

we shouldn't be doing this

removed.

graphql/resolve/auth_test.go, line 767 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	if ClosedByDefault {
		if strings.HasPrefix(tcase.Name, "Query with missing jwt token") || strings.HasPrefix(tcase.Name, "Invalid JWT") {
			require.Equal(t, resolved.Err.Error(), tcase.Error.Error())
		}
	} else if tcase.Error != nil {

this shouldn't be needed

removed.

testutil/graphql.go, line 156 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

closedByDefault
do we need to add this? doesn't seem to be used anywhere. Also if it were meant to be used outside of this package then shouldn't it be PascalCase? like: ClosedByDefault?

removed from here and changed to CloseByDefault.

testutil/graphql.go, line 208 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

 ClosedByDefault
and this should be closedByDefault.

changed.

…QL-713

abhimanyusinghgaur

Reviewable status: 0 of 10 files reviewed, 17 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 41 at r2 (raw file):

Quoted 20 lines of code…

type TestCase struct {
	user            string
	role            string
	result          string
	name            string
	filter          map[string]interface{}
	variables       map[string]interface{}
	query           string
	closedByDefault bool
}
type UserSecret struct {
	Id      string `json:"id,omitempty"`
	ASecret string `json:"aSecret,omitempty"`
	OwnedBy string `json:"ownedBy,omitempty"`
}
type Todo struct {
	Id    string `json:"id,omitempty"`
	Text  string `json:"text,omitempty"`
	Owner string `json:"owner,omitempty"`
}

even these we should not duplicate here. We can just use the ones from auth/auth_test.go wherever needed in this file. For, Todo just declare it in the auth/auth_test.go because the schema we are using is also from the same package, so I think it would be better if we kept all the related things at the same place.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 73 at r2 (raw file):

user:   "user1",

not required

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 74 at r2 (raw file):

result: `{"addUserSecret":{"usersecret":[{"aSecret":"secret1"}]}}`,

result should be null?
also, should be moved after variables.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 81 at r2 (raw file):

result: `{"addTodo":{"Todo":[]}}`,

should be moved after variables

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 107 at r2 (raw file):

		require.Equal(t, len(gqlResponse.Errors), 1)
		require.Contains(t, gqlResponse.Errors[0].Error(),
			"Jwt is required when ClosedByDefault flag is true")

we should also compare the result we receive in response, to the one given in the test case.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 121 at r2 (raw file):

			user:   "user1",
			role:   "ADMIN",

not required

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 131 at r2 (raw file):

			user:   "user1",
			role:   "ADMIN",

not required

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 143 at r2 (raw file):

		gqlResponse := queryParams.ExecuteAsPost(t, graphqlURL)
		require.Contains(t, gqlResponse.Errors[0].Error(),
			"Jwt is required when ClosedByDefault flag is true")

we should also compare the result we receive in response, to the one given in the test case.

graphql/e2e/auth_ClosedByDefault/docker-compose.yml, line 1 at r2 (raw file):

version: "3.5"

I believe we would also not require a separate docker-compose. Just the separate go test file should be enough. We should just move that file to the e2e/auth package, and name that closed_by_default_test.go

graphql/resolve/auth_test.go, line 345 at r2 (raw file):

ClosedByDefault

closedByDefault

graphql/resolve/auth_test.go, line 652 at r2 (raw file):

ClosedByDefault

closedByDefault

graphql/resolve/auth_test.go, line 703 at r2 (raw file):

ClosedByDefault

closedByDefault

graphql/resolve/auth_test.go, line 726 at r2 (raw file):

if !strings.HasPrefix(tcase.Name, "Query with missing jwt token") {

It should be:

if !closedByDefault {

abhimanyusinghgaur

Reviewable status: 0 of 10 files reviewed, 16 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/e2e/auth_ClosedByDefault/docker-compose.yml, line 1 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

I believe we would also not require a separate docker-compose. Just the separate go test file should be enough. We should just move that file to the e2e/auth package, and name that closed_by_default_test.go

sorry, misunderstood this.

JatinDev543

Reviewable status: 0 of 11 files reviewed, 16 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/e2e/auth_ClosedByDefault/docker-compose.yml, line 1 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

sorry, misunderstood this.

no problem. i retained new directory for tests.

graphql/resolve/auth_test.go, line 345 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

ClosedByDefault
closedByDefault

changed.

graphql/resolve/auth_test.go, line 652 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

ClosedByDefault
closedByDefault

changed.

graphql/resolve/auth_test.go, line 703 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

ClosedByDefault
closedByDefault

removed, don't need.

graphql/resolve/auth_test.go, line 726 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

if !strings.HasPrefix(tcase.Name, "Query with missing jwt token") {

It should be:

if !closedByDefault {

no, actually there are some old tests that require this.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 41 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

type TestCase struct {
	user            string
	role            string
	result          string
	name            string
	filter          map[string]interface{}
	variables       map[string]interface{}
	query           string
	closedByDefault bool
}
type UserSecret struct {
	Id      string `json:"id,omitempty"`
	ASecret string `json:"aSecret,omitempty"`
	OwnedBy string `json:"ownedBy,omitempty"`
}
type Todo struct {
	Id    string `json:"id,omitempty"`
	Text  string `json:"text,omitempty"`
	Owner string `json:"owner,omitempty"`
}
even these we should not duplicate here. We can just use the ones from auth/auth_test.go wherever needed in this file. For, Todo just declare it in the auth/auth_test.go because the schema we are using is also from the same package, so I think it would be better if we kept all the related things at the same place.

moved Todo and usersecret to common.go and retained TestCase

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 73 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

user:   "user1",
not required

removed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 74 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

result: `{"addUserSecret":{"usersecret":[{"aSecret":"secret1"}]}}`,
result should be null?
also, should be moved after variables.

yeah,changed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 81 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

result: `{"addTodo":{"Todo":[]}}`,
should be moved after variables

changed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 107 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should also compare the result we receive in response, to the one given in the test case.

done.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 121 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

			user:   "user1",
			role:   "ADMIN",

not required

removed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 131 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

			user:   "user1",
			role:   "ADMIN",

not required

removed.

graphql/e2e/auth_ClosedByDefault/auth_test.go, line 143 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should also compare the result we receive in response, to the one given in the test case.

done.

abhimanyusinghgaur

Reviewed 3 of 11 files at r1, 5 of 10 files at r2, 2 of 3 files at r3.
Reviewable status: 10 of 11 files reviewed, all discussions resolved (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/resolve/auth_test.go, line 726 at r2 (raw file):

Previously, JatinDevDG (Jatin Dev) wrote…

no, actually there are some old tests that require this.

but for the previous diff, it shows that ctx always contained claims, so seems only the new tests require this.

…QL-713

pawanrawal

Reviewable status: 9 of 11 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/authorization/auth.go, line 316 at r4 (raw file):

	if !ok {
		if authMeta.ClosedByDefault {
			return &CustomClaims{}, fmt.Errorf("Jwt is required when ClosedByDefault flag is true")

Change error to the following, no need to mention anything about ClosedByDefault.

A valid JWT is required but was not provided.

graphql/e2e/auth/schema.graphql, line 588 at r4 (raw file):

}

type Todo{

Space before {

type Todo {

graphql/e2e/auth/schema.graphql, line 589 at r4 (raw file):

type Todo{
    id:ID

id: ID

please take care of formatting

graphql/resolve/auth_ClosedByDefault_add_test.yaml, line 1 at r4 (raw file):

- name: "Query with missing jwt token - Type with Auth"

the filename should be like below. Please don't mix cases while naming files

auth_closed_by_default_...

graphql/resolve/auth_ClosedByDefault_add_test.yaml, line 27 at r4 (raw file):

- name: "Query with missing jwt token - Type without Auth"
  gqlquery: |
    mutation addTodo($todo: AddTodoInput!) {

add some update and delete tests as well?

graphql/resolve/auth_test.go, line 387 at r4 (raw file):

			if tcase.Error != nil {
				require.Equal(t, err.Error(), tcase.Error.Error())

Also check

require.NotNil(t, err)

before comparing error values

graphql/resolve/auth_test.go, line 726 at r4 (raw file):

	ctx := context.Background()
	if !strings.HasPrefix(tcase.Name, "Query with missing jwt token") {

Don't like all this special behavior stuff where we decide the behavior of a test based on the value of a test case. Why are we doing this?

graphql/resolve/auth_test.go, line 826 at r4 (raw file):

	t.Run("Query Rewriting "+algo, func(t *testing.T) {
		queryRewriting(t, strSchema, metaInfo, true)

instead of passing this last flag, just read the file and pass the contents of the file to this function.

closedByDefault is already part of the schema so no need to pass it again. You are anyway using it to decide the name of the file, so better read the file and pass the contents of the file to this and mutationAdd function below.

testutil/graphql.go, line 211 at r4 (raw file):

	if algo == "HS256" {
		if closedByDefault {
			authInfo = `# Dgraph.Authorization {"VerificationKey":"secretkey","Header":"X-Test-Auth","Namespace":"https://xyz.io/jwt/claims","Algo":"HS256","Audience":["aud1","63do0q16n6ebjgkumu05kkeian","aud5"],"ClosedByDefault":true}`

Why copy the whole thing twice. Just use fmt.Sprintf(authInfo, closedByDefault) to substitute the appropriate value. Same below.

graphql/e2e/auth_ClosedByDefault/auth_ClosedByDefault_test.go, line 1 at r4 (raw file):

/*

rename file and folder to

auth_closed_by_default/auth_closed_by_default_test.go

graphql/e2e/auth_ClosedByDefault/auth_ClosedByDefault_test.go, line 90 at r4 (raw file):

}

func TestAuthRulesQueryWithClosedByDefaultAFlag(t *testing.T) {

...DefaultFlag

graphql/e2e/auth_ClosedByDefault/auth_ClosedByDefault_test.go, line 125 at r4 (raw file):

func TestMain(m *testing.M) {
	schemaFile := "../auth/schema.graphql"

I am seeing all this code across multiple files. Could you have this inside common and call it from different places.

The function can be called `BootstrapAuthData(closedByDefault bool)``

JatinDev543

Reviewable status: 1 of 14 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

graphql/authorization/auth.go, line 316 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Change error to the following, no need to mention anything about ClosedByDefault.
A valid JWT is required but was not provided.

changed.

graphql/e2e/auth/schema.graphql, line 588 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Space before {
type Todo {

changed

graphql/e2e/auth/schema.graphql, line 589 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

id: ID
please take care of formatting

changed.

graphql/resolve/auth_test.go, line 726 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

but for the previous diff, it shows that ctx always contained claims, so seems only the new tests require this.

ohhh, yeah changed this. I thought it's the one inside queryRewriting.

graphql/resolve/auth_test.go, line 387 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Also check
require.NotNil(t, err)
before comparing error values

done.

graphql/resolve/auth_test.go, line 726 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Don't like all this special behavior stuff where we decide the behavior of a test based on the value of a test case. Why are we doing this?

changed this. Actually we don't generate JWT for cases for which closedByDefault flag is false.

graphql/resolve/auth_test.go, line 826 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

instead of passing this last flag, just read the file and pass the contents of the file to this function.

closedByDefault is already part of the schema so no need to pass it again. You are anyway using it to decide the name of the file, so better read the file and pass the contents of the file to this and mutationAdd function below.

changed.

testutil/graphql.go, line 211 at r4 (raw file):

authInfo = `# Dgraph.Authorization {"VerificationKey":"secretkey","Header":"X-Test-Auth","Namespace":"https://xyz.io/jwt/claims","Algo":"HS256","Audience":["aud1","63do0q16n6ebjgkumu05kkeian","aud5"],"ClosedByDefault":true}`

changed.

graphql/e2e/auth_ClosedByDefault/auth_ClosedByDefault_test.go, line 1 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

rename file and folder to
auth_closed_by_default/auth_closed_by_default_test.go

changed.

graphql/e2e/auth_ClosedByDefault/auth_ClosedByDefault_test.go, line 90 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

...DefaultFlag

changed.

graphql/e2e/auth_ClosedByDefault/auth_ClosedByDefault_test.go, line 125 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I am seeing all this code across multiple files. Could you have this inside common and call it from different places.

The function can be called `BootstrapAuthData(closedByDefault bool)``

This code only used inside auth/auth_test.go and auth_Closed_by_default_test.go . Moved starting part of the code inside common.go. The other part is a little difficult to move because it involves to set metainfo ,algo in different ways.

graphql/resolve/auth_ClosedByDefault_add_test.yaml, line 1 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

the filename should be like below. Please don't mix cases while naming files
auth_closed_by_default_...

done.

graphql/resolve/auth_ClosedByDefault_add_test.yaml, line 27 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

add some update and delete tests as well?

added e2e and unit tests for update and delete.

pawanrawal

is there a doc ticket corresponding to this change? Otherwise please create one, link this issue and assign to Aaron with priority highest.

Reviewed 10 of 13 files at r5.
Reviewable status: 11 of 14 files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

graphql/resolve/auth_test.go, line 181 at r5 (raw file):

	}
	require.Equal(t, authVar, result)
	//reset auth meta, so that it won't effect other tests

// reset
Space after //

graphql/resolve/auth_test.go, line 845 at r5 (raw file):

	var b []byte
	var err error
	b, err = ioutil.ReadFile(file)

b, err can be defined inline

b, err :=

graphql/e2e/auth_ClosedByDefault/auth_ClosedByDefault_test.go, line 23 at r4 (raw file):

	"github.com/dgraph-io/dgraph/graphql/e2e/common"
	"github.com/dgraph-io/dgraph/testutil"
	"github.com/pkg/errors"

rename file to auth_closed... , C shouldn't be capital

…QL-713

This documents the ClosedByDefault setting in the Dgraph.Authorization object. This was introduced in Dgraph v20.11.0 in hypermodeinc/dgraph#6779. Fixes DOC-134 Related: https://discuss.dgraph.io/t/implemented-queries-closed-by-default/12626

…) (#40) This documents the ClosedByDefault setting in the Dgraph.Authorization object. This was introduced in Dgraph v20.11.0 in hypermodeinc/dgraph#6779. Fixes DOC-134 Related: https://discuss.dgraph.io/t/implemented-queries-closed-by-default/12626

JatinDev543 added 4 commits October 20, 2020 15:13

added code for authorization switch

8376670

changed error message

366a295

added unit tests

bfc8974

added e2e tests

408bffc

JatinDev543 requested review from manishrjain, MichaelJCompton, pawanrawal and vvbalaji-dgraph as code owners October 22, 2020 10:54

github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Oct 22, 2020

clean code

f152bad

abhimanyusinghgaur suggested changes Oct 22, 2020

View reviewed changes

JatinDev543 added 3 commits October 23, 2020 02:14

moved tests and clean code

7d9b566

cleaned tests

c78882a

clean code

95ba416

JatinDev543 commented Oct 23, 2020

View reviewed changes

JatinDev543 added 2 commits October 23, 2020 14:34

clean code a bit

2ab68fb

Merge branch 'master' of github.com:dgraph-io/dgraph into jatin/GRAPH…

9766c23

…QL-713

abhimanyusinghgaur suggested changes Oct 23, 2020

View reviewed changes

JatinDev543 changed the title ~~Feat(GraphQL): This PR adds auth switch in graphql authhorization header.~~ Feat(GraphQL): This PR adds auth switch in GraphQL authorization header. Oct 23, 2020

abhimanyusinghgaur reviewed Oct 23, 2020

View reviewed changes

removed extra part

df292af

JatinDev543 commented Oct 23, 2020

View reviewed changes

abhimanyusinghgaur approved these changes Oct 23, 2020

View reviewed changes

JatinDev543 added 4 commits October 23, 2020 18:59

Merge branch 'master' of github.com:dgraph-io/dgraph into jatin/GRAPH…

73a13ae

…QL-713

added false switch by default

1e96a6b

added closedByDefault for RS256 test case

7be00a8

fixed tests

2e53e21

pawanrawal suggested changes Oct 27, 2020

View reviewed changes

modified tests and resolved comments.

9f60182

JatinDev543 commented Oct 28, 2020

View reviewed changes

pawanrawal approved these changes Oct 28, 2020

View reviewed changes

JatinDev543 added 7 commits October 28, 2020 18:19

Merge branch 'master' of github.com:dgraph-io/dgraph into jatin/GRAPH…

1931e46

…QL-713

merge masters,removed antidependency errors and resolved comments

fa0bf5c

removed anti dependency error

941314a

Merge branch 'master' of github.com:dgraph-io/dgraph into jatin/GRAPH…

76b5331

…QL-713

Merge branch 'master' of github.com:dgraph-io/dgraph into jatin/GRAPH…

b6ea68f

…QL-713

Merge branch 'master' of github.com:dgraph-io/dgraph into jatin/GRAPH…

e367f6a

…QL-713

fixed formatting

43e5f6e

JatinDev543 merged commit 85211d2 into master Oct 29, 2020

danielmai deleted the jatin/GRAPHQL-713 branch February 4, 2021 19:00

danielmai mentioned this pull request Feb 4, 2021

docs(graphql): Document ClosedByDefault setting for authorization. dgraph-io/dgraph-docs#39

Merged

danielmai mentioned this pull request Feb 4, 2021

docs(graphql): Document ClosedByDefault setting for authorization. dgraph-io/dgraph-docs#40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat(GraphQL): This PR adds auth switch in GraphQL authorization header. #6779

Feat(GraphQL): This PR adds auth switch in GraphQL authorization header. #6779

JatinDev543 commented Oct 22, 2020 •

edited

Loading

abhimanyusinghgaur left a comment

JatinDev543 left a comment

abhimanyusinghgaur left a comment

abhimanyusinghgaur left a comment

JatinDev543 left a comment

abhimanyusinghgaur left a comment

pawanrawal left a comment

JatinDev543 left a comment

pawanrawal left a comment

Feat(GraphQL): This PR adds auth switch in GraphQL authorization header. #6779

Feat(GraphQL): This PR adds auth switch in GraphQL authorization header. #6779

Conversation

JatinDev543 commented Oct 22, 2020 • edited Loading

abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

JatinDev543 left a comment

Choose a reason for hiding this comment

abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

JatinDev543 left a comment

Choose a reason for hiding this comment

abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

JatinDev543 left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

JatinDev543 commented Oct 22, 2020 •

edited

Loading