Bug: New custom exceptions expose undifferentiated 403 response error

[cainanmahar]

Previously, a 403 response error would generate a ValueError which was caught by BlackboardSync._sync_task(...) and handled as an authorization error. Because the subsequent call to BlackboardSync.auth(...) simply recreated the BlackboardSession and returned (effectively a no-op), the application would simply (attempt to) download the next item.

With the new, differentiated BBAuthError, this response code is no longer handled with this no-op. This will place the BlackboardSync object into an error state. Unfortunately, the response that I receive does not have any fields other than a message field; there is nothing to check for.

I would like to handle this type of response code explicitly instead of resetting the session, like is done with a 403 response for private courses. Since there is no clear way to differentiate different 403 responses, a base exception class titled BBForbiddenError should suffice. BBPrivateCourseError will inherit from this, as it is a 403-related error. I will edit this issue to reference the PR once the new exceptions are completely integrated.

[cainanmahar]

After more digging, this 403 is triggered by an attempt to get the children of a course content object where the availability is 'No' (not False).

[sanjacob]

Seems like a good idea to check for course availability beforehand.

Pydantic used to convert No to false implicitly if I recall correctly, maybe you can see if this can be enabled somehow, otherwise we can implement a property on the BBAvailability class to return the value of the bool or if its a string check for 'No'.

Make sure that we are not lumping together 403's caused by the Blackboard API and otherwise, because the former is just a error with that course or file, the latter can be a site-wide authentication error that effectively demands the user to reauthenticate.

[sanjacob]

Some of the implicit conversions are listed here ('no' is listed but Im not sure if it is case sensitive)

https://docs.pydantic.dev/latest/concepts/conversion_table/

[sanjacob]

I see what's happening. Because we have allowed the field to be a str to support 'Term' and others the value is no longer casted to a bool. I guess we have to do it manually.

[sanjacob]

Ideally, there would be a way to let Pydantic try to cast it into a bool and if that failed then take it as a string, but I don't think it's likely

[cainanmahar]

Right now in my local repo I have a method with a validate_field annotation just for testing that simply returns True when handed anything other than 'No' or 'False'. Maybe a new model is in order? Looks like BB is using this field as an enum. Could throw a boolean cast method on there and it should work fine.

[sanjacob]

Yeah, a validate_field sounds good. We don't really care about other possible values so I don't think an Enum is necessary here.

[sanjacob]

I have been thinking about it, and maybe it's worth it to have an Enum, and just do something akin to this
https://github.com/jacobszpz/BlackboardSync/blob/8e7acee46199e01596576155ec9b55458516fce9/blackboard_sync/blackboard/blackboard.py#L124

Of course with BBResourceType this is overriden by a custom validator that maybe can be tweaked to allow a string to avoid something like #106 ?

https://github.com/jacobszpz/BlackboardSync/blob/8e7acee46199e01596576155ec9b55458516fce9/blackboard_sync/blackboard/blackboard.py#L135-L138

I'd probably just take the Enum, str option to be consistent but if you think it's easier to have a validator like you described you can take that route too

[cainanmahar]

I think I'll try to maintain consistency. I should be implementing a __bool__ method to preserve the old boolean functionality, right?

[sanjacob]

Sure, sounds good

[cainanmahar]

Ideally, there would be a way to let Pydantic try to cast it into a bool and if that failed then take it as a string, but I don't think it's likely

I've found (this page)[https://docs.pydantic.dev/latest/api/standard_library_types/#union] that indicates there is a way to do this with union_mode and Field, however it appears to only work on >=2.2.0 which includes this pull. Currently,

$ pip list | grep pydantic
pydantic                      2.1.1

does not support this.

[sanjacob]

That's a really good find. I don't mind bumping pydantic and in fact we need to do it regardless. I have glanced at the changelog and there shouldn't be any breaking changes but let's test throughly as last time I bumped pydantic chaos ensued.
Furthermore, we have one more thing to fix, as the flatpak release actions workflow has a hardcoded version of pydantic-core if I recall correctly.

[sanjacob]

This change should mean that when possible, strings like 'Yes', 'No' will be casted to a bool directly. But strings like 'Term' won't. So we will assume True if a string was passed (as 'No would have already been casted to False by then)

[cainanmahar]

I did notice that there is a value of 'Disabled' that will return a 403 when the associated resource is walked. We should consider that cause as well. I'll adapt the code in #111 to match this behavior otherwise, though.

[sanjacob]

I'll take a deeper look at this either today or tomorrow, but expect me to update the branch and merge within the next few days