NCEAS / metacat

Data repository software that helps researchers preserve, share, and discover data
https://knb.ecoinformatics.org/software/metacat
GNU General Public License v2.0
26 stars 12 forks source link

Log in call in old metacat api #1694

Closed taojing2002 closed 6 months ago

taojing2002 commented 1 year ago

I have disabled all old Metacat API calls (actions) except the log-in method since currently Metacat admin pages need the method. We can have those options to fix the issue:

  1. Use the DataONE log-in (like the one in MetacatUI) in the Metacat admin servlet. It is an overhaul and can consolidate the ldap administrator and dataone administrator, which confuses users a lot. Of course, it requires lot of work.
  2. Keep the log-in method in the old Metacat API. We don't need to do anything. But it contradicts our goal to eliminate the old Metacat API.
  3. Move the log-in method to the metacat admin servlet. This is a point between two above approaches, which requires minimum changes.
artntek commented 1 year ago

I think option 3 is a good, iterative step in the right direction, and we can then do option 1 later, maybe after the v3.0.0 release?

taojing2002 commented 1 year ago

Yeah. Option 3 sounds good to me as well.

mbjones commented 11 months ago

After our call discussing this, I think we decided Option 1 to use DataONE auth APIs is the best approach. Does that sounds right to you @taojing2002 ?

taojing2002 commented 11 months ago

Yes, we decided to use option 1 and Dou is working on the issue. @doulikecookiedough

artntek commented 7 months ago

Also need to change auth.administrators properties list to be semicolon-delimited instead of colon-delimited, since it now includes orcids with colons

doulikecookiedough commented 6 months ago

Reminder for me or @artntek to resolve:

artntek commented 6 months ago

Reminder for me or @artntek to resolve:

  • AuthAdmin does not persist properties in memory [...]
  • The auth-configuration.jsp page [...] restore the previous layout

Thanks for making a note of these @doulikecookiedough! Since you're closest to that code, would you mind reverting the changes in those 2 files and pushing them to feature-1694-dou-develop, so I can pick them up? (I have not touched either file so far in my branch)

Thanks!

doulikecookiedough commented 6 months ago

@artntek I've pushed the updates discussed above to dou-develop. Note - I was unsuccessful in including http://orcid.org/ as part of the admin user string. This is due to the : present - our existing code splits the single string (ex. http://orcid.org/0000-0000-0000-000X into two users when being iterated over. I am not sure how to best handle it at the moment, but did not want to leave dou-develop to be in a broken state so I pushed with the current fixes.

To expand a bit further on this issue, when MN node checks for admin privileges, even if the Metacat admin ORCID user is valid, it will be considered not due to the omitted http://orcid.org/. Either the method to check needs to be updated, or we should figure out how to ensure that http://orcid.org/0000-0000-0000-000X is read as one string/admin user.

artntek commented 6 months ago

Note the PR #1824 is focused on getting the admin login working. There is some subsequent cleanup that should be done, once it is merged, to get rid of old code that is no longer used. I deferred this to a separate PR, to increase speed and reduce risk and reviewer burden.

Cleanup reminders:

artntek commented 6 months ago

moving these to a new issue #1846, so they can be done in following milestone, and we can close this issue: