Ident Review by ConsenSys Mesh and contributors

[humbitious]

Ident
Repository: https://github.com/provideplatform/ident
Reviewer: Ryan Fisch
Structure
The project contains multiple service implementations isolated by directory structure with service root defining the noun. Each service directory contains the following at a minimum:
handlers.go
<service_noun>.go

The application handlers.go file defines the service endpoints for the rest API.

Documentation
There is no content in the README.md and the online code reference to: https://docs.provide.services/api/microservices/ident is 404
Test Coverage
Tests are run using the command line “make integration”, this downloads local dependencies, starts container instances and executes all the tests in the tests/integration folder then terminates running instances.. All tests appear to have passed as there are no ERROR outputs on the console execution.

Results from make integration test run:

=== RUN TestCreateApplication
=== PAUSE TestCreateApplication
=== RUN TestListApplicationUsers
=== PAUSE TestListApplicationUsers
=== RUN TestGetApplicationDetails
=== PAUSE TestGetApplicationDetails
=== RUN TestUpdateApplicationDetails
=== PAUSE TestUpdateApplicationDetails
=== RUN TestFetchAppDetailsFailsWithUnauthorizedUser
=== PAUSE TestFetchAppDetailsFailsWithUnauthorizedUser
=== RUN TestUserUpdateAppDetailsAccess
=== PAUSE TestUserUpdateAppDetailsAccess
=== RUN TestDeleteApplication
=== PAUSE TestDeleteApplication
=== RUN TestListApplicationTokens
=== PAUSE TestListApplicationTokens
=== RUN TestApplicationOrganizationList
=== PAUSE TestApplicationOrganizationList
=== RUN TestCreateApplicationOrganization
=== PAUSE TestCreateApplicationOrganization
=== RUN TestDeleteApplicationOrganizationWithApplicationAPIToken
=== PAUSE TestDeleteApplicationOrganizationWithApplicationAPIToken
=== RUN TestCreateApplicationUser
=== PAUSE TestCreateApplicationUser
=== RUN TestUpdateApplicationUser
=== PAUSE TestUpdateApplicationUser
=== RUN TestDeleteApplicationUserWithApplicationAPIToken
=== PAUSE TestDeleteApplicationUserWithApplicationAPIToken
=== RUN TestInviteUserFailsWithoutEmail
=== PAUSE TestInviteUserFailsWithoutEmail
=== RUN TestInviteUserFailsWithInvalidEmail
=== PAUSE TestInviteUserFailsWithInvalidEmail
=== RUN TestInviteUserByUserWithoutSudoFailsWithArbitraryPermission
=== PAUSE TestInviteUserByUserWithoutSudoFailsWithArbitraryPermission
=== RUN TestInviteUserFailsWithExistingUserEmail
=== PAUSE TestInviteUserFailsWithExistingUserEmail
=== RUN TestInviteUserWithUserAPIToken
=== PAUSE TestInviteUserWithUserAPIToken
=== RUN TestInviteApplicationUserWithApplicationAPIToken
=== PAUSE TestInviteApplicationUserWithApplicationAPIToken
=== RUN TestInviteOrganizationUserWithOrganizationAPIToken
=== PAUSE TestInviteOrganizationUserWithOrganizationAPIToken
=== RUN TestCreateOrganization
=== PAUSE TestCreateOrganization
=== RUN TestListOrganizationTokens
=== PAUSE TestListOrganizationTokens
=== RUN TestGetOrganizationDetailsWithAuthorizedUserToken
organization_test.go:105: auth response: &{User:0xc00012a320 Token:0xc0004d8160}
organization_test.go:134: getting organisation details for org 70db3899-6ad8-4d3c-b5eb-f6f274bd401a
--- PASS: TestGetOrganizationDetailsWithAuthorizedUserToken (0.18s)
=== RUN TestOrganizationDetailsWithOrgToken
organization_test.go:203: authy auth: &{User:0xc0000e0460 Token:0xc000120630}
organization_test.go:214: orgy orgy: &{Model:{ID:92f9ea7d-06c6-49c5-af78-b9f9a541fc7c CreatedAt:2021-08-18 11:58:33.936462665 +0000 UTC Errors:[]} Name:0xc000420200 UserID:3dfe746f-2615-4620-baa4-9767a1467dab Description:0xc0004201f0 Permissions:0 Metadata:map[]}
organization_test.go:214: orgy orgy: &{Model:{ID:8a5a3720-1155-45c5-a23b-d89a6c1534cf CreatedAt:2021-08-18 11:58:33.942468777 +0000 UTC Errors:[]} Name:0xc0000cb610 UserID:3dfe746f-2615-4620-baa4-9767a1467dab Description:0xc0000cb600 Permissions:0 Metadata:map[]}
organization_test.go:214: orgy orgy: &{Model:{ID:c52926d7-feda-47ae-b469-e422e02e0307 CreatedAt:2021-08-18 11:58:33.947883869 +0000 UTC Errors:[]} Name:0xc000694d00 UserID:3dfe746f-2615-4620-baa4-9767a1467dab Description:0xc000694cf0 Permissions:0 Metadata:map[]}
organization_test.go:214: orgy orgy: &{Model:{ID:af23d356-e34f-4869-b751-dadbb81a5959 CreatedAt:2021-08-18 11:58:33.952784426 +0000 UTC Errors:[]} Name:0xc000421370 UserID:3dfe746f-2615-4620-baa4-9767a1467dab Description:0xc000421360 Permissions:0 Metadata:map[]}
organization_test.go:222: getting organisation details for org 92f9ea7d-06c6-49c5-af78-b9f9a541fc7c
organization_test.go:249: org 92f9ea7d-06c6-49c5-af78-b9f9a541fc7c details ok
organization_test.go:222: getting organisation details for org 8a5a3720-1155-45c5-a23b-d89a6c1534cf
organization_test.go:249: org 8a5a3720-1155-45c5-a23b-d89a6c1534cf details ok
organization_test.go:222: getting organisation details for org c52926d7-feda-47ae-b469-e422e02e0307
organization_test.go:249: org c52926d7-feda-47ae-b469-e422e02e0307 details ok
organization_test.go:222: getting organisation details for org af23d356-e34f-4869-b751-dadbb81a5959
organization_test.go:249: org af23d356-e34f-4869-b751-dadbb81a5959 details ok
--- PASS: TestOrganizationDetailsWithOrgToken (0.28s)
=== RUN TestOrganizationDetailsWithUserToken
organization_test.go:310: getting organisation details for org dd352075-6623-4b45-ba00-aa6d79c0ba16
organization_test.go:332: org dd352075-6623-4b45-ba00-aa6d79c0ba16 details ok
organization_test.go:310: getting organisation details for org 92c339c2-1a31-447a-ab0d-e79ef00c5c46
organization_test.go:332: org 92c339c2-1a31-447a-ab0d-e79ef00c5c46 details ok
organization_test.go:310: getting organisation details for org 98200a43-c73b-492f-acc8-38658e22b60b
organization_test.go:332: org 98200a43-c73b-492f-acc8-38658e22b60b details ok
organization_test.go:310: getting organisation details for org 6caac979-5f62-436b-94e0-a963180bf07c
organization_test.go:332: org 6caac979-5f62-436b-94e0-a963180bf07c details ok
--- PASS: TestOrganizationDetailsWithUserToken (0.19s)
=== RUN TestUpdateOrganizationDetails
=== PAUSE TestUpdateOrganizationDetails
=== RUN TestFetchOrgDetailsFailsWithUnauthorizedUser
=== PAUSE TestFetchOrgDetailsFailsWithUnauthorizedUser
=== RUN TestCreateOrganizationUser
=== PAUSE TestCreateOrganizationUser
=== RUN TestUpdateOrganizationUser
=== PAUSE TestUpdateOrganizationUser
=== RUN TestListApplicationOrganizationUsers
=== PAUSE TestListApplicationOrganizationUsers
=== RUN TestDeleteOrganizationUser
=== PAUSE TestDeleteOrganizationUser
=== RUN TestListOrganizationUsersWithNoUsersInOrg
=== PAUSE TestListOrganizationUsersWithNoUsersInOrg
=== RUN TestListOrganizationUsersUsingOrganizingUser
=== PAUSE TestListOrganizationUsersUsingOrganizingUser
=== RUN TestListOrganizationUsers
=== PAUSE TestListOrganizationUsers
=== RUN TestUserAccessRefreshToken
=== PAUSE TestUserAccessRefreshToken
=== RUN TestAppRevocableToken
=== PAUSE TestAppRevocableToken
=== RUN TestAppAccessRefreshToken
=== PAUSE TestAppAccessRefreshToken
=== RUN TestOrgAccessRefreshToken
=== PAUSE TestOrgAccessRefreshToken
=== RUN TestCreateUser
=== PAUSE TestCreateUser
=== RUN TestAuthenticateUser
=== PAUSE TestAuthenticateUser
=== RUN TestUserDetails
=== PAUSE TestUserDetails
=== RUN TestUserUpdate
=== PAUSE TestUserUpdate
=== RUN TestDeleteUser
=== PAUSE TestDeleteUser
=== RUN TestOauthCallback
=== PAUSE TestOauthCallback
=== CONT TestCreateApplication
=== CONT TestDeleteOrganizationUser
--- PASS: TestCreateApplication (0.42s)
=== CONT TestAppRevocableToken
--- PASS: TestDeleteOrganizationUser (1.05s)
=== CONT TestListOrganizationUsersUsingOrganizingUser
--- PASS: TestAppRevocableToken (0.22s)
=== CONT TestListOrganizationUsersUsingOrganizingUser
organization_user_test.go:284: got correct number of organization users back 1 using organizing user token
=== CONT TestListApplicationOrganizationUsers
--- PASS: TestListOrganizationUsersUsingOrganizingUser (0.20s)
=== CONT TestListApplicationOrganizationUsers
organization_test.go:779: test not implemented yet
=== CONT TestListOrganizationUsers
--- PASS: TestListApplicationOrganizationUsers (0.00s)
=== CONT TestListOrganizationUsers
organization_user_test.go:372: got correct number of organization users back 3
--- PASS: TestListOrganizationUsers (0.53s)
=== CONT TestListOrganizationUsersWithNoUsersInOrg
organization_user_test.go:226: got correct number of organization users back 0 using org token
=== CONT TestOauthCallback
--- PASS: TestListOrganizationUsersWithNoUsersInOrg (0.20s)
=== CONT TestOauthCallback
user_test.go:239: TBD
=== CONT TestInviteUserWithUserAPIToken
--- PASS: TestOauthCallback (0.00s)
--- PASS: TestInviteUserWithUserAPIToken (0.22s)
=== CONT TestCreateApplicationOrganization
=== CONT TestCreateUser
--- PASS: TestCreateApplicationOrganization (0.27s)
--- PASS: TestCreateUser (0.13s)
=== CONT TestDeleteUser
user_test.go:225: TBD - might require soft delete code change?
=== CONT TestFetchOrgDetailsFailsWithUnauthorizedUser
--- PASS: TestDeleteUser (0.00s)
=== CONT TestInviteUserByUserWithoutSudoFailsWithArbitraryPermission
--- PASS: TestFetchOrgDetailsFailsWithUnauthorizedUser (0.34s)
--- PASS: TestInviteUserByUserWithoutSudoFailsWithArbitraryPermission (0.16s)
=== CONT TestUserUpdateAppDetailsAccess
--- PASS: TestUserUpdateAppDetailsAccess (0.37s)
=== CONT TestUpdateOrganizationDetails
--- PASS: TestUpdateOrganizationDetails (0.18s)
=== CONT TestFetchAppDetailsFailsWithUnauthorizedUser
application_test.go:391: about to check app details with unauthorized user
application_test.go:392: app id: dd8b0542-3d85-4ea8-a674-c0ebfbe40e49
application_test.go:393: user id: &{Model:{ID:a4993628-395e-4e2a-bc53-b11938e8d187 CreatedAt:2021-08-18 11:58:38.664069696 +0000 UTC Errors:[]} Name:first last.notauth FirstName:first LastName:last.notauth Email:[email protected] Permissions:415 PrivacyPolicyAgreedAt: TermsOfServiceAgreedAt: Metadata:map[]}
application_test.go:391: about to check app details with unauthorized user
application_test.go:392: app id: cbbebe92-9347-4986-9ee1-8e64bb904b64
application_test.go:393: user id: &{Model:{ID:a4993628-395e-4e2a-bc53-b11938e8d187 CreatedAt:2021-08-18 11:58:38.664069696 +0000 UTC Errors:[]} Name:first last.notauth FirstName:first LastName:last.notauth Email:[email protected] Permissions:415 PrivacyPolicyAgreedAt: TermsOfServiceAgreedAt: Metadata:map[]}
=== CONT TestListApplicationUsers
--- PASS: TestFetchAppDetailsFailsWithUnauthorizedUser (0.36s)
=== CONT TestListApplicationUsers
application_test.go:163: got correct number of application users back 3
=== CONT TestUpdateApplicationDetails
--- PASS: TestListApplicationUsers (0.55s)
--- PASS: TestUpdateApplicationDetails (0.27s)
=== CONT TestDeleteApplication
--- PASS: TestDeleteApplication (0.47s)
=== CONT TestInviteUserFailsWithoutEmail
--- PASS: TestInviteUserFailsWithoutEmail (0.18s)
=== CONT TestInviteUserFailsWithInvalidEmail
--- PASS: TestInviteUserFailsWithInvalidEmail (0.17s)
=== CONT TestUserUpdate
=== CONT TestDeleteApplicationUserWithApplicationAPIToken
--- PASS: TestUserUpdate (0.99s)
=== CONT TestCreateOrganizationUser
--- PASS: TestDeleteApplicationUserWithApplicationAPIToken (0.28s)
--- PASS: TestCreateOrganizationUser (0.51s)
=== CONT TestListApplicationTokens
--- PASS: TestListApplicationTokens (0.31s)
=== CONT TestCreateOrganization
=== CONT TestUpdateApplicationUser
--- PASS: TestCreateOrganization (0.33s)
=== CONT TestUpdateApplicationUser
application_test.go:1097: not yet implemented
=== CONT TestDeleteApplicationOrganizationWithApplicationAPIToken
--- PASS: TestUpdateApplicationUser (0.00s)
--- PASS: TestDeleteApplicationOrganizationWithApplicationAPIToken (0.25s)
=== CONT TestInviteApplicationUserWithApplicationAPIToken
=== CONT TestInviteOrganizationUserWithOrganizationAPIToken
--- PASS: TestInviteApplicationUserWithApplicationAPIToken (0.23s)
=== CONT TestUserDetails
--- PASS: TestInviteOrganizationUserWithOrganizationAPIToken (0.22s)
=== CONT TestCreateApplicationUser
--- PASS: TestUserDetails (0.49s)
=== CONT TestApplicationOrganizationList
--- PASS: TestCreateApplicationUser (0.54s)
--- PASS: TestApplicationOrganizationList (0.25s)
=== CONT TestAppAccessRefreshToken
--- PASS: TestAppAccessRefreshToken (0.24s)
=== CONT TestInviteUserFailsWithExistingUserEmail
=== CONT TestOrgAccessRefreshToken
--- PASS: TestInviteUserFailsWithExistingUserEmail (0.22s)
--- PASS: TestOrgAccessRefreshToken (0.23s)
=== CONT TestUpdateOrganizationUser
organization_test.go:774: TBD not yet implemented in code
=== CONT TestAuthenticateUser
--- PASS: TestUpdateOrganizationUser (0.00s)
=== CONT TestGetApplicationDetails
--- PASS: TestAuthenticateUser (0.20s)
=== CONT TestListOrganizationTokens
--- PASS: TestGetApplicationDetails (0.25s)
=== CONT TestListOrganizationTokens
organization_test.go:71: TBD no method in provide-go to list organization tokens
=== CONT TestUserAccessRefreshToken
--- PASS: TestListOrganizationTokens (0.00s)
--- PASS: TestUserAccessRefreshToken (0.20s)
PASS
ok github.com/provideplatform/ident/test/integration 13.270s

Code Review
Database Access
The application is primarily based on the use of inline SQL query generation. Although this will be one of those updates for debate items, the use of function or stored procedures in Postgres v11+ is a more performant and safer approach to data access. It also allows for non-code deployment in the event of data access logic changes or schema changes that don’t influence the call interface.

That being said the code implements the GORM Object Relational Mapper client but may benefit from more object/model usage than straight SQL generation, or using named parameters or other argument based query calls. There may also be performance optimizations available for alternative usage like cache query plans on function/proc cached plans.

I haven’t found tests yet for the SQL query generation, but the code is exercised through the integration tests.

GORM does provide SQL Injection interception and value escaping.

Database configurations are in a personal repository referenced in by GO resolution:
dbconf "github.com/kthomas/go-db-config"

Each call in crud inline ops appears to open a new connection, with a query result return. I'm unsure of the GO behavior on connection termination or the ability to capitalize on connection pooling?

Need to review query execution errors or timeouts as they are not immediately handled in code and no error is being assessed except for the absence of a response from the ORM?

Category: Performance
File: token/token.go
Lines: 968 - 971
Recommendation:

Collapse multiple append’s into a single append operation.

subscribeAllow = append(subscribeAllow, "network..connector.", "network.*.status", "platform.>", "baseline.>")

Category: Style
File: user/user.go
Lines: 108, 78
Recommendation:
Drop type declaration on like type arguments.
func Exists(email string, applicationID, organizationID *uuid.UUID)

Category: Style
File: common/utils.go
Lines: 39
Recommendation:
Drop type declaration on like type arguments.
func PanicIfEmpty(val, msg string)

Category: Style
File: cdm/api_accountant/accountant.go
Lines: 160, 162, 171, 189
Recommendation:
Error strings starting in fmt.Errorf should not be capitalized.

Category: Security
File: ops/sudo.sh
Lines: 27
Recommendation:
Leaked secret detected
Note: Unsure what the secret is being used from from a test/prod standpoint so may be fine

Category: Security
File: ops/run_consumer.sh
Lines: 4, 130
Recommendation:
Leaked secret detected
Note: Unsure what the secret is being used from from a test/prod standpoint so may be fine

Category: Security
File: ops/run_api.sh
Lines: 28, 154
Recommendation:
Leaked secret detected
Note: Unsure what the secret is being used from from a test/prod standpoint so may be fine

Category: Security
File: common /utils.go
Lines: 14
Recommendation:
Possible use of weak random number generator, consider using crypto/rand vs. math/ran
Ref: https://blog.gopheracademy.com/advent-2017/a-tale-of-two-rands/

Category: Anti-pattern
File: token/middleware.go
Lines: 74
Recommendation:
subject = fmt.Sprintf("%s", *token.Subject)
Subject is already a string

Category: Anti-pattern
File: application/application.go
Lines: 78
Recommendation:
return orgs != nil && len(orgs) == 1
Possible redundant check on nil, orgs slices defined as default 0, len(orgs) == 1 probably sufficient

Category: Anti-pattern
File: cmd/migrate/main.go
Lines: 100
Recommendation:
if time.Now().Sub(startedAt) >= initIfNotExistsTimeout {
Replace time.Now().Sub(startedAt)with time.Since(startDate)

Category: Anti-pattern
File: token/token.go
Lines: 948 - 950, 934 - 936
Recommendation:
Replace loop with subscribeDeny = append(publishAllow, deny…)

Category: Anti-pattern
File: token/token.go
Lines: 942-944
Recommendation:
Replace loop with subscribeAllow = append(subscribeAllow, allow...)

Category: Anti-pattern
File: token/token.go
Lines: 928-930
Recommendation:
Replace loop with publishAllow = append(publishAllow, allow...)

Category: Anti-pattern
File: cmd/consumer/main.go
Lines: 43
Recommendation:
Attempting to capture an uncapturable SIGKILL signal

Category: Anti-pattern
File: cmd/api_accountant/main.go
Lines: 101
Recommendation:
Attempting to capture an uncapturable SIGKILL signal

Category: Anti-pattern
File: cmd/api/main.go
Lines: 87
Recommendation:
Attempting to capture an uncapturable SIGKILL signal

Category: Anti-pattern
File: organization/organization.go
Lines: 262-263
Recommendation:
Empty If block possible missed implementation

Category: Anti-pattern
File: token/token.go
Lines: 489
Recommendation:
Pure function value is ignored
if len(t.Errors) > 0 {
msg = fmt.Sprintf("%s; %s", msg, *t.Errors[0].Message) }

Category: Bug Risk
File: user/invite.go, line 128, 174;
Observation: unused value assignment in err
Category: Bug Risk
File: user/handlers.go, line 510;
Observation: unused value assignment in err

Category: Bug Risk
File: token/token.go, line 489;
Observation: unused value assignment in msg

Category: Bug Risk
File: organization/handlers.go, line 275, 446;
Observation: unused value assignment in err

Category: Bug Risk
File: cmd/migrate/main.go, line 117;
Observation: unused value assignment in result

Category: Bug Risk
File: ops/ci-process.sh, line 23;
Observation: Use "$@" to prevent whitespace problems

Category: Bug Risk
File: check all .sh 27 occurrences across bash files
Observation: Use double quote to prevent globbing and word splitting

Category: Bug Risk
File: ops/ecs_deploy.sh, line 73;
Observation: remove surrounding $() to avoid executing output unless this is the desired behavior.

Category: Bug Risk
File: common/middleware.go; line 25 & cmd/api_accountant/daemon.go line 85
Observation: shadowing of predeclared identifier

Category: Bug Risk
File: (24 total occurrences) user/user.go, line 225; user/invite.go, line 18; user/consumer.go, line 17, 51, organization/organization.go line 34, 39; common/permissions.go, line 114; common/config.go, line 21; cmd/sudo/main.go, line 20; cmd/sudo/ident.go, line 14, 15, 16, 17; cmd/api_accoutnant/daemon.go, line 18, 22; cmd/api_accountant/accountant.go, line 81, 92; application/application.go, line 271, 328
Observation: Unused Code