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

WIP: Persistence layer migrations for MFA support #419 #452

Closed
wants to merge 19 commits into from
Closed

WIP: Persistence layer migrations for MFA support #419 #452

wants to merge 19 commits into from

Conversation

sam-glendenning
Copy link
Collaborator

@sam-glendenning sam-glendenning commented Jan 24, 2022 •
edited
Loading

This PR contains the necessary database migrations and Java model changes to support the inclusion of multi-factor authentication into the IAM. Broadly speaking, the changes encompass:

  • new tables for authenticator app secrets and their respective recovery codes (iam_totp_mfa and iam_totp_recovery_code)
  • an additional iam_authority called ROLE_PRE_AUTHENTICATED which is used by Spring to grant a user access to webpages where they can continue the authentication process after signing in with their basic login credentials but does not grant them full access to the IAM. This was necessary to simulate the "two-step" login process associated with multi-factor authentication.

This PR is related to #441 which focuses on the higher-level implementation of MFA in the IAM.

Closes #419

andreaceccanti and others added 9 commits September 11, 2021 15:49
@andreaceccanti
Merge pull request indigo-iam#416 from indigo-iam/release-v1.7.1
0c674fb 
Release v1.7.1
Merge branch 'iam-spring-update-oct-2021' of git://github.com/indigo-…
dccc0b6 
…iam/iam into iam-spring-update-oct-2021
Merge branch 'iam-spring-update-oct-2021' of git://github.com/indigo-…
47baf5f 
…iam/iam into iam-spring-update-oct-2021
Merge branch 'indigo-iam:iam-spring-update-oct-2021' into iam-spring-…
f0de8a8 
…update-oct-2021
Adding SQL migration files for MFA
838df00 
This includes a new table for multi-factor secrets, a new table for recovery codes associated with a multi-factor secret and changes to the iam_account table that link a multi-factor secret to an account.

In a separate migration, we are also adding a new authority called ROLE_PRE_AUTHENTICATED. This is assigned to a user if they have MFA enabled on their account and they complete one factor of authentication . The role is used to grant the user access only to pages where they can authenticate through an additional factor of authentication.
Adding new model files and changes to IamAccount model
d612570 
IamAccount now has a nullable IamTotpMfa object. The IamTotpMfa entity represents the iam_totp_mfa table which has non-nullable links to a set of IamTotpRecoveryCodes. The IamTotpRecoveryCode entity represents the iam_totp_recovery_code table.
Removing unneeded method to regenerate recovery codes
6e20058 
This method is present in the IamTotpMfa class itself, when this can be handled by the IamAccount service instead. This then calls the setRecoveryCodes method on the IamTotpMfa object in question.
Adding correct licence headers
041665f
Adding creation time and last update time to IamTotpMfa
72973b9 
This helps to keep track of changes to the MFA secret object, particularly changes to its associated recovery codes.
@sam-glendenning sam-glendenning changed the base branch from develop to iam-spring-update-oct-2021 January 24, 2022 13:17
Sam Glendenning added 5 commits January 24, 2022 13:57
Adding missing comma to SQL file
bb1fb70
Merge branch 'indigo-iam:iam-spring-update-oct-2021' into iam-spring-…
9f39720 
…update-oct-2021
Merge branch 'iam-spring-update-oct-2021' into issue-419-mfa-db-migra…
74d9842 
…tions
Fixing migration for adding pre_authenticated authority
c1e665b 
Specifying an ID claimed there was already an entry at the specified ID. Given the table uses an auto_increment for this field, there isn't a need to specify an ID so removing it from the INSERT statement.
Fixing license headers
7f9a9cc
@sam-glendenning
Copy link
Collaborator Author

sam-glendenning commented Jan 25, 2022 •
edited
Loading

At this point, I believe the only errors in this branch are the ones inherited from iam-spring-update-oct-2021, as this branch has had its commits merged across. Not much more I can do until those are fixed.

@sam-glendenning sam-glendenning marked this pull request as ready for review February 10, 2022 13:29
@enricovianello enricovianello changed the base branch from iam-spring-update-oct-2021 to v1.8.0 February 14, 2022 13:09
enricovianello
enricovianello reviewed Feb 14, 2022
View reviewed changes
iam-persistence/src/main/resources/db/migration/mysql/V71___add_pre_authenticed_authority.sql Outdated
@@ -0,0 +1,2 @@
INSERT INTO iam_authority(AUTH) VALUES
Copy link
Member

Choose a reason for hiding this comment

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

rename file to pre_authenticated_authority

enricovianello
enricovianello reviewed Feb 14, 2022
View reviewed changes
iam-persistence/src/main/resources/db/migration/mysql/V70___totp_mfa.sql Outdated
@@ -0,0 +1,15 @@
-- TOTP MFA secrets

CREATE TABLE iam_totp_mfa (ID BIGINT AUTO_INCREMENT NOT NULL, active TINYINT(1) default 0 NOT NULL, secret VARCHAR(256) NOT NULL, creation_time DATETIME NOT NULL, last_update_time DATETIME NOT NULL, ACCOUNT_ID BIGINT, PRIMARY KEY (ID));
Copy link
Member

Choose a reason for hiding this comment

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

use 255

enricovianello
enricovianello reviewed Feb 14, 2022
View reviewed changes
iam-persistence/src/main/resources/db/migration/h2/V70___totp_mfa.sql Outdated

CREATE TABLE iam_totp_recovery_code (ID BIGINT IDENTITY NOT NULL, code VARCHAR(255) NOT NULL, totp_mfa_id BIGINT NOT NULL, PRIMARY KEY (ID));

ALTER TABLE iam_account ADD COLUMN totp_mfa_id BIGINT;
Copy link
Member

Choose a reason for hiding this comment

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

avoid adding an extra column to iam_account

@sam-glendenning
Copy link
Collaborator Author

Currently working on removing changes to iam_account table and creating a repository for iam_totp_mfa. I'll need to test this and make sure it works properly as well. Was held back by SQL problems today so will finish this tomorrow.

Sam Glendenning added 2 commits February 15, 2022 15:27
Removing changes to IamAccount, adding IamTotpMfaRepository
804a4b9 
Repository contains one method, for finding TotpMfa based on IamAccount ID
Fixing test data, updating repository method names
77083be 
Test data made use of account UUID instead of correctly using account ID.

IamTotpMfaRepository now uses more logical method names to help with querying database at runtime
@sam-glendenning
Copy link
Collaborator Author

@enricovianello I think this is ready for review again with the suggested changes added. The only failing tests are the ones inherited from Andrea's spring boot upgrade branch.

The new repository for totp secrets contains only one method, for fetching a totp secret by the IAM account ID it relates to. I couldn't think of any further methods that it needed but let me know if you can think of anything.

Squashed commit of the following:
2c1efbb 
commit 8476b84
Author: rmiccoli <[email protected]>
Date:   Wed Feb 16 18:36:44 2022 +0100

    Fix license

commit e08db5f
Author: rmiccoli <[email protected]>
Date:   Wed Feb 16 18:18:04 2022 +0100

    Fix error message about username

commit eeed9fc
Author: rmiccoli <[email protected]>
Date:   Wed Feb 16 16:54:30 2022 +0100

    Fix IAM version in pom files

commit f559b51
Author: rmiccoli <[email protected]>
Date:   Wed Feb 16 16:37:11 2022 +0100

    Fix errors in database file

commit f2fa805
Author: rmiccoli <[email protected]>
Date:   Wed Feb 16 16:25:18 2022 +0100

    Fix deprecated classes

commit 92fbb6f
Author: Andrea Ceccanti <[email protected]>
Date:   Tue Nov 23 11:36:08 2021 +0100

    Remove @transactional

    Which leads to strange behaviour for an integration test...

commit 6cba0ab
Author: rmiccoli <[email protected]>
Date:   Wed Nov 17 14:43:21 2021 +0100

    WIP: Investigate test problem

commit 6bde519
Author: rmiccoli <[email protected]>
Date:   Fri Nov 12 11:25:27 2021 +0100

    WIP: Filter requested scopes according to the scope policy

commit 50d4dda
Author: rmiccoli <[email protected]>
Date:   Wed Nov 3 15:36:45 2021 +0100

    Further corrections

commit a90bcec
Author: rmiccoli <[email protected]>
Date:   Wed Oct 27 15:59:52 2021 +0200

    Cosmetic improvements

commit db2b39d
Author: rmiccoli <[email protected]>
Date:   Tue Oct 26 18:41:40 2021 +0200

    Scopes error fixed

commit 51c70c6
Author: rmiccoli <[email protected]>
Date:   Tue Oct 26 16:49:25 2021 +0200

    Cosmetic fix

commit d43606a
Author: rmiccoli <[email protected]>
Date:   Mon Oct 25 18:34:35 2021 +0200

    New consent page

commit 0c69539
Author: rmiccoli <[email protected]>
Date:   Tue Sep 28 15:57:54 2021 +0200

    WIP: Added client application logo

commit 236bfea
Author: rmiccoli <[email protected]>
Date:   Fri Sep 24 15:45:57 2021 +0200

    WIP: new consent page template

commit d50abc3
Author: rmiccoli <[email protected]>
Date:   Tue Sep 21 14:36:44 2021 +0200

    Bumped version back to 1.8.0-SNAPSHOT

commit 9123834
Author: rmiccoli <[email protected]>
Date:   Tue Sep 21 14:07:01 2021 +0200

    wip

commit 97046c5
Author: rmiccoli <[email protected]>
Date:   Thu Sep 16 14:18:43 2021 +0200

    Fix licenses

commit 6875bde
Author: rmiccoli <[email protected]>
Date:   Thu Sep 16 13:51:10 2021 +0200

    Step 1: Move and test the consent page

commit ae99690
Merge: 971df59 4b9560c
Author: Enrico Vianello <[email protected]>
Date:   Wed Feb 16 10:25:46 2022 +0100

    Merge remote-tracking branch 'origin/issue-391-show-group-labels-in-account-home-page' into v1.8.0

commit 971df59
Merge: cbafc73 ce50926
Author: Enrico Vianello <[email protected]>
Date:   Wed Feb 16 10:15:28 2022 +0100

    Merge remote-tracking branch 'origin/fix-user-details-update-button' into v1.8.0

commit cbafc73
Merge: 4efd231 256714a
Author: Enrico Vianello <[email protected]>
Date:   Wed Feb 16 10:14:59 2022 +0100

    Merge remote-tracking branch 'origin/restore-tokens-component-in-IAM-dashboard' into v1.8.0

commit 4efd231
Author: rmiccoli <[email protected]>
Date:   Tue Feb 15 17:00:15 2022 +0100

    Changed the minimum length of the username field

    to two characters

commit 256714a
Author: rmiccoli <[email protected]>
Date:   Tue Feb 15 16:29:26 2022 +0100

    Fix failed tests

commit 744bf3b
Author: rmiccoli <[email protected]>
Date:   Mon Feb 14 16:04:49 2022 +0100

    Add compose file for linux users

commit aef6ef3
Merge: d968d47 d2c977d
Author: Enrico Vianello <[email protected]>
Date:   Mon Feb 14 12:16:24 2022 +0100

    Merge branch 'develop' into merge_before

commit d968d47
Merge: 04300ef edb6d2f
Author: Enrico Vianello <[email protected]>
Date:   Fri Feb 11 17:44:30 2022 +0100

    Merge commit 'edb6d2f2951841f67fe454eb94cb25f150b4f9ab' into merge_before

commit bfc3bfc
Author: rmiccoli <[email protected]>
Date:   Wed Feb 2 16:56:39 2022 +0100

    Added client name in /iam/api/access-tokens

commit 3a7d27a
Author: rmiccoli <[email protected]>
Date:   Wed Feb 2 16:33:53 2022 +0100

    Added tokens component

commit ce50926
Author: rmiccoli <[email protected]>
Date:   Mon Dec 27 19:18:13 2021 +0100

    Solved update button bug

commit 9a71c84
Author: rmiccoli <[email protected]>
Date:   Mon Dec 20 17:37:43 2021 +0100

    WIP: added green tick for the name when valid

commit ceeb6b6
Author: rmiccoli <[email protected]>
Date:   Tue Dec 14 14:23:08 2021 +0100

    Update button bug fixed

commit d2c977d
Author: Andrea Ceccanti <[email protected]>
Date:   Fri Dec 3 12:09:43 2021 +0100

    Fix missing license issues

commit cead8d0
Author: Andrea Ceccanti <[email protected]>
Date:   Fri Dec 3 11:32:59 2021 +0100

    Upgrade to flyway 4.2.0

    As this will manage the upgrade to later versions more gracefully

commit e5f5247
Author: Andrea Ceccanti <[email protected]>
Date:   Thu Dec 2 16:57:40 2021 +0100

    wip

commit 9157be9
Author: Andrea Ceccanti <[email protected]>
Date:   Fri Nov 19 09:00:58 2021 +0100

    Added voms-test profile for easier voms-aa testing

commit 71c1b00
Author: Andrea Ceccanti <[email protected]>
Date:   Thu Nov 18 08:23:46 2021 +0100

    wip

commit edb6d2f
Author: Andrea Ceccanti <[email protected]>
Date:   Thu Nov 18 07:39:56 2021 +0100

    Fail early if wrong version of java is detected

    And enforce that Java 8 is used.

    https://maven.apache.org/enforcer/enforcer-rules/versionRanges.html

commit 55473e4
Merge: e9e5408 37a2df2
Author: Andrea Ceccanti <[email protected]>
Date:   Mon Nov 15 14:31:28 2021 +0100

    Merge pull request indigo-iam#438 from indigo-iam/nginx-local-dev-linux

    Add compose file for linux users

commit 37a2df2
Author: Federica Agostini <[email protected]>
Date:   Sun Oct 31 23:25:39 2021 +0100

    Add compose file for linux users

    which maps host.docker.internal into host-gateway.
    It prevents 'host not found in upstream "host.docker.internal" in /etc/nginx/conf.d/default.conf:24' error.
    Solution for this error found in

    https://stackoverflow.com/questions/48546124/what-is-linux-equivalent-of-host-docker-internal/61001152

commit 4b9560c
Merge: 15f7f9f 0014a27
Author: Andrea Ceccanti <[email protected]>
Date:   Wed Sep 1 06:00:17 2021 +0200

    Merge branch 'develop' into issue-391-show-group-labels-in-account-home-page

commit 15f7f9f
Merge: 7515c14 b2d5805
Author: Andrea Ceccanti <[email protected]>
Date:   Tue Aug 10 12:34:24 2021 +0200

    Merge pull request indigo-iam#395 from rmiccoli/issue-391-show-group-labels-in-account-home-page

    WIP: Issue 391 show group labels in account home page

commit b2d5805
Author: rmiccoli <[email protected]>
Date:   Fri Jul 23 16:38:24 2021 +0200

    Changes fixed

commit f88845c
Author: rmiccoli <[email protected]>
Date:   Mon Jul 19 17:09:20 2021 +0200

    Cosmetic fix

commit 7be2d03
Author: rmiccoli <[email protected]>
Date:   Mon Jul 19 16:58:47 2021 +0200

    Added group labels by using CSS classes

commit 62fc2f5
Author: rmiccoli <[email protected]>
Date:   Fri Jul 16 11:43:10 2021 +0200

    Added group labels in the account home page

commit 1bebbda
Author: rmiccoli <[email protected]>
Date:   Wed Jul 7 17:55:59 2021 +0200

    WIP add label to groups

commit 7515c14
Author: Andrea Ceccanti <[email protected]>
Date:   Tue Jun 29 14:36:36 2021 +0200

    Bootstrap development for issue 391
@sam-glendenning sam-glendenning mentioned this pull request Feb 18, 2022
Merged
@sam-glendenning
Copy link
Collaborator Author

This branch has got itself confused following the merge of v1.8.0's commits. There are now dozens more files that are registered as changed but are actually merge conflicts with v1.8.0. This likely happened because this branch was originally based off of iam-spring-update-oct-2021 and v.1.8.0 was also based off of iam-spring-update-oct-2021 but has since changed. This PR assumes that the changes made on v1.8.0 will be overwritten by the "changes" made on here.

Rather than fix this entire thing, I thought it would be easier to close this PR and make a new one for the relatively small number of changes this PR is actually trying to make. You can find this new PR here - #460

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enricovianello enricovianello Awaiting requested review from enricovianello

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sam-glendenning @enricovianello @andreaceccanti