[uss_qualifier] Add GetAccessTokens scenario - #1634
Conversation
the-glu
left a comment
the-glu
left a comment
There was a problem hiding this comment.
The scenario only allows for positive results (e.g., "With these settings, I get this token").
Shouldn't we also test the negative case? For example: "With this unknown scope, I don't get a token because it shouldn't be allowed."
This may be out of scope for these specific tests, but it could be valuable for validating authorization servers.
| JQIDAQAB | ||
| -----END PUBLIC KEY----- | ||
| expires_in_more_than: 50m | ||
| expires_in_less_than: 1h10m |
There was a problem hiding this comment.
It's a bit special, but have you considered moving it to has_claim_values to have all claims at the same place?
There was a problem hiding this comment.
I'm not sure I follow -- there are expectations that could be folded into has_xyz_values (e.g., alg into has_header_values), but I don't think this is one unless we introduce ASTExpressions. The value in exp is an absolute time, so to achieve the effect of these expires_in_xyz fields, we would need a greater_than_expr expression that is something like "($request_time + parse_time_delta('50m')).timestamp()" and specifically populate the request_time variable and parse_time_delta function when evaluating that expression. It seems clearer and more straightforward to do it this way.
There was a problem hiding this comment.
The idea was to move in into the 'has_claim_values' object, but with a similar name and do the request_time + X computation there as well, without any other complexity, just the same two posibilities.
This would also have the advantage of testing 'nbf' and 'iat' (and possibility others claims with date) with similar checks.
| jwt.decode( | ||
| token, | ||
| key=public_pem, | ||
| algorithms=[header.get("alg", "RS256")], |
There was a problem hiding this comment.
nit/minor: Getting the algorithm from the header itself is not correct, it should come from the expected algorithm (based on the key or expectations.alg), to avoid some attacks.
There was a problem hiding this comment.
I agree that when validating an access token for the purpose of granting access, the algorithm should definitely be supplied rather than taken from the header so a fake token couldn't be generated with a weak algorithm. In this case though, I would expect a test designer to specify an expected alg to achieve the same effective result. I'll add additional warning documentation regarding this expectation.
BenjaminPelletier
left a comment
BenjaminPelletier
left a comment
There was a problem hiding this comment.
Shouldn't we also test the negative case? For example: "With this unknown scope, I don't get a token because it shouldn't be allowed."
This could be added as an additional feature but I think the content of this PR contains a self-contained feature (this change isn't needed to make the PR complete), this PR is already pretty big, and #1629 doesn't ask for this capability. So, I think it's probably out of scope for this PR at least.
| jwt.decode( | ||
| token, | ||
| key=public_pem, | ||
| algorithms=[header.get("alg", "RS256")], |
There was a problem hiding this comment.
I agree that when validating an access token for the purpose of granting access, the algorithm should definitely be supplied rather than taken from the header so a fake token couldn't be generated with a weak algorithm. In this case though, I would expect a test designer to specify an expected alg to achieve the same effective result. I'll add additional warning documentation regarding this expectation.
| JQIDAQAB | ||
| -----END PUBLIC KEY----- | ||
| expires_in_more_than: 50m | ||
| expires_in_less_than: 1h10m |
There was a problem hiding this comment.
I'm not sure I follow -- there are expectations that could be folded into has_xyz_values (e.g., alg into has_header_values), but I don't think this is one unless we introduce ASTExpressions. The value in exp is an absolute time, so to achieve the effect of these expires_in_xyz fields, we would need a greater_than_expr expression that is something like "($request_time + parse_time_delta('50m')).timestamp()" and specifically populate the request_time variable and parse_time_delta function when evaluating that expression. It seems clearer and more straightforward to do it this way.
2 participants
This PR defines, documents, and implements a new GetAccessTokens scenario to address #1629 using new InterUSS requirements that a test designer can use to express/capture authorization-related requirements of their interoperability ecosystem. A new dev test configuration that exercises this scenario is added and invoked from the CI.