[ID-1117] Add fence_account_key and distributed_lock tables to db #174

samanehsan · 2024-02-28T21:47:37Z

Jira: https://broadworkbench.atlassian.net/browse/ID-1117

What:

Add a new fence_account_key table that is equivalent to the one in Bond and a distributed_lock table to store the lock information about fence tokens (or eventually passports).

Why:
The Identiteam is moving Bond's functionality to authenticate with fence account providers into ECM, so ECM needs to store information about fence account keys.

How:

Add a new changeset to the liquibase change log.
Added corresponding model and DAO classes

Testing:
I ran the ECM server locally and confirmed that the new table was created.

This will be used for Bond providers

tlangs · 2024-02-29T21:18:07Z

service/src/main/java/bio/terra/externalcreds/models/DistributedLock.java

+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface DistributedLock {


Suggested change

public interface DistributedLock {

public interface DistributedLock extends WithDistributedLock {

This, along with ./gradlew assemble, will let you uncomment the builder.

Oh! I had to add extends WithDistributedLock first and then run ./gradlew assemble in order to generate that interface

Hmm I'm still getting an error about the builder being final 🤔

Oh ./gradlew assemble fixed the error!

tlangs · 2024-02-29T21:18:16Z

service/src/main/java/bio/terra/externalcreds/models/FenceAccountKey.java

+import org.immutables.value.Value;
+
+@Value.Immutable
+public interface FenceAccountKey {


Same as above

samanehsan · 2024-02-29T22:09:53Z

...in/resources/db/changelog/changesets/20240228_add_fence_account_key_and_dist_lock_table.yaml

+      author: sehsan
+      changes:
+        - createTable:
+            tableName: fence_account_key


I think we also need a foreign key to the user's linked_account record, so that we can retrieve a user's fence account key by user_id and provider (similar to ga4gh_passport). Does that make sense @tlangs?

That does make sense!

Needed to locate a key based on the user and provider name.

Also fix encoding/decoding key JSON

samanehsan · 2024-03-04T18:06:33Z

service/src/main/java/bio/terra/externalcreds/models/FenceAccountKey.java

+
+@Value.Immutable
+public interface FenceAccountKey extends WithFenceAccountKey {
+  Optional<Integer> getId();


FYI this is optional because when we pass a FenceAccountKey object to insertFenceAccountKey, it won't have an id at that point.

Ghost-in-a-Jar · 2024-03-04T22:52:48Z

...in/resources/db/changelog/changesets/20240228_add_fence_account_key_and_dist_lock_table.yaml

+                    references: linked_account(id)
+                    foreignKeyName: fk_linked_account_id
+              - column:
+                  name: key_json


nit: maybe this should be called key_json_base64?

Ghost-in-a-Jar

Looks good overall, just some small changes requested

service/src/main/java/bio/terra/externalcreds/dataAccess/DistributedLockDAO.java

Ghost-in-a-Jar · 2024-03-04T23:02:10Z

service/src/main/java/bio/terra/externalcreds/dataAccess/DistributedLockDAO.java

+  public Optional<DistributedLock> getDistributedLock(String lockName, String userId) {
+    var namedParameters =
+        new MapSqlParameterSource().addValue("lockName", lockName).addValue("userId", userId);
+    var query = "SELECT * FROM distributed_lock WHERE lock_name = :lockName AND user_id = :userId";


This is nitpicky but I think it is best to name each column in a DAO query.

Ghost-in-a-Jar · 2024-03-04T23:02:45Z

service/src/main/java/bio/terra/externalcreds/dataAccess/FenceAccountKeyDAO.java

+    var namedParameters =
+        new MapSqlParameterSource("userId", userId).addValue("providerName", providerName);
+    var query =
+        "SELECT p.* FROM fence_account_key p"


Same comment as above I would name each column instead of using *.

also nitpicky but I would use something more descriptive than 'p' for fence_account_key's alias, maybe 'fence' or 'fak'. Just to make the query easier to read.

Ghost-in-a-Jar · 2024-03-04T23:07:53Z

service/src/main/java/bio/terra/externalcreds/dataAccess/FenceAccountKeyDAO.java

+    var generatedKeyHolder = new GeneratedKeyHolder();
+    jdbcTemplate.update(query, namedParameters, generatedKeyHolder);
+
+    return fenceAccountKey.withId(Objects.requireNonNull(generatedKeyHolder.getKey()).intValue());


I love that you can get the id back from the insert query, there are limitations on this in scala libs that I have used.

tlangs · 2024-03-05T17:12:42Z

service/src/main/resources/db/changelog/db.changelog-master.yaml

@@ -29,3 +29,6 @@ databaseChangeLog:
  - include:
      file: changesets/20220412_add_last_encrypt_timestamp_to_ssh_key_pair_table.yaml
      relativeToChangelogFile: true
+  - include:


There's a merge conflict here. Make sure this changeset comes after the just-merged changeset.

tlangs · 2024-03-05T17:19:42Z

service/src/main/java/bio/terra/externalcreds/dataAccess/FenceAccountKeyDAO.java

+          new FenceAccountKey.Builder()
+              .id(rs.getInt("id"))
+              .linkedAccountId(rs.getInt("linked_account_id"))
+              .keyJson(base64Decode(rs.getString("key_json")))


So, I looked at the Bond code, and its storing the keys as raw json instead of base64. I thought it about it a little bit more, and storing the keys as JSON or JSONB might actually be better, since we could then query into the keys to find info. That may be useful for debugging or analysis.

Apologies for the whiplash...

No worries! That's a good point about search-ability

im confused i thought it was storing the keys as b64?

Bond stores them as plain text.

tlangs · 2024-03-05T21:40:49Z

...in/resources/db/changelog/changesets/20240228_add_fence_account_key_and_dist_lock_table.yaml

+                    primaryKey: true
+                    nullable: false
+              - column:
+                  name: linked_account_id


Oh! Can we make this cascade on delete? That way, if someone deletes their linked account, the key will automatically get deleted too.

Oh I assumed that was the default behavior 😅 I'll double check!

…ials-manager into se/ID-1117-add-fence-account-key-table

tlangs · 2024-03-06T15:26:59Z

service/src/main/java/bio/terra/externalcreds/dataAccess/FenceAccountKeyDAO.java

+  }
+
+  @WithSpan
+  public FenceAccountKey insertFenceAccountKey(FenceAccountKey fenceAccountKey) {


Can we rename this upsertFenceAccountKey and do the same ON CONFLICT logic?

Is it just when there is a conflict on the id and linked_account_id?

Ah I had to add a unique constraint on the linked_account_id in order to use is as the conflict criteria

tlangs · 2024-03-06T16:04:07Z

service/src/main/java/bio/terra/externalcreds/models/DistributedLock.java

+
+  String getUserId();
+
+  Timestamp getExpiresAt();


We should probably use java.time.Instant here instead of the SQL Timestamp.

sonarqubecloud · 2024-03-06T17:20:23Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Add fence_account_key table to db

73fd8a2

This will be used for Bond providers

samanehsan requested a review from a team as a code owner February 28, 2024 21:47

samanehsan force-pushed the se/ID-1117-add-fence-account-key-table branch from 264ec0a to 73d5f55 Compare February 29, 2024 17:39

Add distributed_lock table

6c996e9

samanehsan force-pushed the se/ID-1117-add-fence-account-key-table branch from 73d5f55 to 6c996e9 Compare February 29, 2024 17:51

tlangs reviewed Feb 29, 2024

View reviewed changes

Add models

5b99a70

samanehsan force-pushed the se/ID-1117-add-fence-account-key-table branch from 8ee3aac to 5b99a70 Compare February 29, 2024 21:38

samanehsan commented Feb 29, 2024

View reviewed changes

samanehsan changed the title ~~[ID-1117] Add fence_account_key table to db~~ [DRAFT][ID-1117] Add fence_account_key table to db Feb 29, 2024

samanehsan force-pushed the se/ID-1117-add-fence-account-key-table branch from 4a0cebf to d1bef70 Compare February 29, 2024 22:55

samanehsan added 2 commits March 4, 2024 11:40

Add DistributedLockDAO

0f250c9

Add linked_account_id to fence_account_key

09c2d11

Needed to locate a key based on the user and provider name.

samanehsan force-pushed the se/ID-1117-add-fence-account-key-table branch from d1bef70 to 09c2d11 Compare March 4, 2024 16:42

samanehsan added 3 commits March 4, 2024 12:06

Add FenceAccountKeyDAO

da62d3b

Add FenceAccountKeyDAO tests

2668876

Also fix encoding/decoding key JSON

Add DistributedLockDAO tests

97c8c89

samanehsan commented Mar 4, 2024

View reviewed changes

samanehsan changed the title ~~[DRAFT][ID-1117] Add fence_account_key table to db~~ [ID-1117] Add fence_account_key table to db Mar 4, 2024

samanehsan changed the title ~~[ID-1117] Add fence_account_key table to db~~ [ID-1117] Add fence_account_key and distributed_lock tables to db Mar 4, 2024

Ghost-in-a-Jar reviewed Mar 4, 2024

View reviewed changes

Ghost-in-a-Jar suggested changes Mar 4, 2024

View reviewed changes

tlangs suggested changes Mar 5, 2024

View reviewed changes

tlangs reviewed Mar 5, 2024

View reviewed changes

samanehsan added 4 commits March 5, 2024 16:47

Merge branch 'dev' of github.com:DataBiosphere/terra-external-credent…

c39836f

…ials-manager into se/ID-1117-add-fence-account-key-table

Code review comments

f0a6de3

Change providerName to provider

ccfc78f

Fix tests

fcdd682

tlangs reviewed Mar 6, 2024

View reviewed changes

Ghost-in-a-Jar approved these changes Mar 6, 2024

View reviewed changes

tlangs reviewed Mar 6, 2024

View reviewed changes

samanehsan added 2 commits March 6, 2024 11:47

Change Timestamp to Instant

dded2a2

Upsert fence account key

b81b81d

tlangs approved these changes Mar 6, 2024

View reviewed changes

samanehsan merged commit b54bb7d into dev Mar 6, 2024
10 checks passed

samanehsan deleted the se/ID-1117-add-fence-account-key-table branch March 6, 2024 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ID-1117] Add fence_account_key and distributed_lock tables to db #174

[ID-1117] Add fence_account_key and distributed_lock tables to db #174

samanehsan commented Feb 28, 2024 •

edited

Loading

tlangs Feb 29, 2024

samanehsan Feb 29, 2024

samanehsan Feb 29, 2024

samanehsan Feb 29, 2024

tlangs Feb 29, 2024

samanehsan Feb 29, 2024

tlangs Mar 4, 2024

samanehsan Mar 4, 2024

Ghost-in-a-Jar Mar 4, 2024

Ghost-in-a-Jar left a comment

Ghost-in-a-Jar Mar 4, 2024

Ghost-in-a-Jar Mar 4, 2024

Ghost-in-a-Jar Mar 4, 2024

Ghost-in-a-Jar Mar 4, 2024

tlangs Mar 5, 2024

tlangs Mar 5, 2024

tlangs Mar 5, 2024

samanehsan Mar 5, 2024

Ghost-in-a-Jar Mar 6, 2024

tlangs Mar 6, 2024

tlangs Mar 5, 2024

samanehsan Mar 5, 2024

tlangs Mar 6, 2024

samanehsan Mar 6, 2024 •

edited

Loading

samanehsan Mar 6, 2024

tlangs Mar 6, 2024

sonarqubecloud bot commented Mar 6, 2024

	public interface DistributedLock {
	public interface DistributedLock extends WithDistributedLock {

[ID-1117] Add fence_account_key and distributed_lock tables to db #174

[ID-1117] Add fence_account_key and distributed_lock tables to db #174

Conversation

samanehsan commented Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ghost-in-a-Jar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samanehsan Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 6, 2024

Quality Gate passed

samanehsan commented Feb 28, 2024 •

edited

Loading

samanehsan Mar 6, 2024 •

edited

Loading