Skip to content
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

Agent-Specific Discovery #291

Closed
wants to merge 12 commits into from
Closed

Agent-Specific Discovery #291

wants to merge 12 commits into from

Conversation

woutermont
Copy link
Contributor

No description provided.

@woutermont woutermont self-assigned this Dec 6, 2022
@woutermont
Copy link
Contributor Author

@tomhgmns

Signed-off-by: Wouter Termont <woutermont@gmail.com>
@justinwb
Copy link
Member

@woutermont do you plan to adjust the relevant places in the interop spec as part of this proposal?

@woutermont
Copy link
Contributor Author

@justinwb yes, working on it now; will hopefully finish tomorrow!

Signed-off-by: Wouter Termont <woutermont@gmail.com>
Signed-off-by: Wouter Termont <woutermont@gmail.com>
Signed-off-by: Wouter Termont <woutermont@gmail.com>
Signed-off-by: Wouter Termont <woutermont@gmail.com>
@woutermont
Copy link
Contributor Author

Okay, so I've updated the proposal, taking more terminology from the main document, and then subtituted references to the proposal for some portions of the relevant sections in the main document. I tried to be conservative, so there is still quite a lot of text repeating what is already stated in the proposal.

In trying to align the two as good as possible, I wondered why we want to distinguish Social Agents from Applications at this level. Does the Authorization Agent need to know what type it is, given that they really contain the same info. Especially the double Registry->Registration predicate (hasApplicationRegistration/hasSocialAgentRegistration) is hard to incorporate. I feel that it only adds redundant data, since we get the same info from the resource it points to: the following paths hold the same information.

:registry interop:hasApplicationRegistration :registration .
:registry interop:hasRegistration / rdf:type interop:ApplicationRegistration .
:registry interop:hasRegistration / inter:registeredAgent / rdf:type interop:Application .

On another note: the resource hierachy included at the end of the section on Agent Registrations seems to give the wrong hint that it always has to be a filesystem-like structure. I think just including Turtle examples, or graphs like the ones in the Primer, are clearer.

Signed-off-by: Wouter Termont <woutermont@gmail.com>
Copy link
Member

@elf-pavlik elf-pavlik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about specifying registering agents (§5), if we want to do it it might be served better as a follow up step.

proposals/specification/agent-management/terms.bs Outdated Show resolved Hide resolved
proposals/specification/agent-management/amd-response.bs Outdated Show resolved Hide resolved
omitted, in which case the [=Identity Document=] [SHOULD] contain an [=RDF
Triple=] linking the [=Entity's=] [=Identifier=] to the URL of its [=Agent
Manager=] with the predicate `interop:hasAgentManager`. The predicate [MAY] also
be substituted by `rdfs:seeAlso` or `pim:preferencesFile`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for MAY on very generic rdfs:seeAlso? It could lead to a wikipedia page or anything that creator of the description finds relevant in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was for compatibility with legacy clients / discovery paths.

ACTION (@woutermont):

  • snippets of what legacy vs modern clients would have to implement
  • look into different profiles
  • put question of compatibility on next weeks agenda

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACTION: remove rdfs:seeAlso

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the removal. Feels like very quickly an innocuous piece of code code be misconstrued as the trusted agent manager.

proposals/specification/agent-management/amd-result.bs Outdated Show resolved Hide resolved
Comment on lines +7 to +10
[=Agent Manager's=] URL in which the value of the [:Authorization:] header is
set to a [[DPOP]]-bound [[OIDC]] [=Identity Token=] containing an [=Identifier=]
of the [=Agent=] as value of the `azp` [=Claim=] or as *sole* value of the
`aud` [=Claim=].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be that specific we should probably reference Solid OIDC.
The ID Token is only pushed as a claim and access token should be used in Authorization header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! This should be an Access Token with the client_id set to the URI of the Agent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid OIDC doesn't put any more specific requirement on the format of Access Tokens, they are left up to RS & AS.
I think we should use some abstract language and have non-normative reference to Solid-OIDC as a concrete example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csarven we think this should be discussed on Wednesday

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACTION: remove token details, talk about "authorized request" and refer to (yet to be created) solid ecosystem options


#### Error Responses #### {#sec-ard-response-error}

- If no [:Authorization:] header is present, the [=Agent Manager=] [MUST] respond with status code [S401], including a [:WWW-Authenticate:] header with the [:DPoP:] scheme. This response [MUST NOT] include an [:error:] parameter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents Agent Manager using it as it's Identity Document which would include some public information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. What would the Agent Manager use as who's Identity Document?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In sai-impl-service the AuthorizationAgent needs to get data from the solid storage. It has privileged access set to the end user (agent) and the authrization agent (client). In that case the Authorization Agent acts as Solid OIDC client and serves its identity document from the same IRI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but what is preventing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACTION: restore possibility of 200 response for public agents

@woutermont
Copy link
Contributor Author

woutermont commented Jan 10, 2023

Ass requested by @elf-pavlik in yesterday's meeting, here's a quickly drafted snippet of how a client would discover its registration, with support for the "legacy" use of pim:preferencesFile, reading links out of the body rather than the headers, and supporting all reponse codes (303, 204 and 200). I've commented out (with //) those parts that would be superfluous when we would not provide this additional support (only read a single relation, only out of headers, and with only 204 responses) ... it's not much difference i.m.o.

The only thing I left out is the rdfs:seeOther link, because I must agree with @elf-pavlik that this would put too much burden on the client. So we only check for one or two predicates/links of which we specify that they should be unique.

/* RDF Parser */
// import { Parser } from 'n3';
// const parser = new Parser();

/* Shorthands */
const hasAM = 'http://www.w3.org/ns/solid/interop#hasAgentManager';
const regAgent = 'http://www.w3.org/ns/solid/interop#registeredAgent';
// const prefFile = 'https://www.w3.org/ns/pim/space#preferencesFile';

/* Helper functions */
// const const rdfLinks = (rdf) => Object.fromEntries(parser.parse(rdf)
//   .map((quad) => [quad.predicate.value, { target: quad.object.value, context: quad.subject.value }]))
const headerLinks = (response) => Object.fromEntries(
  response.headers?.get('link')?.split(',')?.map((link) => link.trim().split('; ')).map((params) => [
    params.find((param) => param.startsWith('rel=')).replaceAll(/(^rel="|"$)/g,''), ({
      target: params[0].replaceAll(/(^<|>$)/g,''),
      context: params.find((param) => param.startsWith('anchor='))?.replaceAll(/(^anchor="|"$)/g,'') ?? response.url
    })
  ])
)

const getRegistration = async (webid) {

  /* Get links from profile header and body */
  const profile = await fetch(webid);
  const hlinks = headerLinks(profile)
  let am = hlinks[hasAM];
  // let pf = hlinks[prefFile];
  // if (!am && !pf) {
  //   const rlinks = rdfLinks(await profile.text());
  //   am = rlinks[hasAM];
  //   pf = rlinks[prefFile];
  // }

  /* Abort if no indication of agent manager  */
  if (!am && !pf) return undefined; 

  /* Request agent registration */
  const token = getToken(profile);
  const target = am?.target ?? pm?.target
  const dpop = getDPoP('GET', target);
  const registration = await fetch(target, {
    redirect: manual,
    headers: {
      Authorization: `DPoP ${token}`,
      DPoP: dpop,
    },
  });

  /* Optimal case, we're done */
  // if (registration.status === 200) return parser.parse(await registration.text());

  /* Otherwise, follow registration link, then we're done */
  if (registration.status === 204 || registration.status === 303) { 
    const reg = headerLinks(registration)[regAgent].context;
    // const loc = registration.headers.get('location');
    const dpop = getDPoP('GET', reg ?? loc);
    const registration = await fetch(reg ?? loc, {
      headers: {
        Authorization: `DPoP ${token}`,
        DPoP: dpop,
      },
    });
    return parser.parse(await registration.text());
  }

  /* All other cases indicate some auth error
   * which SAI-aware clients could have foreseen. */
  throw new Error('Auth error');

}

PS: I wrote this code directly into this comment, so please treat it as pseudocode; no idea if it would work without some finetuning.

Co-authored-by: elf Pavlik <elf-pavlik@hackers4peace.net>
Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinigs I fixed --

  • i.e. and e.g. should be avoided per i18n
  • Various language and punctuation errors

Things I haven't fixed --

This document does not specify the requirements an [=Agent=] has to comply with
to get an [=Agent Registration=] in the [=Agent Registry=] of an [=Entity=], nor
does it supply an exhaustive list of methods by which this **registration** is
achieved (e.g. automatically via an API, interactively via a user interface).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
achieved (e.g. automatically via an API, interactively via a user interface).
achieved (such as automatically via an API, or interactively via a user interface).

[=Agent Manager=] as [=Link Context=] and the registration endpoint as [=Link
Target=]. The [=Relation Type=] of the [:Link:] header [MUST] hold the semantics
that indicate how to perform an [=Agent Registration Request=] at the [=Link
Target=], e.g. as specified in an extension to this document.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Target=], e.g. as specified in an extension to this document.
Target=], such as those specified in an extension to this document.


[=Agents=] wishing to discover the [=Agent Manager=] for a given [=Entity=]
[SHOULD] dereference an [=HTTP-Dereferenceable Identifier=] of that [=Entity=],
e.g. `https://my.id/doc#me`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
e.g. `https://my.id/doc#me`.
for instance, `https://my.id/doc#me`.


### Discovery Response ### {#sec-amd-response}

Upon reception of a request to the [=Identity Document=], its host [SHOULD]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Upon reception of a request to the [=Identity Document=], its host [SHOULD]
Upon receipt of a request to the [=Identity Document=], its host [SHOULD]


</div>

A host of [=Identity Documents=] [MUST] implement any of the above in order to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A host of [=Identity Documents=] [MUST] implement any of the above in order to
A host of [=Identity Documents=] [MUST] implement at least one of the above to

This document builds on the concepts and definitions of [RDF].
For conformance, terms from [[!RFC9110|HTTP Semantics]] and [LINKS] are used.
[[WEBID|WebID's]] and [[DID|Decentralized Identifiers]] are used as examples.
References to all these are linked throughout the text,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
References to all these are linked throughout the text,
References to all of these are linked throughout the text,

<dfn>RDF-Dereferenceable</dfn> [=Identifiers=] dereference to an [=RDF Document=].

: <dfn>Agent</dfn>
:: An actor, i.e. an [=Entity=] that acts, does stuff, or has the power to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:: An actor, i.e. an [=Entity=] that acts, does stuff, or has the power to.
:: An actor (that is, an [=Entity=]) that acts, does stuff, or has the power to do so.


: <dfn>Agent</dfn>
:: An actor, i.e. an [=Entity=] that acts, does stuff, or has the power to.
It can do this on its own, or mediated by some other [=Agent=].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It can do this on its own, or mediated by some other [=Agent=].
It can do this on its own, or through mediation by some other [=Agent=].

@@ -185,27 +187,16 @@ are processed by the [=Authorization Agent=].

### Authorization Agent Discovery ### {#authorization-agent-discovery}

This document extends the [Agent Manager Discovery](https://solid.github.io/data-interoperability-panel/specification/agent-management/index.html#sec-am-discovery) of the [[sai-am|Agent Manager]] specification,
by defining `interop:hasAuthorizationAgent` as subproperty of `interop:hasAccessManager`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
by defining `interop:hasAuthorizationAgent` as subproperty of `interop:hasAccessManager`.
by defining `interop:hasAuthorizationAgent` as a subproperty of `interop:hasAccessManager`.

@@ -185,27 +187,16 @@ are processed by the [=Authorization Agent=].

### Authorization Agent Discovery ### {#authorization-agent-discovery}

This document extends the [Agent Manager Discovery](https://solid.github.io/data-interoperability-panel/specification/agent-management/index.html#sec-am-discovery) of the [[sai-am|Agent Manager]] specification,
by defining `interop:hasAuthorizationAgent` as subproperty of `interop:hasAccessManager`.
the [=Authorization Agent=] for a given [=Social Agent=] can be discovered
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
the [=Authorization Agent=] for a given [=Social Agent=] can be discovered

Copy link
Member

@justinwb justinwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks pretty good - a few comments / questions

Abstract:

This specification details how Agents can discover the Resource Discovery
Hub of an Entity, and how they can interact with it in order to find
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell this is the only place in this PR where the term "Resource Discovery Hub" is used, but it is never defined anywhere else. For the purposes of Abstract, a more straightforward explanation would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, should be Agent Manager now.

this limited set of <dfn export>Agent-Specific Resources</dfn> &mdash;
[=Resources=] that are accessible by and possibly relevant for a specific [=Agent=]
&mdash; in a performant, secure, and privacy-minded manner,
without having to bother with all other, irrelevant [=Resources=].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this last statement?

without having to bother with all other, irrelevant resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It stresses the efficiency gain of using an Agent Manager, instead of needing to traverse all Resources in order to find out which ones are accessible and relevant.

Do you think we should leave it out?

NOTE: If the [=Entity=] is an [=Agent=] itself,
it can of course manage its own [=Identity Document=],
in which case there is no difference
between the [=Entity=] and its [=Managing Agent=].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A short example of where this would be the case might be useful here.

can use an [AGENT_MANAGER] ([=RDF Class=] `interop:AgentManager`):
a [=Resource=] linked uniquely to the [=Entity=],
at which each [=Agent=] can identify itself
in return for quick access to its [=Agent-Specific Resources=] (if it has any).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit tough to process. Suggest re-wording for clarity.

omitted, in which case the [=Identity Document=] [SHOULD] contain an [=RDF
Triple=] linking the [=Entity's=] [=Identifier=] to the URL of its [=Agent
Manager=] with the predicate `interop:hasAgentManager`. The predicate [MAY] also
be substituted by `rdfs:seeAlso` or `pim:preferencesFile`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the removal. Feels like very quickly an innocuous piece of code code be misconstrued as the trusted agent manager.

- It [MAY] respond with status code [S200] including a [:Content-Location:] header
with the URL of the [=Agent Registration=] URL as value. If the request method
was [:GET:], it [MUST] include a [[JSON-LD11|JSON-LD]] serialization of the
[=Agent Registration=] in the body of the response.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rationale behind the MUST on JSON-LD serialization here?

Comment on lines +25 to +26
&mdash; in a performant, secure, and privacy-minded manner,
without having to bother with all other, irrelevant [=Resources=].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&mdash; in a performant, secure, and privacy-minded manner,
without having to bother with all other, irrelevant [=Resources=].
&mdash; in a performant, secure, and privacy-minded manner.

Copy link
Contributor

@TallTed TallTed Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or possibly

Suggested change
&mdash; in a performant, secure, and privacy-minded manner,
without having to bother with all other, irrelevant [=Resources=].
&mdash; in a performant, secure, and privacy-minded manner,
without trying (and/or failing) to access all other, irrelevant [=Resources=].

@woutermont
Copy link
Contributor Author

Closing this since I am no longer convinced that a separate discovery service is needed (cf. #315). An agent should at all times be able to request access to (types of) resources at an AS, and the combination of the AS's response and a token info endpoint should suffice to gain all necessary information. If not, a better approach than proposed in this PR would be to add a client/agent info endpoint that functions similarly to the OIDC user info endpoint.

@woutermont woutermont closed this Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants