GraphQL authorization using standard claims

Issues GraphQL
, ,
1

I’m setting up DGraph utilising the GraphQL API and have ran into an issue around DGraph requiring custom claims in order for it’s authorization features to work properly.

I am not in control of the JWT issuer, and I can’t put custom claims in my JWT. All I need to make authorisation decisions is access to the sub claim since we store this value in the graph.

Is there any specific reason why you mandate that the Namespace field within the Dgraph.Authorization comment block is mandatory?

I have two proposals, both of which make Namespace optional:

  1. If Namespace is not set, allow the jwt.StandardClaims be accessible by customClaims.AuthVariables
  2. Add a new field to AuthMeta, say, AllowStandardClaims: bool, which makes the jwt.StandardClaims be accessible by customClaims.AuthVariables in addition to the variables set through the Namespace.

I say, “make Namespace optional”, in both proposals since it’s not necessary possible for all users to configure their JWTs this way. The two options are more about allowing implicit behaviour (option 1), or explicit (option 2).

I’ve looked through the code and would be willing to create a Pull Request, it’s causing my company a huge issue (no fault of yourselves) with a looming deadline, that we’re likely going to fork the repository in the meantime.

Thanks!

2

I vaguely remember a conversation about this before and I was thinking that if a name space wasn’t provided that it would use the root to get the variables for claims… I am trying to find that post.

3

That would be awesome, but would require a PR to change validation:

// Validate required fields.
func (a *AuthMeta) validate() error {
	var fields string
	if a.VerificationKey == "" {
		fields = " `Verification key`"
	}

	if a.Header == "" {
		fields += " `Header`"
	}

	if a.Namespace == "" {
		fields += " `Namespace`"
	}

	if a.Algo == "" {
		fields += " `Algo`"
	}

	if len(fields) > 0 {
		return fmt.Errorf("required field missing in Dgraph.Authorization:%s", fields)
	}
	return nil
}
4

Ah, found it. It was:

Basically instead of using namespace as an object it used it as a JSON string and parses it. Does that help at all?

If you are at liberty to say, Who is the JWT issuer? That might make a big impact on effecting use cases and get a better solution if needed.

I can see use cases for using the Registered Claim Names though too.

  • sub: The “sub” (subject) claim identifies the principal that is the subject of the JWT. The claims in a JWT are normally statements about the subject. The subject value MUST either be scoped to be locally unique in the context of the issuer or be globally unique. The processing of this claim is generally application specific. The “sub” value is a case-sensitive string containing a StringOrURI value. Use of this claim is OPTIONAL.
  • jti: The “jti” (JWT ID) claim provides a unique identifier for the JWT. The identifier value MUST be assigned in a manner that ensures that there is a negligible probability that the same value will be accidentally assigned to a different data object; if the application uses multiple issuers, collisions MUST be prevented among values produced by different issuers as well. The “jti” claim can be used to prevent the JWT from being replayed. The “jti” value is a case-sensitive string. Use of this claim is OPTIONAL.

With the jti one could write a rule that checks against revoked token IDs stored in the database. That could be very useful to answer this other topic as well:

5

I’m afraid the issuer is just a client of ours that we need to integrate with. Not sure what backend they actually use.

The issue you’ve reference doesn’t actually support what I’m looking for, as you say, that’s for using the namespace to a string representing a JSON object, i.e.

"https://example.com/claims": "{\"nested\": \"withinastring\"}"

I don’t have any control over any of the claims, whether they be nested JSON objects or strings representing JSON objects.

Just to note, you couldn’t change the default behaviour to add each claim from StandardClaims into AuthVariables since it would be a breaking change if someone had a property "sub" within their custom claim.

If this is something you’d accept a PR on, I will have one in a few hours (pretty desperate to make this change :sweat_smile:)

6

Not my wheel house, just trying to lend a helping hand. That is a question for @core-devs, I know they really appreciate PRs!

1 Like
7

@core-devs would anyone be able to comment on this? Looking to make a change regardless, but would rather be an approach approved by a maintainer to increase the likelihood of this being merged.

8

Hey @dan-j

Apologies that we couldn’t get back on this issue in time.

No one has come up with such a use case earlier and to prevent collision we made namespace mandatory. For your use case, we can allow an option to read from standard claims and if there is a collision between standard and custom claims then custom claims take preference. I am happy with option 1, i.e. this happening implicitly. So we read the standard claims and then if a namespace is provided and there is a collision between a standard and custom claim, then the custom claim is what we use.

2 Likes
9

Hi @pawan

Any idea of a timeline on this?

Someone kindly pointed me to this post as I have a similar use case and posted earlier about the same.

10

Hey @haPartnerships, this feature is taken by the team into our roadmap and it should be the part of 21.03 release.

1 Like
12

Is 21.03 March 21 or is that just coincidental?

Is there an anticipated date for that release?

Also does this cover slash graph?

13

Hey @minhaj, similar question to @haPartnerships, is there an estimated release date for 21.03? I’ve searched for a branch for this issue in github and can’t find anything so I’m presuming this hasn’t been started yet?

Apologies I haven’t had the opportunity to contribute this PR myself yet, but the issue is moving higher up in priority on our own backlog so may find some time in the next couple of weeks, just let me know if it’s going to conflict with your plans.

14

21.03 would be out in March though @minhaj will find some time to work on this next week and it should land in master by the end of next week.

2 Likes
15

Hey @dan-j, this has been merged into master. See this PR.

1 Like
16

@Thammada :eyes: ^

So this I believe is part of 21.03