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: allow users based on 'IdentityClass' #181

Merged
merged 1 commit into from
Jan 19, 2019
Merged

cern: allow users based on 'IdentityClass' #181

merged 1 commit into from
Jan 19, 2019

Conversation

zzacharo
Copy link
Member

@zzacharo zzacharo commented Dec 3, 2018

closes #178 .

@zzacharo
Copy link
Member Author

zzacharo commented Dec 3, 2018 •
edited
Loading

Lightweight accounts do not have a uidNumber so we get a 500 error. I use instead the PersonID field as they have a negative integer as an id in that case which can be used for our purposes. Do you see something that can go wrong with this solution?

@lnielsen
Copy link
Member

lnielsen commented Dec 4, 2018

The external id is treated as a string so probably not an issue as long as the two doesn't conflict. Did you try it out?

@lnielsen lnielsen added this to Backlog in Invenio Sprint Dec 3 - Dec 14 2018 via automation Dec 4, 2018
@ntarocco
Copy link
Contributor

ntarocco commented Dec 4, 2018 •
edited
Loading

@lnielsen it appears that all of them are treated as string, no? I would go for string to be safe, and then let who is using that value do the parsing and eventually fail in a controlled manner.

@zzacharo I would also add, if the @lnielsen agrees, a new config variable for cern contrib, something like OAUTHCLIENT_CERN_ALLOWED_IDENTITY_CLASS = ['CERN Registered', 'CERN Shared'] (primary and secondary accounts, and probably also add service accounts by default, but I don't know the identity class for that) and fail login if not in the list.
With this, we can block lightweight account if needed (probably my most of the websites).
Probably you need to throw an exception in account_info, to be checked.

@lnielsen
Copy link
Member

What was the conclusion in the end - that we close or integrate this PR?

@ntarocco
Copy link
Contributor

There was no further discussion I guess... Integrate? @zzacharo?

@zzacharo zzacharo changed the title cern: use personID for lightweight accounts cern: allow users based on 'IdentityClass' Dec 18, 2018
@zzacharo
Copy link
Member Author

@lnielsen @ntarocco @pamfilos I upgraded the PR with a new solution. We enable the cern accounts that belong to a specific IdentityClass so in that way be default we disable the lightweight accounts. Please review again.

@zzacharo zzacharo force-pushed the cern-lightweight-accounts branch 2 times, most recently from e84c133 to 0a4ad05 Compare December 18, 2018 13:24
lnielsen
lnielsen reviewed Dec 18, 2018
View reviewed changes
invenio_oauthclient/errors.py Outdated
@@ -71,3 +71,7 @@ def __init__(self, message, remote, response):
super(OAuthClientError, self).__init__(
self.description or message, remote, response
)


class OauthCernRejectedAccountError(OAuthResponseError):
Copy link
Member

Choose a reason for hiding this comment

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

OAuthCERNRejectedAccountError (sorry to be annoying with the naming)

lnielsen
lnielsen reviewed Dec 18, 2018
View reviewed changes
invenio_oauthclient/handlers.py Outdated
@@ -227,6 +228,11 @@ def inner(*args, **kwargs):
return oauth2_handle_error(
e.remote, e.response, e.code, e.uri, e.description
)
except OauthCernRejectedAccountError as e:
current_app.logger.warning(e.message, exc_info=True)
flash(_('Cern account not allowed.'),
Copy link
Member

Choose a reason for hiding this comment

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

Cern -> CERN

lnielsen
lnielsen approved these changes Dec 18, 2018
View reviewed changes
Copy link
Member

@lnielsen lnielsen left a comment

Choose a reason for hiding this comment

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

Looks fine for me, just the naming. However, @pamfilos or @tiborsimko should have a look at it since I'm not familiar with the CERN OAuth.

@zzacharo
Copy link
Member Author

@tiborsimko can you please check this PR so we can merge it if you don't see any problem?

@ntarocco ntarocco merged commit fdfd79e into inveniosoftware:master Jan 19, 2019
Invenio Sprint Dec 3 - Dec 14 2018 automation moved this from Backlog to Done Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lnielsen lnielsen lnielsen approved these changes

@ntarocco ntarocco ntarocco approved these changes

@pamfilos pamfilos Awaiting requested review from pamfilos

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Invenio Sprint Dec 3 - Dec 14 2018
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cern: lightweight account throws 500
3 participants
@zzacharo @lnielsen @ntarocco