Addresses most of Brandon's concerns #14
Conversation
There was a problem hiding this comment.
This is a great start. In addition to the comments in the files, I would suggest looking through the Error List tab for the files you've added and make sure Warnings are fixed, and Messages that are not just "style" related are fixed (for example, the unused variable messages).
There are still some things missing, compared to the old directory: phone numbers, the email host, user address, and student affiliations. Maybe the title and department columns can be combined to display this data. Also the Alt Photo link should be shown for users with SVMSecure.CATS.ServiceDesk.
How do you want to handle the "All UCD" mode of the directory, where it searches the campus LDAP first, and then adds our info?
I appreciate the work you've done on this! Once we figure out the permission issues with the IDs I can approve this as a starting point.
| { | ||
| //TODO: Create the Directory Delegate Users role and add anyone with access to delegate roles | ||
| [Area("Directory")] | ||
| [Authorize(Roles = "VMDO SVM-IT,RAPS Delegate Users", Policy = "2faAuthentication")] |
There was a problem hiding this comment.
[Permission(Allow = "SVMSecure")] is probably a sufficient permission check. As it is now, this restricts the page to a much smaller set of users.
| [Authorize(Roles = "VMDO SVM-IT,RAPS Delegate Users", Policy = "2faAuthentication")] | ||
| public class DirectoryController : AreaController | ||
| { | ||
| private readonly Classes.SQLContext.RAPSContext _RAPSContext; |
There was a problem hiding this comment.
Make sure to remove copy/pasted variables that are not needed in this class
| List<IndividualSearchResult> results = new(); | ||
| individuals.ForEach(m => | ||
| { | ||
| results.Add(new IndividualSearchResult() |
There was a problem hiding this comment.
We might need a permission check here for the IDs. Since this data is sent to the browser via AJAX, it's visible to the user, even if not included in the page. One way to handle this would be the IndividualSearchResult class only containing data visible to everyone, and then an IndividualSearchResultWithIds child class containing the restricted data.
| no-results-label="No results found"> | ||
| <template v-slot:body-cell-photo="props"> | ||
| <q-td :props="props"> | ||
| <q-img :props="props" :src="'https://viper.vetmed.ucdavis.edu/public/utilities/getbase64image.cfm?mothraID=' + props.row.mothraId + '&altphoto=1'" style="width:87px; height:111px" :id="'photo_' + props.row.mothraId"></q-img> |
There was a problem hiding this comment.
There's a static function GetOldViperRootURL() in HttpHelper that could be used to get the url for Viper on the current server, instead of hard coding viper.vetmed.
| createVueApp({ | ||
| data() { | ||
| return { | ||
| userSearch: getItemFromStorage("searchField") ?? "", |
There was a problem hiding this comment.
The key for the storage has to be unique across all of VIPER, so consider prefixing searchField with, e.g. directory or some other prefix.
| methods: { | ||
| findUsers: async function () { | ||
| this.users.urlBase = "search/" + this.userSearch | ||
| if (this.userSearch.length >= 3) { |
There was a problem hiding this comment.
I know I used 3 as the min length for RAPS, but it's been suggested to me that 2 might be better.
| { | ||
| var individuals = await _aaud.AaudUsers | ||
| .Where(u => (u.DisplayFirstName + " " + u.DisplayLastName).Contains(search) | ||
| || (u.MailId != null && u.MailId.Contains(search)) |
There was a problem hiding this comment.
Consider allowing additional ID searches - pidm, spriden id, mothra id, employee id, iam id
| }); | ||
| results.ForEach(r => | ||
| { | ||
| LdapUser l = new LdapService().GetUser(r.LoginId); |
There was a problem hiding this comment.
I'm glad you were able to use this class!
I'm sure there will be additional back-and-forth, but here's the next round of changes: --- [Permission(Allow = "SVMSecure")] is probably a sufficient permission check. As it is now, this restricts the page to a much smaller set of users. Done --- Make sure to remove copy/pasted variables that are not needed in this class Sorry about that; I had left in some of my early code (as I was trying to figure out how all this works!). I removed the variables, but I added back in what seemed necessary to do the permission check. --- We might need a permission check here for the IDs. Since this data is sent to the browser via AJAX, it's visible to the user, even if not included in the page. One way to handle this would be the IndividualSearchResult class only containing data visible to everyone, and then an IndividualSearchResultWithIds child class containing the restricted data. Maybe you can help me think about a better way of doing this. I couldn't quite figure out how to use a generic to be able to switch between the two possible resulting objects. --- There's a static function GetOldViperRootURL() in HttpHelper that could be used to get the url for Viper on the current server, instead of hard coding viper.vetmed. Good tip. Thanks! Its seems like vue isn't very happy with localhost, but maybe adding a local certificate will help with some of the problems I am now getting. --- The key for the storage has to be unique across all of VIPER, so consider prefixing searchField with, e.g. directory or some other prefix. Fixed. I am now using directory_search --- I know I used 3 as the min length for RAPS, but it's been suggested to me that 2 might be better. Good suggestion. Fixed. --- Consider allowing additional ID searches - pidm, spriden id, mothra id, employee id, iam id Done --- I'm glad you were able to use this class! I added phone number to the LDAP lookup. If that is getting too unwieldy, we could pull it out of RAPS and create a generic LDAPlookup class.
JasonRobertFrancis
changed the title
Thanks! I will work on a few of the remaining items (email host, address, student affiliations, alt photo). I will also need to think about the All UCD mode. I wanted to at least start a conversation about a few of my changes, so I thought it would be helpful to show you where I am now. |
This is all pretty messy, but I think I can unwrap the debugging stuff once we figure out the error. The current message at /directory/search/francis/ucd is "DirectoryServicesCOMException: An invalid dn syntax has been specified." I thought the invalid username message I was getting earlier was an indication that at least the username/password was correct, but I am getting the invalid dn error even with [username] and [password]. Every filter string I have tried has returned the same error.
Next step is exploring cards for display instead of the table. I will focus more on making sure all the relevant data is displayed with the next update.
…into feature/directory
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
# Conflicts: # web/Areas/RAPS/Services/LdapService.cs
This reverts commit ed3ce5d.
…es in the Utilities folder. Updating LDAP result properties to match properties from CF directory code.
No description provided.