AP-580: Add validations for CalNet user attributes

[yzhoubk]

During Rails upgrade code review testing, we discovered users were missing email addresses. Investigation showed that a CalNet schema attribute had been renamed, though we were not notified when the change occurred. This ticket aims to address unexpected attribute changes from CalNet and improve our handling of schema updates.

[awilfox]

This looks great and should help with debugging AP-583 as well. r+ after tiny question.

[awilfox]

Ah, sorry, I didn't see the test failures. Looks like Rubocop has some suggestions, and we need to fix up the mock user objects in the other tests to have the required attributes. Still, I approve of the overall work - just need to fix those issues before merging.

[anarchivist]

agreed with @awilfox - this is looking good, but let's address the test failures before moving forward.

[yzhoubk]

@anarchivist, @awilfox

I added berkeleyEduAlternateId as a fallback attribute when retrieving the email from CalNet. I also moved the CalNet attribute name mappings into a config file so they can be shared by both the User model and CalNet-related tests, without modifying the CalNet data fixtures.

For the student ID value, since there is no berkeleyEduStuID attribute in the response, I temporarily fell back to using berkeleyEduCSID, as the fixture data and comments suggest that these two attributes contain the same value.

However, after further review, I think we should not introduce a fallback for the berkeleyEduStuID attribute. In my local CalNet response, the berkeleyEduStuID attribute is missing simply because I am not a student. Previously, the User model did not validate CalNet attributes, so we did not notice the absence of berkeleyEduStuID. - That's my guess.

If this is the case, a student might also not have an employeeNumber if he/she is not a student employee.

It seems we need a sample CalNet response for an actual student account to clarify the expected attributes. Based on that, we can decide how to properly update the current validation logic for studentID and employeeID, or we just eliminate to validate related attributes: berkeleyEduStuID and employeeNumber .

I will rebase locally to fix the conflict tomorrow.

[awilfox]

@yzhoubk I agree that we should gather a real CalNet student response, anonymise it, and then use it as part of our testing. It would be beneficial to have both a student employee and a student non-employee to see how employeeNumber behaves.

The overall direction of this MR looks good. I will review it in-depth when you are ready.

[yzhoubk]

The PR isn’t ready for review yet — I’m planning to refactor and restructure some of the code .

[yzhoubk]

@awilfox It's ready for review. I've added implementation notes in slack, please take a look.

[awilfox]

A few minor typos, but otherwise looks great. Going to test against Rails 7.2 in a bit.

[awilfox]

Suggested change

	cs_id: auth_extra['berkeleyEduCSID'], # Not included in CALNET_ATTRS because it's not used by any applications; Just keept it here.
	cs_id: auth_extra['berkeleyEduCSID'], # Not included in CALNET_ATTRS because it's not used by any applications; Just keep it here.

minor typo.

[awilfox]

Suggested change

	missing_attrs = "Expected Calnet attribute(s) not found (case-sensitive): #{missing.join(', ')}."
	missing_attrs = "Expected CalNet attribute(s) not found (case-sensitive): #{missing.join(', ')}."

[awilfox]

Suggested change

	msg = "Expected Calnet attribute(s) not found (case-sensitive): #{missing.join(', ')}. The actual CalNet attributes: #{actual.join(', ')}. The user is expected display name"
	msg = "Expected CalNet attribute(s) not found (case-sensitive): #{missing.join(', ')}. The actual CalNet attributes: #{actual.join(', ')}. The user is expected display name"

when the other fix is applied, this will need to be as well.

[yzhoubk]

Thanks for catching the typos!

[awilfox]

Confirmed that this works on Rails 7.2 as well (applied atop #28).