camaraproject / Commonalities

Repository to describe, develop, document and test the common guidelines and assets for CAMARA APIs
Apache License 2.0
12 stars 25 forks source link

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

Closed shilpa-padgaonkar closed 5 months ago

shilpa-padgaonkar commented 6 months ago

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

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 e.g. device:roaming-status-sub:create

Alternative solution

Additional context

Kevsy commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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.

roaming-status:subscriptions:read-roaming-status

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 commented 6 months ago

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 commented 6 months ago

@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 https://github.com/camaraproject/Commonalities/issues/153#issuecomment-1980916412

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

shilpa-padgaonkar commented 6 months ago

@jlurien @PedroDiez : Any feedback on this topic?

PedroDiez commented 6 months ago

Hi,

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

PedroDiez commented 6 months ago

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:

<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

<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:

bigludo7 commented 6 months ago

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

shilpa-padgaonkar commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

To me is fine to move forward on this @bigludo7

shilpa-padgaonkar commented 5 months ago

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

shilpa-padgaonkar commented 5 months ago

@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,

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

bigludo7 commented 5 months ago

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

More globally should we make following proposal:

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

PedroDiez commented 5 months ago

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>

shilpa-padgaonkar commented 5 months ago

@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 commented 5 months ago

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

PedroDiez commented 5 months ago

@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 commented 5 months ago

@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:

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 commented 5 months ago

@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 commented 5 months ago

@bigludo7, @shilpa-padgaonkar

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

<event-type>:subscriptions:create

shilpa-padgaonkar commented 5 months ago

@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.