Release 07/05/2023 #942
Release 07/05/2023 #942
Conversation
Implementing people search by involvements
continue project from MyProfile-Involvments-Clutter - resolved merge conflicts
format ternary operator
…n-max-length Modify code on api after increasing max length for description on the database
| /// <summary> | ||
| /// Gets the memberships of the given user | ||
| /// </summary> | ||
| /// <param name="username"> username filter </param> | ||
| /// <returns>The number of followers of the activity</returns> | ||
| [HttpGet] | ||
| [Route("student/{username}")] | ||
| [StateYourBusiness(operation = Operation.READ_ONE, resource = Resource.MEMBERSHIP)] // not sure why | ||
| public ActionResult<int> GetMembershipsOfUser(string username) | ||
| { | ||
| var result = _membershipService.GetMemberships(username: username, sessionCode: "*"); | ||
|
|
||
| return Ok(result); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't understand why this route was added. All it does is implement d subset of the base /memberships route, in basically the same way as the /profiles/{username}/memberships route. The latter is better in terms of REST API design, but it was marked obsolete because the base /memberships route already covers all it's possible use cases with optional query params.
It looks like this was added for gordon-cs/gordon-360-ui#1083, but the better route to use for that should be /profiles/{username}/memberships-history, the same way it's currently used for the Experience Transcript.
There was a problem hiding this comment.
Antonia and Amos confirmed that the new endpoint is not needed, and in the process found an apparent security flaw in the recommended endpoint: if given no username, it returns all involvements for all users, without filtering to only public involvements. Antonia is working on a fix for both issues.
There was a problem hiding this comment.
Antonia and Amos confirmed that the new endpoint is not needed, and in the process found an apparent security flaw in the recommended endpoint: if given no username, it returns all involvements for all users, without filtering to only public involvements. Antonia is working on a fix for both issues.
If you're talking about the GetMembershipHistory method that I recommended (route /profiles/{username}/memberships-history), I'm fairly certain it can't be called without the username param. If you try, the API sends back this error:
{
"errors": {
"username": [
"The username field is required."
]
},
"type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
"title": "One or more validation errors occurred.",
"status": 400,
"traceId": "00-b582e79374a558d2ff3c822dc9ba1d2a-ddb6eacb502fbb4c-00"
}There was a problem hiding this comment.
Sorry, it's the base /memberships route which appears to have a security flaw.
| /// Gets the memberships of the given user | ||
| /// </summary> | ||
| /// <param name="username"> username filter </param> | ||
| /// <returns>The number of followers of the activity</returns> |
There was a problem hiding this comment.
If we don't delete this endpoint, then the comment will need improving. The returns explanation looks wrong, and the summary should be more explicit about what's included: all memberships over all terms? just public? just those visible to the current user (so different for own profile than public profiles)?
|
We won't push this. If the fixes can be done today, we'll test them for a day on train and consider pushing tomorrow. |
removed unused route
Remove unused route
|
Late removal of unused endpoint (never used in prod, and only briefly in train). |
…-info changed patch to put in both routes
Change 2 new endpoints to put, which is more accurate than patch.
|
I think we've addressed all the issues. But we don't want to merge until tomorrow morning, during working hours, to be sure it doesn't cause problems. |
This Release will merge the following changes into production:
This has been running on 360train since Monday 5:13pm.