search

hypothesis / lms

LTI app for integrating with learning management systems
BSD 2-Clause "Simplified" License
46 stars 14 forks source link

Include admin email in token refresh error messages #6302

Open robertknight opened 4 months ago

robertknight commented 4 months ago

Address the feedback in https://github.com/hypothesis/lms/pull/6290 about providing a more specific error message that indicates the email address of the admin user we are using, when refreshing the admin token fails:

I think the error messages need to be more specific. The last thing we want is for the feature to stop working for the whole school and to brick-wall users with an overly-generic error message that doesn't give them the information they need to fix the problem.

See also this thread in Slack about issues seanh ran into while tesitng this.

seanh commented 4 months ago

Just to add that I think the error message should also say what exactly the admin user needs to do to fix the problem. (I think they need to launch any Hypothesis Canvas Studio assignment.)

robertknight commented 4 months ago

An addition to this. In early testing with a customer we ran into an issue where the admin had a mixed-case email address and the case of the email in the LTI launch didn't match that in the Canvas Studio settings. The initial launch of an assignment failed with a message that the admin had not authenticated. The problem would have been more obvious if the email address was present in the settings.

The code currently does a naive string comparison of email addresses, and is not aware of email providers which treat the prefix as case insensitive, or normalize prefixes in other ways for comparison (eg. ignoring periods as Gmail does).