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

Admin tool to clear application or system tokens #1695

Open
tlock opened this issue May 31, 2021 · 7 comments
Open

Admin tool to clear application or system tokens #1695

tlock opened this issue May 31, 2021 · 7 comments
Assignees
Labels
Issue type - enhancement request New feature being requested outside of original scope. Plugin - local_o365 Triaging status - triaged A ticket has been created accordingly in the maintainers' ticket system.

Comments

@tlock
Copy link

tlock commented May 31, 2021

$DB->delete_records('config_plugins', ['plugin' => 'local_o365', 'name' => 'task_usersync_lastdeltatoken']);

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
Copy link
Author

tlock commented May 31, 2021

Office365-Maintenance-Reset-Token.zip

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

@nenorojas nenorojas added the Triaging status - triaged A ticket has been created accordingly in the maintainers' ticket system. label May 31, 2021
@weilai-irl
Copy link
Collaborator

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

@tlock
Copy link
Author

tlock commented Jun 2, 2021

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

@weilai-irl
Copy link
Collaborator

Hi @tlock,

Am I right in understanding this issue can be closed?

Regards,
Lai

@tlock
Copy link
Author

tlock commented Jun 29, 2021

Hi @weilai-irl ,

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

@weilai-irl
Copy link
Collaborator

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

@tlock
Copy link
Author

tlock commented Jun 30, 2021

Hi @weilai-irl,

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

@weilai-irl weilai-irl changed the title Major functional issue on upgrade Admin tool to clear application or system tokens Jul 7, 2021
@weilai-irl weilai-irl added Issue type - enhancement request New feature being requested outside of original scope. Plugin - local_o365 labels Jul 26, 2021
@nenorojas nenorojas assigned audrieMSFT and unassigned weilai-irl Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue type - enhancement request New feature being requested outside of original scope. Plugin - local_o365 Triaging status - triaged A ticket has been created accordingly in the maintainers' ticket system.
Projects
None yet
Development

No branches or pull requests

4 participants