Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The users should be able to change their affiliation string #6515

Closed
pkiraly opened this issue Jan 14, 2020 · 10 comments
Closed

The users should be able to change their affiliation string #6515

pkiraly opened this issue Jan 14, 2020 · 10 comments
Labels
Component: JSF Involves modifying JSF (Jakarta Server Faces) code, which is being replaced with React. Feature: Account & User Info Type: Feature a feature request User Role: Depositor Creates datasets, uploads data, etc. UX & UI: New React UI Needs enough design work that it should probably go in the new React UI, not JSF

Comments

@pkiraly
Copy link
Member

pkiraly commented Jan 14, 2020

In #6514 there is an explaination that a different type of Shibboleth setup, which does not use DiscoFeed for the affiliation string might set wrong value, and the user can not change it. Moreover: iven if the site administrator change it via an SQL query when the user logs in again, the value is updated.

Dataverse should provide a method which lets the users change their affiliation, and once the affiliation has been manually changed a login mechanism should not overwrite it.

@pdurbin's comment: I'm especially interested in fixing this. OAuth, Builtin, and OIDC users can set their affiliation to whatever they want whenever they want. Should we empower Shibboleth users to edit their affiliation as well?

@pkiraly
Copy link
Member Author

pkiraly commented Jan 16, 2020

Just an investigation update. To enable editing, one method should be added to the src/main/java/edu/harvard/iq/dataverse/authorization/providers/shib/ShibAuthenticationProvider.java file:

@Override
public boolean isUserInfoUpdateAllowed() {
  return true;
};

Maybe it can be turn on and off with a setting:

@EJB
SettingsServiceBean settingsService;

@Override
public boolean isUserInfoUpdateAllowed() {
  String key = SettingsServiceBean.Key.ShibUserInfoUpdateAllowed; // this is a new setting!!!
  String updateAllowed = settingsService.getValueForKey(key);
  return updateAllowed.equals("true");
};

However there is a problem. Shib.init() (src/main/java/edu/harvard/iq/dataverse/Shib.java) has the following lines which overwrites the edited values upon every login:

logger.fine("Updating display info for " + au.getName());
authSvc.updateAuthenticatedUser(au, displayInfo);

So we should track somewhere if the author infor was updated manually. au in the last line refers to authenticateduser table, which has the following columns: {id, affiliation, email, emailconfirmed, firstname, lastname, position, superuser, useridentifier, createdtime, lastlogintime, lastapiusetime}, none of them seems to be proper for keeping track of editing.

@pdurbin Is there any other table from which we can read if a user ever edited its profile? If there is, we can use that. If there is no, we can create a new table, minimally with two columns: user id (id) and a boolean if the user edited its info or not (edited).

Once we have this table when a user edits its profile, this column should set to true. We should modify the last snippet as:

boolean updatable = isUserDisplayInfoUpdateble(au);
if (updatable) {
  logger.fine("Updating display info for " + au.getName());
  authSvc.updateAuthenticatedUser(au, displayInfo);
}
...

private boolean isUserDisplayInfoUpdateble(AuthenticatedUser au) {
  // wrapper of an SQL query which returns true if there is a single row in the table
  // where the user id equals with au's ID,
  // and edited = true
}

We should also modify the method which saves changes to the user's profile (I have to dig into this).

@pkiraly
Copy link
Member Author

pkiraly commented Jan 16, 2020

The profile info change happens at DataverseUserPage.save() (src/main/java/edu/harvard/iq/dataverse/authorization/providers/builtin/DataverseUserPage.java line 374):

AuthenticatedUser savedUser = authenticationService.updateAuthenticatedUser(
  currentUser, userDisplayInfo
);

should be modified as

public String save() {
  ...
  AuthenticatedUser savedUser = authenticationService.updateAuthenticatedUser(
    currentUser, userDisplayInfo
  );
  if (!currentUser.getAffiliation().equals(userDisplayInfo.getAffiliation())) {
    registerAffiliationModification(currentUser);
  }
  ...
}
...
private void registerAffiliationModification(AuthenticatedUser currentUser) {
  // check if the user is already in the affiliation change tracking table
  // if true: set `edited` to `true`
  // if false: insert a new entry with the user ID and  `edited` to `true`
}

@pdurbin
Copy link
Member

pdurbin commented Jan 16, 2020

So we should track somewhere if the author info was updated manually.

Yeah. Right now the rule is that only Shibboleth users have their email, name, affiliation, etc, overwritten every time. If we're going to let Shibboleth users update their name, affiliation, etc (as I think we should), we're going to need to prevent these automatic updates from occurring.

How do you think we should go about this?

@scolapasta
Copy link
Contributor

I've only skimmed this issue, but apologies if I've missed something. But I wanted to make sure to get this out there. The big thing about Shibboleth is that, by making it not editable, we are guaranteed that the user is who they say they are. If I request access to a dataset, that dataset curator knows that I am Gustavo Durand from Harvard University (assuming it trusts that Harvard is a trustworthy idp). If we allows users to make changes, I could spoof and say that I am Phil Durbin from Ohio State and that guarantee is lost.

If we decide to do this, at the very least, we should consider making some other changes in UI/UX that display which shibboleth idp was used (though the e-mail address domain may be enough - I think we show e-mail).

That was the data owner would see that "Phil Durbin" from "Ohio State" was logging in via Harvard idp and at least be suspicious.

Thoughts?

@pkiraly
Copy link
Member Author

pkiraly commented Jan 16, 2020

@scolapasta I should tell, that I am not a Shibboleth expert, so maybe there is a solution with Shibboleth. Right now Dataverse reads affiliation information from the DiscoFeed. In Göttingen our DiscoFeed have a single entry for about 40K users and it is not an organization name ("Service of GWDG" in which GWDG is the IT facility of the campus). So it ends up that every user has wrong affiliation. In #6514 I started to think how to change that. @pdurbin suggested that we should also investigate if it would be possible to change affiliation even for Shibbolet, so this ticket is about this investigation. It would not be turned on by default, only those organisation should turn it on which has a similar situation than ours.

@pkiraly
Copy link
Member Author

pkiraly commented Jan 21, 2020

@scolapasta @pdurbin What if not the edit is enabled, but the overwrite is disabled? This would let the site administrator to update the affiliation of the users by individual request via an SQL command, and prevent that Dataverse automatically updates the affiliation with the wrong general string. Do you agree?

@scolapasta
Copy link
Contributor

Well, if we do that, it would likely make sense to allow the superusers to do this via UI or API). Would the affiliation be the same for all users from one idp or is the idea that there are multiple?

@poikilotherm
Copy link
Contributor

@pdurbin made me head over here to leave a comment. In #6679 he asked about making email ineditable, which is indeed a thing for non-Shib users (IIRC it's already disabled for you folks).

As @mheppler kinda highjacked #6676 for proposing more general approaches, I do the same here 😉 👋 : maybe it's time to think about a more general solution and make it configurable per provider what you are allowed to edit at first login and in your user details page?

As always most of the time: willing to help here. Kinda depending on #6694 about refactoring that JSON configuration schema.

@pdurbin
Copy link
Member

pdurbin commented Oct 14, 2022

In Göttingen our DiscoFeed have a single entry for about 40K users and it is not an organization name ("Service of GWDG" in which GWDG is the IT facility of the campus). So it ends up that every user has wrong affiliation.

@pkiraly in practice you're now making use of this PR you made, right?

That is, this :ShibAffiliationAttribute setting you added: https://guides.dataverse.org/en/5.12/installation/config.html#shibaffiliationattribute

Are you still interested in letting users change their affiliation string? Thanks.

@pdurbin pdurbin added Feature: Account & User Info Status: Still Interested? User Role: Depositor Creates datasets, uploads data, etc. Component: JSF Involves modifying JSF (Jakarta Server Faces) code, which is being replaced with React. labels Oct 14, 2022
@pdurbin pdurbin added Type: Feature a feature request UX & UI: New React UI Needs enough design work that it should probably go in the new React UI, not JSF labels Oct 9, 2023
@cmbz
Copy link

cmbz commented Sep 30, 2024

2024/09/30: We are closing the issue. @pkiraly please let us know if the existing functionality doesn't meet your needs.

@cmbz cmbz closed this as not planned Won't fix, can't repro, duplicate, stale Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: JSF Involves modifying JSF (Jakarta Server Faces) code, which is being replaced with React. Feature: Account & User Info Type: Feature a feature request User Role: Depositor Creates datasets, uploads data, etc. UX & UI: New React UI Needs enough design work that it should probably go in the new React UI, not JSF
Projects
None yet
Development

No branches or pull requests

6 participants