o365-moodle icon indicating copy to clipboard operation
o365-moodle copied to clipboard

Admin tool to clear application or system tokens

Open tlock opened this issue 4 years ago • 7 comments

https://github.com/microsoft/o365-moodle/blob/6a210db3779a6da9db544139ccb4e0d12873d199/local/o365/db/upgrade.php#L649

This should have never made into the codebase because everybody with a decent directory will have to fetch again at 200 user per hour unless they increase the frequency for a nice have "additional profile fields" which would occur on next login of the user.

tlock avatar May 31 '21 05:05 tlock

Office365-Maintenance-Reset-Token.zip

This change need to be merge ASAP and upgrade step as noted removed.

tlock avatar May 31 '21 05:05 tlock

Hi @tlock,

Thank you for raising the issue. I understand the root of your concern, but the user sync task already handles it.

The code change was made on purpose and in our tests, it does what it's supposed to do.

  • The delta token needs to be cleared because the delta user Graph API call now requires more user properties to be included in the return value. The properties returned by the delta user API are defined in the first call, and kept in the delta token. Any change to it will require making a fresh request without passing in delta token.
  • After the delta token is cleared, the sync user task will make calls to the user delta API to make get users. You are right that each call will only return a maximum of 200 users, but the task has logic to make the call multiple times, until the all users are returned.
    • In the first call, no delta token is passed, which will ensure it returns the first batch of users from the full list of users.
    • If nextLink is returned, it means there are more users to return, and the skiptoken is stored. The skiptoken is used in a new call to the delta user API to return the next batch of users.
    • If deltaLink is returned, it means it reaches the last batch of users, and the deltatoken is stored.

The logic ensures it will not process only 200 users in a task run.

On a separate note, could you confirm based on which version was your patch file created please. It cannot be applied in the latest code in MOODLE_39_STABLE or MOODLE_310_STABLE branch.

Regards, Lai

weilai-irl avatar May 31 '21 09:05 weilai-irl

Hi Lai,

The code was based on 3.8.0.1 and detected this issue while looking at the 3.10 code.

In regard the raised issue, I didn't noticed the loop to get more than 200 users per cron run. That itself is great, but running the sync previously from start to finish was hours which concerns me because I don't see any guard again the cron job running over the top every hour.

As discussed originally, force re-sync needs to controlled and additional fields will not be included to till that has occur by running the maintenance operation to "Hard reset" the tokens.

Therefore, I have commented out the problem line and will address separately.

Regards, Tim

tlock avatar Jun 02 '21 02:06 tlock

Hi @tlock,

Am I right in understanding this issue can be closed?

Regards, Lai

weilai-irl avatar Jun 29 '21 15:06 weilai-irl

Hi @weilai-irl ,

Are you able to implement the code as suggested, then I'd be happy for this to be closed.

tlock avatar Jun 29 '21 23:06 tlock

Hi @tlock,

As explained in my first comment, unsetting the task_usersync_lastdeltatoken config variable to force a full user sync is a necessary step during upgrade in order to receive additional profile fields from Azure AD in Graph API call. This is due to the design of the user: delta Graph API. In reality, this will cause the first run of the user sync task after the plugin upgrade to take longer time to finish, but it will not be only processing 200 users per run.

If the purpose of your proposed change is to provide a way for Moodle site admins to manually reset the task_usersync_lastdeltatoken config variable, this would be categorised as a feature request, and will be scheduled for a future release. Or please update your patch to be based on a more recent version of the code and possibly create a PR, so that it can be reviewed and merged more easily.

Regards, Lai

weilai-irl avatar Jun 30 '21 10:06 weilai-irl

Hi @weilai-irl,

I will update the patch with the latest code in due course and post here.

tlock avatar Jun 30 '21 21:06 tlock