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

Subscription-Issue4: Need to change the scope pattern for explicit subscriptions #163

Closed
shilpa-padgaonkar opened this issue Mar 12, 2024 · 24 comments · Fixed by #177
Closed
Labels
correction correction in documentation subscriptions

Comments

@shilpa-padgaonkar
Copy link
Collaborator

Problem description
The API design guidelines state that scopes should be represented as:

  • API Name: address-management, numbering-information...
  • Protected Resource: orders, billings…
  • Grant-level, action on resource: read, write…

For explicit subscriptions, giving an example of device-status, this pattern results in

device-status:subscriptions:create

And with this subscription, any one of the below event types could be subscribed to:

org.camaraproject.device-status.v0.roaming-status,
org.camaraproject.device-status.v0.roaming-on,
org.camaraproject.device-status.v0.roaming-off,
org.camaraproject.device-status.v0.roaming-change-country,
org.camaraproject.device-status.v0.connectivity-data,
org.camaraproject.device-status.v0.connectivity-sms,
org.camaraproject.device-status.v0.connectivity-disconnected,

The scope device-status:subscriptions:create doesn't give any insights on the event types.

Expected behavior

  • For explicit subscriptions, the scope should give some insights into which event types could be subscribed to.

For e.g.
device:roaming-status-sub:create

  • The new pattern should be documented in API design guidelines for explicit subscriptions
  • The subprojects who have implemented this pattern for explicit subscriptions should make changes as agreed in design guidelines.

Alternative solution

Additional context

@shilpa-padgaonkar shilpa-padgaonkar added correction correction in documentation subscriptions labels Mar 12, 2024
@Kevsy
Copy link
Collaborator

Kevsy commented Mar 12, 2024

Makes sense to me - regarding proposed syntax, device:roaming-status-sub:create could also allow a wildcard covering all permissions for that subscription type, i.e. device:roaming-status-sub:* to also support read/update/delete.

But roaming-status-sub is not bound to the event type, org.camaraproject.device-status.v0.roaming-status. So either the full event type name should be used,

device:subscription:org.camaraproject.device-status.v0.roaming-status:create

or there should be a definition for the supported short names in the SubscriptionEventType definitions. WDYT?

@bigludo7
Copy link
Collaborator

Hello
For me we need to have very explicit scope indicating the information subscribed.

As the guidelines states:

Scopes should be represented as:
API Name: address-management, numbering-information...
Protected Resource: orders, billings…
Grant-level, action on resource: read, write…

What about something like this:

            - device-status:subscriptions:read-roaming-status
            - device-status:subscriptions:read-roaming-on
            - device-status:subscriptions:read-roaming-off
            - device-status:subscriptions:read-roaming-change-country
            - device-status:subscriptions:read-connectivity-data
            - device-status:subscriptions:read-connectivity-sms
            - device-status:subscriptions:read-connectivity-disconnected
            - device-status:subscriptions:create

device-status:subscriptions:create is a wild card for all event type.

For the GET & DELETE we can keep it simple:

            - device-status:subscriptions:read

and

            - device-status:subscriptions:delete

@shilpa-padgaonkar
Copy link
Collaborator Author

@Kevsy @bigludo7 Thanks for your comments.

@Kevsy : Yes, I was trying to indicate with the example that may be for explicit subscriptions, some shorter naming convention could be defined in the design guidelines and then used accordingly (and was hoping that api name is not part of it :) ).

@bigludo7: I agree with the idea of keeping the get and delete simple as suggested above.

Regarding create:
I am ok with the format you have shown above if others in WG agree, though I must confess that I am not particularly fond of the idea of using api-name within the scope for explicit subscriptions. (I had not liked this idea of using the api name even in the event type when we had this discussion early on in commonalities :(, but I guess that ship has already sailed ).

BTW, I heard that device status subproject is currently discussing the idea about splitting the API going ahead.

  • If they are called roaming-status.yaml and connectivity-status.yaml (just as example) after the split, and if subscription endpoint stays in each of those APIs, then based on the format you have suggested above, the scope to represent roaming-status event type would look as below:

roaming-status:subscriptions:read-roaming-status

  • If subscription endpoints land in separate API specs of their own and have another name, then it would depend on this name.

another-api-name:subscriptions:read-roaming-status

Is this understanding correct?

BTW is it decided if device status will align more closely to device location and move subscription to a separate API or not?
The reason I ask is that, it not only impacts the scope but the event type will have to change from org.camaraproject.device-status.v0.roaming-status to either org.camaraproject.roaming-status.v0.roaming-status or org.camaraproject.<some-name>.v0.roaming-status as well.

@bigludo7
Copy link
Collaborator

Hello @shilpa-padgaonkar

For splitting the device status API this is an in progress discussion and not decision yet.
The point was raised & supported by Noel (from DT) ... today and in 30 minutes both Telefonica (with Jorge) and Orange (myself) supported fully the idea of having 4 APIs. We need of course more feedback from the community (here)

In this case, yes the event should change to org.camaraproject.<some-name>.v0.roaming-status and we have to find a good name for this.

I realized that in Geofencing API we have geofencing:subscriptions:write scope which probably not good as we need to more precise. I will craft an issue there and probably good to have @jlurien & @PedroDiez feedbacks on this topic.

@shilpa-padgaonkar
Copy link
Collaborator Author

@bigludo7 : Yes I am aware that there is no decision yet :) hence I mentioned

"the subproject is currently discussing the idea"

Thanks for creating the issue, and my guess is that at least the subprojects which I had listed here in the comment, follow the same format for scope #153 (comment)

Not sure if they have seen this discussion here though...

@shilpa-padgaonkar
Copy link
Collaborator Author

@jlurien @PedroDiez : Any feedback on this topic?

@PedroDiez
Copy link
Collaborator

Hi,

@shilpa-padgaonkar, @bigludo7 we are checking this internally. Expect to comment on this tomorrow

@PedroDiez
Copy link
Collaborator

Hello,

thanks to raise this topic @shilpa-padgaonkar. This topic fully makes sense and we think has benefits to tackle it.

From our point of view, we agree about the main ideas exposed by all in this thread. Our considerations would be:

  • For subscriptions creation, the allowed scopes in the context of a given API would be:

<api-name>:subscriptions:create -> provides "rights" to create subscriptions for any of the event-types under the APi scope
<api-name>:<event-type>:subscriptions:create -> provides fine-grained "rights" to create subscriptions for a specific

  • For query and read subscriptions, we agree on the simplification, as far as the "creation" phase determines what an API client is allowed to manage.
<api-name>:subscriptions:read
<api-name>:subscriptions:delete

Additionally, another point we would like to share with you all, as we consider it is relevant in this issue is how to deal with the case of creating a subscription when the scope "rights" do not match with the susbcription detail.

E.g. in device status an API client has scope to create subscriptions for roaming status so scope:

device-status:roaming-status:subscriptions:create

An it tries to create a subscription with subscription detail body according to type: "org.camaraproject.device-status.v0.connectivity-data"

In this scenario, we propose to raise an HTTP 403 exception with code:

  • SUBSCRIPTION_NOT_ALLOWED or SUSBCRIPTION_MISMATCH (just a initial wording, we can agree in other naming if seen better)

@bigludo7
Copy link
Collaborator

@PedroDiez Thanks & good to see that we're aligned.
Agreed for the 403 proposal.

@shilpa-padgaonkar
Copy link
Collaborator Author

@PedroDiez and @bigludo7 : Good to know that we are aligned on the fact that event types are relevant in defining scopes.

@PedroDiez : SUSBCRIPTION_MISMATCH sounds good.
SUSBCRIPTION_NOT_ALLOWED might be useful in situations where the end user might have denied consent?

@bigludo7
Copy link
Collaborator

@shilpa-padgaonkar @PedroDiez Do you want that we wait for a global alignement on all subscriptions related topic to update the guidelines or can we already propose PR?

For this one, probably we can propose an updated here https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#1161-scope-naming but looking on your view before to craft a PR.

@PedroDiez
Copy link
Collaborator

@PedroDiez and @bigludo7 : Good to know that we are aligned on the fact that event types are relevant in defining scopes.

@PedroDiez : SUSBCRIPTION_MISMATCH sounds good. SUSBCRIPTION_NOT_ALLOWED might be useful in situations where the end user might have denied consent?

Yes, better SUSBCRIPTION_MISMATCH in the context of this topic

@PedroDiez
Copy link
Collaborator

To me is fine to move forward on this @bigludo7

@shilpa-padgaonkar
Copy link
Collaborator Author

@bigludo7 : I am ok to move forward with a PR for the scope topic, as most of us are aligned here.

@shilpa-padgaonkar
Copy link
Collaborator Author

shilpa-padgaonkar commented Apr 9, 2024

@PedroDiez @bigludo7

BTW, I looked at recent comments in device status separate endpoint issue and looked at the possible API names suggested
Let's say we go for device-status-connectivity-subscriptions.yaml

then as per the discussed pattern above <api-name>:<event-type>:subscriptions:create we would have

device-status-connectivity-subscriptions:org.camaraproject.device-status-connectivity-subscriptions.v0.connectivity-data:subscriptions:create

This means that we repeat the API name twice,

  • once when we add it to our scope <api-name>:<event-type>:subscriptions:create and
  • the second time because we add it to the event type org.camaraproject.<api-name>.<api-version>.<event-name>.

Do we need to repeat it twice or could we get rid of api name from the event type field?

@bigludo7
Copy link
Collaborator

Fair point @shilpa-padgaonkar !
BTW: Regarding device status I've proposed a simplification in the name.

More globally should we make following proposal:

  • When we have a dedicated yaml for subscription management then subscription must be part of the API name
  • In this case, in the scope (as defined in commonalities 10.2.b) , the API-name prefix could be omitted as it is part of the event type structure.

I tend to prefer keeping the API-name in the event type as this one will circulate 'alone' in event.
WDYT?

@PedroDiez
Copy link
Collaborator

PedroDiez commented Apr 10, 2024

Good point @shilpa-padgaonkar

When proposing this approach:

<api-name>:subscriptions:create -> provides "rights" to create subscriptions for any of the event-types under the APi scope
<api-name>:<event-type>:subscriptions:create -> provides fine-grained "rights" to create subscriptions for a specific

Indeed i was meant (i was thinking about):
<api-name>:<event-name>:subscriptions:create

Because in the context of this functionality:
org.camaraproject.<api-name>.<api-version>.<event-name>

  • org.camaraproject.: is part of the cloudevent convention
  • <api-name> is already considered in the scope format
  • <api-version> is within the context of a specific API version
  • <event-name>: Indeed is the relevant semantic information to provide the fine-grained business logic

In other words within a given API, the event-name must be unique so as it is the relevant key to identify the event

@shilpa-padgaonkar
Copy link
Collaborator Author

@bigludo7 Would be ok to get the api name out of the scope as you have suggested above.
But, do you think we might have a situation where an event type might be part of both implicit and explicit subscription? Or do we think such a case is not possible? I am not able to come to a conclusion here. May be you and @PedroDiez can help here.

@shilpa-padgaonkar
Copy link
Collaborator Author

@bigludo7 May I am overthinking ;) Let's start with your proposal
@PedroDiez Hope its ok with you

@PedroDiez
Copy link
Collaborator

@shilpa-padgaonkar, @bigludo7
I am not sure of that, due to the format indicated in section 11.6 of commonalities:

https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#116-security-definition

@bigludo7
Copy link
Collaborator

@shilpa-padgaonkar, @bigludo7 I am not sure of that, due to the format indicated in section 11.6 of commonalities:

https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#116-security-definition

Agree on the interpretation @PedroDiez but don't you think that we can raffine this ?
We have rules colliding here:

  • scope must have api-name as prefix
  • eventType must have the api-name
  • scope for event require eventType(not a rule...yet ... but the result of our discussion)

Result: If we want to avoid duplicate of the api-name we have to update the scope or the eventType.

Removing the api-name suffix in scope for event based scope make us not lost information because you'll have the api-name in the eventType

alternate will be to keep the duplicate but this not very elegant.

WDYT?

@shilpa-padgaonkar
Copy link
Collaborator Author

@PedroDiez @bigludo7 I have created a PR to just see how the changes would look like. If we think it's too premature, I don't mind changing the PR to draft PR. Just thought, it could get us started somehow :)

@PedroDiez
Copy link
Collaborator

@bigludo7, @shilpa-padgaonkar

Let's talk tomorrow. The model proposed is therefore this?

<event-type>:subscriptions:create

@shilpa-padgaonkar
Copy link
Collaborator Author

@PedroDiez
We currently have 2 options:

  1. Keep the way of defining scopes as-is but then remove api-name from event-type (and use simpler categorization to ensure that event-types are unique)
  2. Change how scopes are defined in explicit subscription APIs with the said format -><event-type>:subscriptions:create so that we do not introduce api-name twice in a scope field.

@bigludo7 supports option2. I lean towards option1, but if the group decides that option2 is the way to go, I won't object.
As we were not moving forward with many subscription issues, I created this PR which uses option2 to see if we can move forward and get an alignment. Let's discuss in the call today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
correction correction in documentation subscriptions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants