Conversation
338c400 to
f10a986
Compare
|
To already give a quick answer the 'other stuff':
|
termontwouter
left a comment
There was a problem hiding this comment.
Looks good qua implementation for the aggregator spec. Two questions, more related to unrelated changes:
-
What went wrong with the contractnegotiator? Did it never work, or when did it start to fail?
-
The TicketStrategy used to contain those two methods because they are two questions we want to ask of the evaluator... How does the process now calculate which requirements are missing?
It still works, as in, responses will be returned. But what it returns is a mostly hardcoded contract: It then tries to store this contract in a hardcoded container which does not exist: user-managed-access/packages/uma/src/dialog/ContractNegotiator.ts Lines 169 to 174 in b24d853 So I didn't really see a point in keeping it in there in the current state, as it doesn't actually add anything. Parts of this code can be re-inserted when contracts are actually supported.
While that is the idea behind the function that was removed, in practice this never happened since the ODRL authorizer did not support that call: it would throw an error when calling that function. Since that is the main authorizer the server is running on, this means it was never possible to find the missing requirements. I think it makes more sense to come back to this once we can actually answer that question. Potentially this can already partially be done by seeing in the ODRL report which parts of a policy were fulfilled and which were not. Although then we still need to see how to return that in a need_info error. I did actually improve the need_info error a bit in this PR. The ODRL authorizer returns the permissions that were granted, and if that is only a subset of what was requested, the missing permissions will be in the need_info error in the |
https://spec.knows.idlab.ugent.be/aggregator-protocol/latest/
Implements everything that is needed to support the above specification. A full run of the process can be seen in the
Aggregation.test.tsintegration test.Notes
derivation-creationpermission to an aggregator before it can request aderivation_resource_id, and similarly forderivation-read.AggregatorNegotiatorwhich adds thederivation_resource_idto relevant responses, and aAggregatorStrategywhich handles thederivation-readpermissions for derived resources.derivation_resource_idis generated by encoding the ID string. This way no additional mapping needs to be stored on the server. It can then be decoded by the server when needed.claim_tokensfield when requesting a token now also supports an array of token/format entries.ContractNegotiatorfrom the configuration as it currently returns a mostly hardcoded contract and performs requests that always fail.derived_fromfield is supported when registering a resource.derivation_resource_id->issuerlinks forderived_fromentries in resource registrations. This way we can check if access tokens for these identifiers are issued by the correct issuer..well-known/openid-configuration. I only needed thejwksfield to be accessible but it was easier to just link everything there. Reason being that when validating the access tokens used during the aggregator interactions, this is where the OIDC library will look to find the key to validate these tokens.credentialsfunction is removed from the interface as the main authorizer did not support it anyway, and it introduced a lot of similar-ish code.ACCESSclaim which has as value an array of id/scopes entries, indicating which permissions were (already) granted to the client from other sources.Other stuff that came up