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

6936 Implement email domain based groups #6974

Merged
merged 18 commits into from
Jul 1, 2020

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Jun 10, 2020

What this PR does / why we need it:
Next to explicit groups, IP groups and Shibboleth groups, there is another valuable source of information to which association a user belongs to: a verified email.

Based on the domain part of the email, it's very easy to create groups based on affiliations. This might be handy for installations like the one from Leuven, where different universities share one instance (multi-tenancy). Our use case @ FZJ is to be able to open up for collaboration while automatically granting some rights to staff.

As this is independent from Shibboleth, this might be good for other instances like WZB (ping @RightInTwo), that create users via API.

Which issue(s) this PR closes:

Closes #6936

Special notes for your reviewer:

This is not only introducing a new group provider, but also refactors some other places in the codebase. Some noteable places:

  • Add a simpler verification check function for email addresses and make use of it. It's much easier to understand now and easier to extend (looking at Refactor authentication provider JSON configuration #6694).
  • Extend the usage of ExceptionMappers in REST API endpoints. There are a lot of places with lots of boilerplate and catch all like try-catch code. This makes the code more verbose and violates DRY principle. Obviously I didn't touch too much of other code to keep the scope of this PR as small as possible. (But didn't want to encourage the use of catch-alls any further...)

Suggestions on how to test this:

  • Deploy, but don't yet publish the root dataverse.
  • Create a new mail domain group
  • Add the group as member or similar to the root dataverse
  • Create a user with a group-matching mail address
  • Verify user email
  • Get access to the root dataverse
  • Delete the permission
  • Verify permission is gone

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Nope.

Is there a release notes update needed for this change?:

It should be mentioned this is a new option to create groups of people.

Additional documentation:

Nada.

…tructure. Still lacking tests, API endpoints etc. IQSS#6936
…-all for exceptions, support JsonParseExceptions and JPA ConstraintViolationExceptions. Relates to IQSS#3423 and IQSS#6936.
@poikilotherm poikilotherm added Type: Feature a feature request Feature: Permissions Feature: Account & User Info Feature: Admin Guide User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh labels Jun 10, 2020
@poikilotherm poikilotherm self-assigned this Jun 10, 2020
@coveralls
Copy link

coveralls commented Jun 10, 2020

Coverage Status

Coverage increased (+0.04%) to 19.63% when pulling 76ae927 on poikilotherm:6936-domaingroups into 75742b9 on IQSS:develop.

@poikilotherm poikilotherm marked this pull request as ready for review June 15, 2020 16:17
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Jun 15, 2020

This is done I think. Can somebody please do a review? Thx!
BTW it works already on our production instance 😄

@landreev
Copy link
Contributor

This is done I think. Can somebody like do a review? Thx!
BTW it works already on our production instance 😄

Will do, thank you!

@landreev landreev self-requested a review June 15, 2020 16:23
@@ -546,6 +546,7 @@ public void sendConfirmEmail() {

/**
* Determines whether the button to send a verification email appears on user page
* TODO: cant this be refactored to use confirmEmailService.hasVerifiedEmail(currentUser) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Somebody else here has pointed out that there is a connection with issue #3438; in a sense that, if we rely on users having verified emails for the purposes of group membership, not being able to verify it becomes more of an issue.
Since you have been using these email groups in production, have you done anything to address #3438? (one possible solutions appear to be to just always show this button below - ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I didn't. I was going to lean on auth provider support for this. Is this still an issue? It's 4 years old and a lot happened since then. If we encounter these problems again, I could take a look at this...

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that it has never been fixed precisely because it was not much of a serious issue for anyone (I didn't have any recollection of it). And form what I'm reading, that condition only happens if a user tries to refresh the page at the wrong time during the verification... The fact that you haven't heard many complaints about it is somewhat reassuring.
We'll address #3438, now that we have an extra reason to do so. I asked in part because I was wondering if the refactoring you suggested in the comment above might by itself solve it - but no, I don't think so.

@djbrooke djbrooke assigned landreev and unassigned poikilotherm Jun 16, 2020
@kcondon kcondon assigned kcondon and unassigned landreev Jun 22, 2020
@kcondon kcondon merged commit c79ce40 into IQSS:develop Jul 1, 2020
@poikilotherm
Copy link
Contributor Author

Yeeeeehaw! 🤠 Thanks for merging @kcondon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Account & User Info Feature: Admin Guide Feature: Permissions Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As an administrator, I would like to create groups of people based on their email address domain
5 participants