KantaraInitiative / wg-uma

This is the repository of all specifications related to the User Managed Access Group
http://kantarainitiative.org/confluence/display/uma/
Other
27 stars 21 forks source link

No error defined for policy evaluation failed #340

Closed mrpotes closed 6 years ago

mrpotes commented 6 years ago

I'm not sure whether this is a issue in UMA or OAuth2, but it is not clear what error the AS should return to the Client if it had access to all the claims it needed (i.e. no need_info), but no policy matched and a token cannot be returned. invalid_grant seems almost appropriate, but that says to return 400 status code, which suggests to the Client they did something wrong in the request, but a repeated request would not fix the issue.

Perhaps the solution is to adjust the invalid_grant description to state that the AS MAY respond with the HTTP 400 (Bad Request) status code if the Client could attempt a retry and expect a different outcome. however, I'm not really sure that invalid_grant describes the error properly - a new access_denied error code would probably be much more useful to the Client, as they can give a useful UX to the RqP.

xmlgrrl commented 6 years ago

That should be invalid_grant: From Grant Sec 3.3.6, relying in part on 6749, "If the provided ticket was not found at the authorization server, or the provided ticket has expired, or the client is not authorized to have these permissions added, or any other original reasons to use this error response are found as defined in [RFC6749], the authorization server responds with the HTTP 400 (Bad Request) status code." I think the best place to look for the 6749 reference is Sec 5.2, though it's not a beacon of clarity.

mrpotes commented 6 years ago

Yes, in the case of 6749, a status of 400 is appropriate because the client did something wrong (or correctable) in each case they document. In the case I'm describing there is nothing the client has done wrong, nor can correct. c.f. error code 400 in RFC7239: https://tools.ietf.org/html/rfc7231#page-58

On 27 July 2017 at 14:59, Eve Maler notifications@github.com wrote:

That should be invalid_grant: From Grant Sec 3.3.6 https://docs.kantarainitiative.org/uma/wg/oauth-uma-grant-2.0-05.html#authorization-failure, relying in part on 6749, "If the provided ticket was not found at the authorization server, or the provided ticket has expired, or the client is not authorized to have these permissions added, or any other original reasons to use this error response are found as defined in [RFC6749], the authorization server responds with the HTTP 400 (Bad Request) status code." I think the best place to look for the 6749 reference is Sec 5.2 https://tools.ietf.org/html/rfc6749#section-5.2, though it's not a beacon of clarity.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KantaraInitiative/wg-uma/issues/340#issuecomment-318370348, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6kNtpsXBp7sAPoEXyB_XuulgKvHKatks5sSJepgaJpZM4OlGUB .

jricher commented 6 years ago

I disagree: the client has done something wrong, they gave a claim token format that the server didn't support. The client can do something about it: present a different claim token.

400 with invalid_grant is correct.

mrpotes commented 6 years ago

Justin, have you mistaken this issue with one of the others I raised? In this situation the claim token & format was fine.

On 27 July 2017 at 16:22, Justin Richer notifications@github.com wrote:

I disagree: the client has done something wrong, they gave a claim token format that the server didn't support. The client can do something about it: present a different claim token.

400 with invalid_grant is correct.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KantaraInitiative/wg-uma/issues/340#issuecomment-318395060, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6kNt1RH_mLEf9QwgS6rk6nymO3zSREks5sSKsegaJpZM4OlGUB .

jricher commented 6 years ago

Ah, I misread. In this case the answer would be need_info, because no policies matched the claims that the client gave. That's what need_info means. There's still a chance, as far as the AS knows, that the client could fulfill a policy with different claims.

Your description of the situation doesn't make sense to me: "it had access to all the claims it needed ... but no policy matched". If no policy matched, then all the claims needed weren't there.

mrpotes commented 6 years ago

I think need_info makes some sense, the ticket it then returns would probably have to result in the user being shown a "sorry, you're denied access".

For scenario, consider a single policy that is based on age, but no date of birth claim is available - the first need_info sends the RqP into a claim-collecting loop to acquire date of birth, then on returning to the token endpoint and evaluation of the policy again the age results in a deny result from the policy. Once you've got a deny result, what can you do? Assuming that deny overrules any accept, the AS needs to tell the user that they now have no chance to get in.

On 27 July 2017 at 20:20, Justin Richer notifications@github.com wrote:

Ah, I misread. In this case the answer would be need_info, because no policies matched the claims that the client gave. That's what need_info means. There's still a chance, as far as the AS knows, that the client could fulfill a policy with different claims.

Your description of the situation doesn't make sense to me: "it had access to all the claims it needed ... but no policy matched". If no policy matched, then all the claims needed weren't there.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/KantaraInitiative/wg-uma/issues/340#issuecomment-318460905, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6kNnoIZ2R4kkJMrHnC_nvxXnnL4Gn8ks5sSOMHgaJpZM4OlGUB .

xguttner commented 6 years ago

I understand the what-the-error-should-be decisioning in the following way:

As long as the policy evaluation leads to deny based on the fact at least one policy applicable on given permission didn't match (the result was indeterminate based on provided claims), the error should be need_info.

The date-of-birth scenario IMHO should be request_submitted as the AS knows it has all the necessary claims and there is no option to fulfill the policies. In UMA 1, there would be another option for this - not_authorized, but I think there was already a long discussion about this in #298 .

mrpotes commented 6 years ago

Thanks for the reference to #298 - I guess I'm saying that I don't agree with that outcome. In my example I think it is clear that neither need_info (there is no further info needed, a deny has been arrived at) nor request_submitted (I'm an adult-only website and you're only 16, there's nowhere for me to send a request for a policy to be created) are appropriate.

In this scenario, at what point does the Client say to the RqP "no, you don't have access to this"? At the moment we seem to be saying that this is impossible, which seems to me to be terrible UX.

The only "workaround" I can see is to return:

{
  "error": "need_info",
  "required_claims": []
}

i.e. "I need more info to be able to grant access, but there are no claims you can give me that can make that happen". This seems to be adding undue complexity to the Client to have to derive the meaning not_authorized from this response.

mrpotes commented 6 years ago

c.f. the access_denied error code from the authorization endpoint in 6749: https://tools.ietf.org/html/rfc6749#section-4.1.2.1

ciseng commented 6 years ago

When policy evaluation returns a deny (and there is no claims missing/no indeterminate result etc.); two things may be possible: () RO may change the policy () RO may not change the policy

AS does not know which way RO will go. (Here we assume, we know the reason for deny is age and that it is not changeable; but AS would not know that). In this case, returning 403 request submitted is appropriate, IMO.

The issue I raised earlier for this in #298 was: why don’t we end it there, and not hand out a ticket. To which, Justin gave a very good example of “Ask me every time RO”.

However, I do understand the concern that this may result in indefinite polling by the client of the AS, for a policy that won’t change. AS will need to stop this somehow.

joebandenburg commented 6 years ago

In this case, returning 403 request submitted is appropriate, IMO.

Does the spec require that an AS MUST submit a request to the RO? If not, then presumably if an AS does not support submitting a request to the RO it should not respond with request_submitted.

xmlgrrl commented 6 years ago

I remember looking at access_denied in the past (don't know if we discussed it in the group), but it's hard to reuse it literally vs just being inspired by it because it's an authorization endpoint error response, and that's a step we don't have in UMA.

It's true UMA1 had not_authorized, and it seems we may possibly be hankering for that semantic once again. If it means something sufficiently different from request_submitted (which effectively means "Policy, as of this moment, requires that I check this out with the RO side") and need_info (which effectively means "Policy, as of this moment, requires that I check this out with the client/RqP side"), then we could have a good rationale for adding it back. Say, "Policy is definitive, as of this moment, that you don't qualify". If it's valuable to align with the OAuth error name, we could even call it access_denied, but we'd probably have to explain the genesis.

(Joe, the AS could return request_submitted and be lying. :-) Or be unable to reach the RO or whatever. UX is outside the scope of UMA.)

xmlgrrl commented 6 years ago

Discussed and decided in UMA telecon 2017-08-07, but reopened after further consideration and decided in a different direction in UMA telecon 2017-08-17. Summary of responses from the token endpoint that relate to authorization assessment, taken from the minutes (but see them for full discussion):