-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Feature: Airtable Analyzer for Personal Access Tokens #3941
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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{ |
There was a problem hiding this comment.
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
}
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. |
… issues; improved code readability
Description:
Airtable PAT Analyzer
Checklist:
make test-community
)?make lint
this requires golangci-lint)?