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.

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

Feature: Airtable Analyzer for Personal Access Tokens #3941

Conversation

nabeelalam
Copy link
Contributor

@nabeelalam nabeelalam commented Feb 26, 2025

Description:

Airtable PAT Analyzer

image

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

@nabeelalam nabeelalam changed the title Feature: Airtable Analyzer for Personal Access Tokens [WIP] Feature: Airtable Analyzer for Personal Access Tokens Feb 27, 2025
@nabeelalam nabeelalam marked this pull request as ready for review February 28, 2025 15:46
@nabeelalam nabeelalam requested review from a team as code owners February 28, 2025 15:46
@nabeelalam nabeelalam changed the title [WIP] Feature: Airtable Analyzer for Personal Access Tokens Feature: Airtable Analyzer for Personal Access Tokens Feb 28, 2025
Copy link
Collaborator

@ahrav ahrav left a comment

Choose a reason for hiding this comment

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

Great job @nabeelalam! Left a few small suggestion and questions, but otherwise looks great.

return nil, err
}

scopeStatusMap[common.PermissionStrings[common.UserEmailRead]] = userInfo.Email != nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Is the idea here that if the response from the FetchAirtableUserInfo API contains an email, they have the user.email:read scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Here's the API reference for that: https://airtable.com/developers/web/api/get-user-id-scopes

}
}

func determineScope(token string, scope common.Permission, ids map[string]string) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: For consistency and better readability, can we rename scope to permission or perm? Since it's later used as an argument for getEndpointByPermission, this would make the code clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. The terms got a bit shuffled since each AirTable scope maps to a Permission enum in the auto generated permissions.go file, but each AirTable scope also has its own granular "permissions".

I agree that the usage of the terms should be more consistent. I'll update this and any other instances I see with inconsistent use of the terms scope/permission.

}

func determineScopes(token string, basesInfo *common.AirtableBases) error {
if basesInfo != nil && len(basesInfo.Bases) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: Similar to my earlier "line of sight" comment, we can invert the logic of this if statement to return early, allowing us to de-indent the following for loop for better readability.

Ex:

if basesInfo == nil || len(basesInfo.Bases) == 0 {
    return nil
}

for _ base := range baseInfo.Bases {
    ...
}

return nil
}

var Endpoints = map[EndpointName]Endpoint{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nice pattern! One small, optional suggestion: consider constructing this in an init() function and making it private with a public getter.

It’s not a big deal, but this approach prevents unintended modifications by external clients since it wouldn’t be directly exported. I don’t think it's a major concern here, but just something to consider! Totally optional. 😊

Ex:

var endpoints map[EndpointName]Endpoint

func init() {
	endpoints = map[EndpointName]Endpoint{
		GetUserInfoEndpoint: {
			URL:    "https://api.airtable.com/v0/meta/whoami",
			Method: "GET",
		},
		// ... existing code ...
	}
}

// GetEndpoint returns the endpoint for the given name and whether it exists
func GetEndpoint(name EndpointName) (Endpoint, bool) {
	endpoint, exists := endpoints[name]
	return endpoint, exists
}

@nabeelalam
Copy link
Contributor Author

Great job @nabeelalam! Left a few small suggestion and questions, but otherwise looks great.

Thanks for the detailed review, @ahrav. Much appreciated. I'm going through your comments and suggestions.

@ahrav
Copy link
Collaborator

ahrav commented Mar 3, 2025

Great job @nabeelalam! Left a few small suggestion and questions, but otherwise looks great.

Thanks for the detailed review, @ahrav. Much appreciated. I'm going through your comments and suggestions.

Feel free to merge whenever you are ready. None of my comments should be blocking.

@nabeelalam nabeelalam merged commit 59c6f2d into trufflesecurity:main Mar 3, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants