Skip to content

[ENG-10586] BE: Ensure only one ORCID ID is availalble in the UserModel.external_identity attribute#11661

Open
mkovalua wants to merge 4 commits intoCenterForOpenScience:feature/orcid-integrationfrom
mkovalua:feature/ENG--10589
Open

[ENG-10586] BE: Ensure only one ORCID ID is availalble in the UserModel.external_identity attribute#11661
mkovalua wants to merge 4 commits intoCenterForOpenScience:feature/orcid-integrationfrom
mkovalua:feature/ENG--10589

Conversation

@mkovalua
Copy link
Copy Markdown
Contributor

@mkovalua mkovalua commented Mar 26, 2026

Ticket

Purpose

If a user has an existing ORCID ID, and then associates a new ORCID ID, the existing ORCID ID should be removed/overwritten

Changes

Side Effects

  1. The was a commit message (user can add more than one external id, fix the overwrite issue), hope the orcid related changings do not break some logic
image

also maybe someone knows it better:

not confident whether we need to deal somehow with already existing in database values

not confident how it is /was possible to create several records for ORCID

maybe some backend workflow also will be needed for the stuff

QE Notes

CE Notes

Documentation

…D, the existing ORCID ID should be removed/overwritten
Copy link
Copy Markdown
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable fix for now, but I will have @cslzchen weigh in on this, as he would have a bit more knowledge on this workflow.

Copy link
Copy Markdown
Collaborator

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised that we have two set of the same/similar code ... one in api v1 and the other in api v2. And there seems to be slight differences. Please double check which is the actual code being used.

Usually if we have v2, it means we are using v2. But it may not be the case for ORCiD. Could the V2 could be some unfinished work from previous projects that never worked or tested at all?

This needs to be wider discussion, likely outside the project scope: if v1 is the one being used and has been fully tested and if v2 is untested/unfinished "future" work, should we maintain both code?


Please add/update unit tests.

external_identity[external_id_provider][external_id] = 'LINK'
if external_id_provider in user.external_identity:
user.external_identity[external_id_provider].update(external_identity[external_id_provider])
if external_id_provider == 'orcid':
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is 'orcid'.

In addition, we should be able to find this key in settings (or parse it out of settings).

Copy link
Copy Markdown
Contributor Author

@mkovalua mkovalua Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cslzchen

  1. There is 'orcid' key used for tests so think it is ok key to use
image

also there is 'orcid' on test run

api_tests/users/views/test_user_external_login.py
image

not confident where should be so have added to both.
As git log tells there are recent code changings for both files

image image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, regarding to what you see in tests. We sometimes use literal string in test server for testing purpose, which is ok. Another possibility is unit tests were not written well, it uses "orcid" within the scope of the test but on servers, it's "ORCID". That's why I doubted using literal string in your actual code wouldn't work.

Here is what I found in website/settings/defaults.py.

# External Identity Provider
EXTERNAL_IDENTITY_PROFILE = {
    'OrcidProfile': 'ORCID',
}

And in framework/auth/cas.py, here is an example of how we use it.

provider = settings.EXTERNAL_IDENTITY_PROFILE[profile_name]

return {
    'provider': provider,
    'id': technical_id,
}

I also checked the database after logging in via ORCiD, it's "ORCID" instead of "orcid", which confirms what mentioned above.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second, regarding the changes, I think the git history you mentioned are on the file. You need to take a look at history for related code / lines.

Given we have both API V1 and V2 doing the same thing, the best way to verify is to go on one of our servers, go through the ORCiD process and see what request is made. Or take a look at your local code legacy flask view/html, on which endpoint the code uses when submitting the form.

external_identity[external_id_provider][external_id] = 'LINK'
if external_id_provider in user.external_identity:
user.external_identity[external_id_provider].update(external_identity[external_id_provider])
if external_id_provider == 'orcid':
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@mkovalua mkovalua requested a review from cslzchen March 27, 2026 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants