feat: support queries to generate self-service api-keys #428

swetabar · 2024-11-07T18:51:49Z

Related Issues

Supporting the following queries with this PR:

getApiKeysByImsUserIdAndImsOrgId
getApiKeyById
updateApiKey

Sample apiKey entry in the DB:

{
  id: '321456987efgh',
  hashedApiKey: 'axspbgh5649075',
  name: 'test-name',
  imsUserId: 'test-ims-user-id',
  imsOrgId: 'test-ims-org-id',
  createdAt: '2024-10-29T14:26:00.000Z'
  expiresAt: '2025-05-29T14:26:00.000Z',
  scopes: [{
        name: 'imports.write',
        domains: ['https://www.test.com'],
      }],
}

atopper · 2024-11-07T20:13:08Z

packages/spacecat-shared-data-access/src/index.d.ts

+  readonly ACTIVE: 'ACTIVE';
+  readonly INACTIVE: 'INACTIVE';
+};
+
 // TODO: introduce AuditType interface or Scores interface


Intentional TODO?

Looks like it was already present in the codebase.

packages/spacecat-shared-data-access/src/dto/api-key.js

blefebvre · 2024-11-07T21:22:47Z

packages/spacecat-shared-data-access/src/dto/api-key.js

@@ -29,6 +29,8 @@ export const ApiKeyDto = {
    createdAt: apiKey.getCreatedAt(),
    expiresAt: apiKey.getExpiresAt(),
    revokedAt: apiKey.getRevokedAt(),
+    deletedAt: apiKey.getDeletedAt(),
+    status: apiKey.getStatus(),


What does status represent? A sample API Key object in the PR description with details on each field would be helpful.

blefebvre · 2024-11-07T21:23:34Z

packages/spacecat-shared-data-access/src/index.d.ts

+  readonly ACTIVE: 'ACTIVE';
+  readonly INACTIVE: 'INACTIVE';
+};
+
 // TODO: introduce AuditType interface or Scores interface


Looks like it was already present in the codebase.

blefebvre · 2024-11-07T21:25:31Z

packages/spacecat-shared-data-access/src/index.d.ts

+  getApiKeyByImsUserIdAndImsOrgId: (
+      imsUserId: string,
+      imsOrgId: string,
+  ) => Promise<ApiKey | null>;


What if a user has more than 1 API key? IIRC, we were going to allow 3 per IMS User ID, so this query could return up to 3 results.

Good point. Typo for the interface. I'll fix it.

packages/spacecat-shared-data-access/src/models/api-key/api-key-constants.js

blefebvre · 2024-11-07T21:27:55Z

packages/spacecat-shared-data-access/src/models/api-key/api-key-constants.js

+ * Api Key Status Types
+ * Any changes to this object needs to be reflected in the index.d.ts file as well.
+ */
+export const ApiKeyStatus = {


Do we depend on this status property? For instance, if an API key "expires" by the passing of time but its entity in the DB is not updated to reflect that it is now INACTIVE, is the key still considered ACTIVE?

The key is still considered ACTIVE in that case. It is updated to INACTIVE when keys are deleted.

Just thinking out loud: would we be any worse off if we didn't have the status column? We'll have to check the deletedAt, revokedAt, and expiresAt anyways to determine if a key is still valid, so status seems potentially unnecessary.

Yes. I could remove status for the api-key.

blefebvre · 2024-11-07T21:30:06Z

packages/spacecat-shared-data-access/src/models/api-key/api-key.js

+   */
+  self.updateDeletedAt = (deletedAt) => updateState('deletedAt', deletedAt, (value) => {
+    if (!isIsoDate(value)) {
+      throw new Error(`Invalid deletedAt during update: ${value}`);


Worth mentioning the expected format in this error?

blefebvre · 2024-11-07T21:35:37Z

packages/spacecat-shared-data-access/src/service/api-key/accessPatterns.js

 */
 export const createNewApiKey = async (apiKey, dynamoClient, config, log) => {
-  const item = ApiKeyDto.toDynamoItem(apiKey);
-  await dynamoClient.putItem(config.tableNameApiKeys, item, log);
+  const item = createApiKey(apiKey);


Looks like this function was previously invoked with an API key object. Has it changed to accept a POJO instead? If so, the JSDoc should be updated to reflect that.

I think we were not using this method before. I refactored it to work similar to how import-job works now for consistency.

Co-authored-by: Bruce Lefebvre <[email protected]>

…y-constants.js Co-authored-by: Bruce Lefebvre <[email protected]>

github-actions · 2024-11-08T10:11:24Z

This PR will trigger a minor release when merged.

blefebvre · 2024-11-08T15:42:26Z

packages/spacecat-shared-data-access/docs/schema.json

+          "AttributeType": "S"
+        },
+        {
+          "AttributeName": "status",


Status is still present here.

blefebvre · 2024-11-08T15:57:38Z

packages/spacecat-shared-data-access/src/dto/api-key.js

-     *          imsOrgId: *, expiresAt: *}}
+     * @returns {{createdAt: string, name: string, imsUserId: string,
+     * scopes: array<object>, revokedAt: string, deletedAt: string,
+     * hashedApiKey: *, imsOrgId: *, expiresAt: *}}


Please add types.

packages/spacecat-shared-data-access/src/service/api-key/index.js

blefebvre · 2024-11-08T16:00:52Z

packages/spacecat-shared-data-access/src/models/api-key/api-key.js

  self.getScopes = () => self.state.scopes;

+  /**


Would this model be the right place for an isValid() method, which could check all the internal properties to ensure that a key is still valid?

Or isActive(), alternatively?

We could do that in the service. Do we need it in spacecat-shared?

If we do so in the service than anywhere that re-uses this model will need to implement something similar. I see it as a direct concern of the model itself, "am I still valid", and think that it makes sense to have that logic live alongside the model.

I also like the pattern of "asking" the entity itself, are you still valid: if (apiKeyInstance.isValid()) { — nice and clean.

Co-authored-by: Bruce Lefebvre <[email protected]>

blefebvre · 2024-11-08T19:00:35Z

packages/spacecat-shared-data-access/src/models/api-key/api-key.js

+  self.isValid = () => {
+    const now = new Date();
+
+    if (self.state.deletedAt && new Date(self.state.deletedAt) > now) {


Is this comparison correct? If deletedAt is > now, that would mean it was deleted in the future — right? And therefore still valid?

Yes. Correct.

It will be deleted in future. So, the key is still valid.

blefebvre · 2024-11-08T19:01:52Z

packages/spacecat-shared-data-access/src/models/api-key/api-key.js

+    if (self.state.revokedAt && new Date(self.state.revokedAt) > now) {
+      return false;
+    }
+
+    if (self.state.expiresAt && new Date(self.state.expiresAt) > now) {
+      return false;
+    }


Same issue with these two, I think.

If an expiry is set when the key is created (as it must) than it will always be in the future, which would mean that this check fails and the key is not considered valid.

blefebvre · 2024-11-08T19:03:09Z

packages/spacecat-shared-data-access/test/unit/models/api-key.test.js

+    });
+
+    it('returns false if deletedAt is after the current time', () => {
+      apiKey.updateDeletedAt(new Date(Date.now() + 10000).toISOString());


Can concrete values please be used for these test cases so they are easier for a human to reason about?

ie. if a key expires in 2025 and it is 2024, it should still be valid.

blefebvre · 2024-11-08T19:04:40Z

packages/spacecat-shared-data-access/test/unit/models/api-key.test.js

+      expect(apiKey.isValid()).to.be.false;
+    });
+
+    it('returns true if deletedAt, revokedAt, and expiresAt are before the current time', () => {


This is the core of the issue, I think.

if expiresAt is in the future: still valid

if deletedAt or revokedAt are in the past: no longer valid

In other words: expiresAt is the only property we should expect to be in the future. If ANY of these properties are set to a date before now, the key should not be considered valid.

blefebvre

Minor item about dates in test cases.

blefebvre · 2024-11-08T19:23:52Z

packages/spacecat-shared-data-access/test/unit/models/api-key.test.js

+      apiKey.updateDeletedAt('2025-05-27T14:26:00.000Z');
+      apiKey.updateRevokedAt('2025-05-27T14:26:00.000Z');
+      apiKey.updateExpiresAt('2025-05-27T14:26:00.000Z');


This test will start failing next May :)

Can we bump the year to 3025 please?

# [@adobe/spacecat-shared-data-access-v1.50.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-data-access-v1.49.8...@adobe/spacecat-shared-data-access-v1.50.0) (2024-11-08) ### Features * support queries to generate self-service api-keys ([#428](#428)) ([90a46c2](90a46c2))

adobe-bot · 2024-11-08T19:30:46Z

🎉 This PR is included in version @adobe/spacecat-shared-data-access-v1.50.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

swetabar added 3 commits November 7, 2024 11:28

feat: Add new queries to create and retrieve api keys

3fd90eb

fix: Fix comments

84b71a3

fix: Add a couple more unit tests

05335db

swetabar requested a review from blefebvre November 7, 2024 18:51

atopper reviewed Nov 7, 2024

View reviewed changes

blefebvre reviewed Nov 7, 2024

View reviewed changes

swetabar and others added 2 commits November 8, 2024 05:11

Update packages/spacecat-shared-data-access/src/dto/api-key.js

d55c268

Co-authored-by: Bruce Lefebvre <[email protected]>

Update packages/spacecat-shared-data-access/src/models/api-key/api-ke…

d91f203

…y-constants.js Co-authored-by: Bruce Lefebvre <[email protected]>

swetabar added 11 commits November 8, 2024 05:32

Merge branch 'main' into SITES-25915-generate-api-keys-for-importer

9b88940

fix: Update package-lock.json

35a30c6

fix: Change the function name getApiKeysByImsUserIdAndImsOrgId

fd186b6

fix: Change the error message for deletedAt

d712d8c

fix: Fix the maxlen issue

f2d39e8

fix: Modify the return type to an array

4594e36

fix: Remove status from ApiKey

2d30f6d

fix: Remove status from UTs

39705d0

fix: remove updateStatus from a unit test

e78553f

Merge branch 'main' into SITES-25915-generate-api-keys-for-importer

a8bf73a

fix: update package-lock.json

77f24ab

blefebvre reviewed Nov 8, 2024

View reviewed changes

fix: remove status

ae834c3

blefebvre reviewed Nov 8, 2024

View reviewed changes

packages/spacecat-shared-data-access/src/service/api-key/index.js Outdated Show resolved Hide resolved

blefebvre reviewed Nov 8, 2024

View reviewed changes

swetabar and others added 5 commits November 8, 2024 11:04

Update packages/spacecat-shared-data-access/src/service/api-key/index.js

b53564b

Co-authored-by: Bruce Lefebvre <[email protected]>

fix: refactor

eb36d56

fix: add unit tests and function for isValid

9142b07

Merge branch 'main' into SITES-25915-generate-api-keys-for-importer

fd2b122

fix: Add more unit tests

d7334ca

swetabar added 2 commits November 8, 2024 13:51

fix: add the latest package-lock.json

0bbc7eb

fix: change to Date object

9df129c

blefebvre reviewed Nov 8, 2024

View reviewed changes

swetabar added 3 commits November 8, 2024 14:14

fix: Fix the date comparison

69b914b

fix: Add real iso strings for comparison

14b1535

fix: Fix the unit tests

fdf2f6e

blefebvre approved these changes Nov 8, 2024

View reviewed changes

fix: Fix the year

2c6e841

swetabar self-assigned this Nov 8, 2024

swetabar added the enhancement New feature or request label Nov 8, 2024

swetabar merged commit 90a46c2 into main Nov 8, 2024
9 checks passed

swetabar deleted the SITES-25915-generate-api-keys-for-importer branch November 8, 2024 19:28

adobe-bot added the released label Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support queries to generate self-service api-keys #428

feat: support queries to generate self-service api-keys #428

swetabar commented Nov 7, 2024 •

edited

Loading

atopper Nov 7, 2024

blefebvre Nov 7, 2024

blefebvre Nov 7, 2024

blefebvre Nov 7, 2024

blefebvre Nov 7, 2024

swetabar Nov 8, 2024

blefebvre Nov 7, 2024

swetabar Nov 8, 2024

blefebvre Nov 8, 2024

swetabar Nov 8, 2024

blefebvre Nov 7, 2024

blefebvre Nov 7, 2024

swetabar Nov 8, 2024

github-actions bot commented Nov 8, 2024

blefebvre Nov 8, 2024

blefebvre Nov 8, 2024

blefebvre Nov 8, 2024

blefebvre Nov 8, 2024

swetabar Nov 8, 2024

blefebvre Nov 8, 2024

blefebvre Nov 8, 2024

swetabar Nov 8, 2024

swetabar Nov 8, 2024

blefebvre Nov 8, 2024

swetabar Nov 8, 2024

blefebvre Nov 8, 2024

swetabar Nov 8, 2024

blefebvre Nov 8, 2024

blefebvre Nov 8, 2024

blefebvre left a comment

blefebvre Nov 8, 2024

adobe-bot commented Nov 8, 2024

feat: support queries to generate self-service api-keys #428

feat: support queries to generate self-service api-keys #428

Conversation

swetabar commented Nov 7, 2024 • edited Loading

Related Issues

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

github-actions bot commented Nov 8, 2024

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

Choose a reason for hiding this comment

blefebvre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Nov 8, 2024

swetabar commented Nov 7, 2024 •

edited

Loading