feat: Add account-related methods #73

pgrzesik · 2021-01-27T10:58:08Z

Adds account-related methods as described in linked issue.

I've made a decision to depend on injected sdk instance, which is supposed to be an instance of ServerlessSDK from @serverless/platform-client library. It is aimed to be temporary solution, which will be cleaned up when components, enterprise-plugin and serverless will be moved into one repository.

Closes: #72

codecov · 2021-01-27T10:58:55Z

Codecov Report

Merging #73 (3bddaff) into master (f7d2da6) will increase coverage by 0.96%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   89.17%   90.14%   +0.96%     
==========================================
  Files           8        9       +1     
  Lines         194      213      +19     
==========================================
+ Hits          173      192      +19     
  Misses         21       21

Impacted Files	Coverage Δ
config.js	`92.63% <ø> (ø)`
account.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7d2da6...3bddaff. Read the comment docs.

pgrzesik · 2021-01-27T10:59:07Z

account.js

+const jwtDecode = require('jwt-decode');
+const _ = require('lodash');
+
+const logout = () => {


Corresponding method from platform-sdk: https://github.com/serverless/platform-sdk/blob/master/src/logout/index.js#L10

I've made a decision to drop manipulation of enterprise section as it seems to be "legacy" part of the configuration - please correct me if I'm wrong here.

pgrzesik · 2021-01-27T11:00:50Z

account.js

+  // TODO: ADD EXPLANATION TO NOT INTERACT WITH `enterprise` config scope
+};
+
+const refreshToken = async (sdk) => {


Corresponding method in platform-sdk: https://github.com/serverless/platform-sdk/blob/master/src/login/refreshToken.js#L6

I've decided to skip saving expiresAt field as it seems we're not using it anywhere and the expiration date is always derived from idToken anyway. Please let me know if you feel like we're still supposed to save it

pgrzesik · 2021-01-27T11:02:23Z

account.js

+    return;
+  }
+
+  const tokens = await sdk.session.refreshToken(loggedInUser.refreshToken);


Corresponding method from platform-client: https://github.com/serverlessinc/platform/blob/master/sdk/src/index.js#L79

pgrzesik · 2021-01-27T11:02:52Z

account.js

+
+  if (loggedInUser.idToken) {
+    const accessKeyTitle = title || `serverless_${Math.round(+new Date() / 1000)}`;
+    const createdAccessKeyResponse = await sdk.accessKeys.create(


Corresponding method from platform-client: https://github.com/serverlessinc/platform/blob/master/sdk/src/index.js#L461

pgrzesik · 2021-01-27T11:03:32Z

account.js

+  configUtils.set(data);
+};
+
+const getAccessKeyForOrg = async (sdk, orgName, title) => {


Corresponding method in platform-sdk: https://github.com/serverless/platform-sdk/blob/master/src/accessKeys/accessKeys.js#L37

medikoo

Looks great @pgrzesik 👍 I have just few minor concerns

medikoo · 2021-01-27T14:10:37Z

account.js

+};
+
+const getAccessKeyForOrg = async (sdk, orgName, title) => {
+  if (process.env.SERVERLESS_ACCESS_KEY) {


I wonder if this should be handled here, and not where getAccessKeyForOrg is called.

I'd rather see this function to unconditionally retrieve access key for org, and to be invoked when such access key is needed (not injected via env var).

What do you think?

That's a good question - the potential problem here is that all clients of that method would have to account for that fact and implement a potential wrapper that will first check if there's a value injected via SERVERLESS_ACCESS_KEY env var. I think it should be centralized in one place, but I'm no longer sure if utils in the best place, I'll explain in another comment.

account.js

medikoo · 2021-01-27T14:15:25Z

account.js

+  const loggedInUser = configUtils.getLoggedInUser();
+
+  if (!loggedInUser) {
+    throw new Error('Could not find logged in user. Please log in.');


I take this signals programmer error (not something we envision with correct utils usage). In such case, it'll be better to shorten message to simply: Could not find logged in user ("Please log in." suggests it's intended to be presented in valid scenarios to users, and such error should not be thrown with regular Error constructor)

Great point - how would you go about it then? I've adapted to the approach that we currently have in platform-sdk, where such errors are thrown with instructions for users to logout/login (reference: https://github.com/serverless/platform-sdk/blob/master/src/accessKeys/accessKeys.js#L66) and aren't being handled in any specific way.

If those errors handle valid scenarios, and are intended to host messages to be shown to users. Then they should be thrown with ServerlessError (in such case we should move serverless-error utility to this repository).

If those errors are to point errors in our internal flow (and by no means can be triggered by valid user flow), then they should not contain any user facing messaging, but simply state what's wrong.

So my recommendation depends on what kind of error it is (user or programmer?). It's not perfectly clear to me

Thank you and sorry for not being clear. These errors can be triggered by valid flow as far as I'm concerned and they should be visible to users. However, at this point I think it makes more sense to move this function to enterprise-plugin as it's going to be the only client. I'll remove it from this PR, sorry for the confusion

medikoo · 2021-01-27T14:21:28Z

account.js

+
+  if (loggedInUser.idToken) {
+    const accessKeyTitle = title || `serverless_${Math.round(+new Date() / 1000)}`;
+    const createdAccessKeyResponse = await sdk.accessKeys.create(


*Reponse suggests it's a HTTP Response object, and in such case response body will be placed on response.body.

If it's already a parsed response body. I would simply name var accessKeyData.

good call 👍

medikoo · 2021-01-27T14:22:35Z

account.js

+  throw new Error(
+    `Could not find an access key for organization ${orgName}. Log out and log in again to create a new access key for this organization.`
+  );


Same here, it'll be good clarify if it's a programmer or user error, and configure it as one.

Please see my other comment about approach to errors

pgrzesik · 2021-01-28T09:47:27Z

Thanks @medikoo for a review 🙇 I've started thinking again about proper placement for getAccessKeyForOrg method. A few of your suggestions make a lot of sense (about env var handling and error handling), however, that would require the clients of this method to create yet another wrapper for it to provide client specific functionality (honoring env var for example) - which makes me think - are serverless/utils a proper place for this method? The only client at the moment for it will be enterprise-plugin, and if we're going to have to wrap it there anyway, maybe we should just implement it directly in plugin? What do you think?

EDIT: I've also added reconfiguration of sdk with idToken as an accessKey when fetching a new one to ensure that we're calling API with up-to-date token.

medikoo · 2021-01-28T12:47:54Z

The only client at the moment for it will be enterprise-plugin, and if we're going to have to wrap it there anyway, maybe we should just implement it directly in plugin? What do you think?

if it's just for enterprise-plugin I would probably keep there.

Otherwise this utils project is more a common (not really utils) project. It's to host all functionalities that are used by more than one of our CLI packages.

Relying on env vars here is fine. I was just questioning util design, but I don't know all the places where it's used.

pgrzesik · 2021-01-28T13:42:27Z

Thanks for a review, as mentioned, I've decided to remove getAccessKeyForOrg from this PR for now and move the implementation directly to enterprise-plugin.

medikoo

Looks great 👍

pgrzesik self-assigned this Jan 27, 2021

pgrzesik added the enhancement New feature or request label Jan 27, 2021

pgrzesik commented Jan 27, 2021

View reviewed changes

pgrzesik force-pushed the add-account-related-methods branch from 9f226d3 to a7f12b2 Compare January 27, 2021 11:03

pgrzesik requested a review from medikoo January 27, 2021 11:05

medikoo suggested changes Jan 27, 2021

View reviewed changes

pgrzesik requested a review from medikoo January 28, 2021 09:47

pgrzesik force-pushed the add-account-related-methods branch from a7f12b2 to 6ed9287 Compare January 28, 2021 09:51

feat: Add account-related methods

3bddaff

pgrzesik force-pushed the add-account-related-methods branch from 6ed9287 to 3bddaff Compare January 28, 2021 13:38

medikoo approved these changes Jan 28, 2021

View reviewed changes

pgrzesik merged commit c77df3c into master Jan 28, 2021

pgrzesik deleted the add-account-related-methods branch January 28, 2021 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add account-related methods #73

feat: Add account-related methods #73

pgrzesik commented Jan 27, 2021

codecov bot commented Jan 27, 2021 •

edited

pgrzesik Jan 27, 2021

pgrzesik Jan 27, 2021

pgrzesik Jan 27, 2021

pgrzesik Jan 27, 2021

pgrzesik Jan 27, 2021

pgrzesik Jan 27, 2021

pgrzesik Jan 27, 2021

medikoo left a comment

medikoo Jan 27, 2021

pgrzesik Jan 28, 2021 •

edited

medikoo Jan 27, 2021

pgrzesik Jan 28, 2021

medikoo Jan 28, 2021 •

edited

medikoo Jan 28, 2021

pgrzesik Jan 28, 2021

medikoo Jan 27, 2021

pgrzesik Jan 28, 2021

medikoo Jan 27, 2021

pgrzesik Jan 28, 2021

pgrzesik commented Jan 28, 2021 •

edited

medikoo commented Jan 28, 2021

pgrzesik commented Jan 28, 2021

medikoo left a comment

feat: Add account-related methods #73

feat: Add account-related methods #73

Conversation

pgrzesik commented Jan 27, 2021

codecov bot commented Jan 27, 2021 • edited

Codecov Report

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

medikoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik Jan 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medikoo Jan 28, 2021 • edited

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

pgrzesik commented Jan 28, 2021 • edited

medikoo commented Jan 28, 2021

pgrzesik commented Jan 28, 2021

medikoo left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2021 •

edited

pgrzesik Jan 28, 2021 •

edited

medikoo Jan 28, 2021 •

edited

pgrzesik commented Jan 28, 2021 •

edited