-
Notifications
You must be signed in to change notification settings - Fork 24
Updating the context response object + pagination: adding support for… #341
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @baboulebou ! Ideally this would be split across two PRs since there are two distinct issues addressed here, that each have different considerations.
I will review in more detail, but my initial impressions are that the context clarification could perhaps better fit in a profile? Certainly things like “obligations” are fairly PDP-specific, and I could see that in an ABAC profile.
I think that at the syntax level (which is what AuthZEN 1.0 focuses on) we should have an extensible response context mechanism that can be used for many things (reason codes, obligations, step up, etc) without having to specify all of them in the core spec.
As far as pagination, would having a page_token and an offset be enough to satisfy both use-cases (with no need to specify the cursor type?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'm afraid you'll have to live with 1 PR for both for @ogazitt , I'm already stretched for time, but hopefully we can still gather comments for the pagination part. About that, Cursor and Token pagination are the same thing, are they not? I could rename "token" for "cursor", but then we've already published drafts with "token".
For obligations, figured we could add it here, we (Indykite) actually use a similar construct already to signal Step-up...an obligation. We could adopt a profile strategy but really the proposal here covers all the comments/issues I've seen raised here. Having a bit more structure in context should help interoperability...
Anyway, hope you guys can discuss this this week, being in Europe I will miss the weekly this week.
api/authorization-api-1_0.md
Outdated
where: | ||
|
||
`reason-code`: | ||
: REQUIRED. A String value representing a code that uniquely identifies the error. Implementations that do not use any error-code mechanism SHOULD default to valid HTTP Error codes (see {{RFC7231}} ). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the reason-code relate to the id
in the reason object? It seems like the id
is some arbitrary number, whereas the reason-code
is meant to be an HTTP error code. Is there a reason why we need both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually mentioned this in the description of the id
field:
"In case the PDP returns several reasons, the
id
MUST uniquely identify each one of them."
The "reason-code" specifies a code for a given reason. Since we're specifying a reason-admin and a reason-user, those may even have different codes, that could be implementation specific. For example, reason-admin could be a 403, but reason-user could be "something else. Just thought it could provide more flexibility.
An alternative would be to get rid of id
, move reason-code
to the top - the same code applies to all messages- and just have 2 simple (non-complex) properties: reason-admin
and reason-code
with simple string values. Would that be better/sufficient?
|
||
### Sample Response with additional context (non-normative) | ||
#### Environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we defined "context" in the request to provide (among other things) environmental attributes. Why are we calling these out specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if indeed "context" meant environmental considerations (you know, the "context" :) ), it seems that people have been willing to cram all sort of things in "context", including information that one wouldn't necessarily consider "context". So calling out an "environment" field here would make things more explicit and maybe easier for interoperability ?
"reason": { | ||
"id": "0", | ||
"reason_admin": { | ||
"insufficient_user_authentication": "Request failed policy C076E82F" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought these were supposed to be HTTP codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I stated in the "reason" section higher up, that the reason-code
is:
": REQUIRED. A String value representing a code that uniquely identifies the error. Implementations that do not use any error-code mechanism SHOULD default to valid HTTP Error codes (see {{RFC7231}} )."
Rewording to (adding "custom"):
": REQUIRED. A String value representing a code that uniquely identifies the error. Implementations that do not use any custom error-code mechanism SHOULD default to valid HTTP Error codes (see {{RFC7231}} )."
Would that clarify things? I.e., suggesting that we use HTTP codes by default, but could also use custom ones. Is that too permissive?
} | ||
} | ||
], | ||
"metadata": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should just be part of "properties"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes maybe. But again here separating it may make things a bit more interoperable?
Offset: | ||
: Pagination is performed by passing two parameters along with each Search request: `offset`, which points to the beginning record for the current page, and `limit`, which determines the number of records to return starting from the `offset` record (i.e., the page size). In this case, the PEP requests a specific set of records. | ||
|
||
## Pagination semantics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pagination is PEP/storage implementation-specific; therefore, this will not help the clarity or simplicity of the API.
Why should the consumer of the AuthZEN API need to understand or specify a pagination semantic?
Doing so goes against the objective of the API specification to provide an authorization API whicPagination is specific to the implementation of PEP/storage; therefore, it does not enhance the clarity or simplicity of the API.
Why should users of the AuthZEN API need to understand or define pagination semantics?
Requiring this knowledge contradicts the goal of the API specification, which is to provide an authorization API that is independent and agnostic of any specific implementation.
We use a page size that can only be requested, as you do not want to make this change while fetching. We use a base64-encoded token to handle special characters, case-sensitivity, and additional complex information, allowing for continuation while remaining agnostic to the storage system used underneath.
If PEP / storage-specific aspects are leaking into the API, we are missing the objective of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authzen defines 3 search apis, each of which may return a numerous results. The Resource Search API for example, should return all the resources of a certain type that the user has access to. There may be thousands of them, therefore the Search API must support some form of pagination, and the PEP should paginate results, no way around it. The spec defines the search APIs, I don't see how leaving the pagination out of it would help here? I can't even imagine how a PEP could paginate if the API itself doesn't support it... the PEP would have to use another search API that supports pagination...
This PR adds an offset
-limit
semantic to the originally proposed token
pagination (which is still there, and which seems in-line with what you describe @gertd ). The offset
/limit
pagination method is popular for database systems too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point remains that the spec does not call out that the next token:
- Regardless of its type, it is only valid for the continuation of the request response that returned the next token
- As such, the token shape and content are NOT part of the API specification and therefore cross-compatibility between implementations, as it is a runtime implementation detail.
- The only thing that is part of the API specification is that a next token is a non-null string.
api/authorization-api-1_0.md
Outdated
The Reason object being a JSON object, implementations MUST follow the following structure; the `reason` context contains the following elements: | ||
|
||
`id`: | ||
: REQUIRED. A string value that specifies the reason within the scope of a particular response. In case the PDP returns several reasons, the `id` MUST uniquely identify each one of them. The `id` MAY also match specific reasons held by the PDP, part of a reasons list or dictionnary for example, and used for audit correlations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I read this, the "id" returned is a PEP-specific "id"; how is this in line with the objective of the specification of being implementation agnostic? It would be better if the specification defines the IDs similar to gRPC status codes and HTTP status codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems to be a duplicate of @ogazitt 's comment (see further up). Sounds like we need to discuss the Reasons some more.
I'm proposing this simplification:
- no
id
, just areason-code
. - the
reason-code
could be an HTTP error code. Now, because I assume that only 401, 403 or 404 would make sense here, these may not be sufficient to express implementation-specific error conditions (e.g., "insufficient funds in bank account"), therefore: - the
reason-code
could also be an implementation-specific custom code (e.g., "insufficient_funds
").
The reason
object could thus look like this:
[
{
"reason_admin": {
"403": "Request failed policy C076E82F"
},
"reason_user": {
"insufficient_funds": "Insufficient funds in bank account."
}
}
]
Is this acceptable to everyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of a reason-code
instead of an id
, as this could also remove any localization aspects of the string representations of reasons and make it easier inside a code base to reason about code programmatically, rather than interpreting a string representation.
Modifying and cleaning up context response object… offset pagination.