Replace application-level permissions with user-delegated ones

[mickmister]

It looks like the two places where the application permissions are exercised are during two of the scheduled jobs in the plugin, the "daily summary" and "status sync" jobs. Any feature related to user action (meaning not related to these jobs) should not break when these permissions are not set.

As far as eliminating the need for these permissions, we may be able to adjust the code to use individual user tokens within the subrequests of the batch requests for these two jobs. I attempted this before but ran into issues with Microsoft's API.

Issue created from a Mattermost message by @mickmister.

[Kshitij-Katiyar]

@mickmister On a different note. I was exploring the code of the ms Calendar plugin and found something that may be why the refresh token for the super-user client is not working correctly. Please see:-

mattermost-plugin-mscalendar/server/remote/msgraph/get_super_user_token.go

Line 41 in c542892

return res.AccessToken, nil

Here we are only returning the access token and using that we are creating the client. Here I am not able to understand why we are only using the access token and not both (refresh token as well) because without the refresh token, the Oauth library won't be able to refresh the access token and it will eventually expire.
Please give your views on this.

[Kshitij-Katiyar]

@mickmister For fixing the issue. We have to replace the application level with user-delegated ones. As we know that superClient is using the application permissions and they are getting used in the user status sync job and daily summary job. This token is used in calling the batch request which eventually gets the events for all the users which we get from the userIndex array.

The approach I was thinking of is that first, we get the user from the userIndex array and then we will get the token of that user if he is connected, and using that token we will call the API to get the events and eventually change the user status and for daily summary job we will post the summary. Now as we are calling the API for that user only we will only require the user level permissions.

But here we will only be able to change the status and post summary for the users which are connected to mscalender. We wont be using the batch request API as we will be using different tokens for different API calls so the API calls will also be increased.
Please give your views on this and if you have any other approaches feel free to suggest.

[mickmister]

On a different note. I was exploring the code of the ms Calendar plugin and found something that may be why the refresh token for the super-user client is not working correctly. Please see:-

@Kshitij-Katiyar The superuser token response does not contain a refresh token. This is because we are using grant type Client Credentials here:

mattermost-plugin-mscalendar/server/remote/msgraph/get_super_user_token.go

Lines 20 to 25 in c542892

	params := map[string]string{
	"client_id": c.conf.OAuth2ClientID,
	"scope": "https://graph.microsoft.com/.default",
	"client_secret": c.conf.OAuth2ClientSecret,
	"grant_type": "client_credentials",
	}

When we want to perform an operation with the Superuser token, we simply request a new access token every time we perform an operation. This is convenient and simple, but some Azure access configurations that customers have will disable these Superuser token creations. In general, per-user tokens are more accepted from a security mindset, so we want to implement the task described in this ticket.

---

But here we will only be able to change the status and post summary for the users which are connected to mscalender.

This seems normal to me, no issues here.

We wont be using the batch request API as we will be using different tokens for different API calls so the API calls will also be increased.

I believe we can apply a given user's token for each individual request. From these docs, it shows that we can include custom headers in the requests https://learn.microsoft.com/en-us/graph/json-batching

{
  "requests": [
    {
      "id": "4",
      "url": "/me",
      "method": "PATCH",
      "body": {
        "city" : "Redmond"
      },
      "headers": {
        "Content-Type": "application/json"
      }
    },
    {
      "id": "5",
      "url": "users?$select=id,displayName,userPrincipalName&$filter=city eq null&$count=true",
      "method": "GET",
      "headers": {
        "ConsistencyLevel": "eventual"
      }
    }
  ]
}

We should be able to include a user's token in the headers of the individual requests. From the description of this ticket, I mentioned "I attempted this before but ran into issues with Microsoft's API.", this is what I was referring to. Though I wasn't able to spend much time on it, and I think I was doing something wrong. Can you please check if this is a practical solution?

[Kshitij-Katiyar]

@mickmister I have explored the batch APIs. I tried sending the authorization token in the headers and query params of the sub-requests of a batch API but I got the response according to the token passed for the batch API request and the token passed to each request was ignored. I tried sending other headers to the sub-request to verify if those headers are being considered or not, turns out other headers were being considered but the authorization header was being ignored. I tried to find how to support multiple access tokens in a batch request in ms-graph but could not find anything regarding the same not on the community as well.
If you have any documents links or views on this please feel free to share them here.

[mickmister]

@Kshitij-Katiyar I suppose we'll need to implement our own batching in this case. We can use goroutines to fan out these requests concurrently.

A simple solution would be to create a goroutine for each connected user as we loop through them, and use a WaitGroup to synchronize the results. Here's an example implementation of this https://stackoverflow.com/a/45338208. Alternatively, we could create a goroutine pool with a fixed number of goroutines, using a Go channel to synchronize the concurrent operations.

I've opened a discussion on the community server for this https://community.mattermost.com/core/pl/7gi1wcnw6ff75rctcoox3xrhiy, to get other people's thoughts on this.

[mickmister]

@Kshitij-Katiyar As we won't be using the batch API anymore, our local batching algorithm will need to take into account these throttling limits https://learn.microsoft.com/en-us/graph/throttling-limits#limits-per-app-id-and-mailbox-combination:

• 4 concurrent requests
• 2000 requests per second
• 10,000 requests in a 10 minute period

The "4 concurrent requests" restriction forces our solution to do the concurrent request limiting mentioned above. We can use a Go channel as a means to synchronize goroutines performing concurrent requests, and limit it to a maximum of 4 requests at a time.

---

Note that the status sync job runs every 5 minutes, while the daily summary job runs every 15 minutes and performs the same requests of fetching user calendar data. To limit the number of concurrent operations, we should have the daily summary job "piggyback" off of the status sync job, and handle both jobs' use cases with the same responses from msgraph. At the beginning of the status sync job, if we are within 1 minute of a 15 minute point on the hour, then we should also handle the daily summary feature after we get the calendar info. Then we can remove the daily summary "job".

I've created a forum post in Microsoft's system discussing the issue we're running into with the batch request API and the Authorization header https://learn.microsoft.com/en-us/answers/questions/1276274/using-authorization-header-in-individual-requests

---

Also, I noticed this batched GetSchedule function is not called, so we can remove it in this effort.

One last comment: This fix will be a breaking change, so I think we should have this be configurable if possible. Meaning, the admin should be able to decide if they want to use application-level or user-delegated permissions for these features, through a plugin config setting.

[raghavaggarwal2308]

@mickmister I explored about making the 4 concurrent requests in batches and I have
found two approaches:

1. We can implement our own goroutine pool: We can use Go routines for concurrent requests, use Go channels to store the results from each API call and limit it to a maximum of 4 requests at a time by using a channel of length 4.
   Example: https://go.dev/play/p/sM_dhE4t22p

2. We can use the goroutine pool package: https://github.com/Jeffail/tunny

What do you think which approach should we proceed with? Also, please feel free to suggest if you have any other approach in mind

[mickmister]

@raghavaggarwal2308 I would hold off on implementing this for now. We are in the process of determining if this fix is necessary

[raghavaggarwal2308]

@mickmister I am not sure I get this. Are you talking about holding off the whole feature or just holding on the feature to make 4 concurrent requests at a time.

[mickmister]

@raghavaggarwal2308 I mean the feature in general

[mickmister]

@raghavaggarwal2308 We were able to resolve this on the customer's side, so this task should not be worked on now. Closing this issue since we will likely not implement this

[ThiefMaster]

It would be nice to have the option to not require delegated permissions. IN PARTICULAR not ReadWrite which means the plugin can write to anyone's calendar without them explicitly authorizing the app by connecting their Exchange account via OAuth.

I understand that for sync it makes sense to batch those requests in order to not run in any rate limits, but - understandably - my mail admin is a bit hesitant to grant delegated access to everyone's calendar.

[mickmister]

IN PARTICULAR not ReadWrite which means the plugin can write to anyone's calendar without them explicitly authorizing the app by connecting their Exchange account via OAuth.

@ThiefMaster Note that "delegated" means "user-delegated", meaning the user does need to be connected. https://learn.microsoft.com/en-us/graph/auth/auth-concepts#access-scenarios

The superuser permissions do not include write permissions for anything. Only for connected accounts is the plugin able to change calendar event data.