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.

Sign up for GitHub

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

Jump to bottom

cern: replaces 'PersonID' to 'uidNumber' #132

Merged
merged 1 commit into from
Aug 1, 2017
Merged

cern: replaces 'PersonID' to 'uidNumber' #132

merged 1 commit into from
Aug 1, 2017

Conversation

pamfilos
Copy link
Member

@pamfilos pamfilos requested a review from egabancho May 18, 2017 13:26
@tiborsimko
Copy link
Member

@egabancho Can you check whether this change would work for CDS? Or should we make it configurable for each service?

egabancho
egabancho approved these changes Jun 21, 2017
View reviewed changes
Copy link
Member

@egabancho egabancho left a comment

Choose a reason for hiding this comment

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

I would also add a small comment in the body of the commit explaining why the change is needed.

@pamfilos @egabancho
cern: replaces PersonID to uidNumber
6711c15 
* CERN's account management allows the same person to have several
  accounts, and consequently two accounts can have the same `PersonID`.
  Using `uidNumber` as ID instead solves the problem. (closes inveniosoftware#112)

Signed-off-by: Pamfilos Fokianos <pamfilosf@gmail.com>
@lnielsen
Copy link
Member

  • Replace field with config variable that can be set to either PersonID or uidNumber
  • Document the difference between the two attributes.

@egabancho
Copy link
Member

Replace field with config variable that can be set to either PersonID or uidNumber

Do we really need this? Using PersonID might leed to accounts having wrong access rights, among other small problems.

@lnielsen
Copy link
Member

We just had a discussion about this in the Sprint kick-off. As I understood on @drjova you needed to use PersonID on CDS (as it represents the same user over multiple accounts), and CAP needs uidNumber.

If that's not the case, then I think we can just merge?

@egabancho
Copy link
Member

This was de initial idea, but it might cause us a few headaches, so, uidNumber for everyone!

Once tests are passing we can :shipit:

@hjhsalo hjhsalo removed their assignment Jul 31, 2017
@lnielsen lnielsen merged commit eb6dc75 into inveniosoftware:master Aug 1, 2017
@ntarocco ntarocco mentioned this pull request Oct 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egabancho egabancho egabancho approved these changes

Assignees

@hjhsalo hjhsalo

Labels
Service: CAP Service: CDS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cern: PersonID is not an unique identifier
5 participants
@pamfilos @tiborsimko @lnielsen @egabancho @hjhsalo