Skip to content

Conversation

@mike-zorn
Copy link

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Closes #490

Describe the solution you've provided

Supports the api_key prefix on keys used for authentication.

@mike-zorn mike-zorn requested review from a team and keelerm84 February 10, 2025 18:45
return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
ctx := request.Context()
projectKey := request.Header.Get("Authorization")
projectKey = strings.TrimPrefix(projectKey, "api_key ") // some sdks set this as a prefix
Copy link
Author

Choose a reason for hiding this comment

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

@keelerm84 Are there other scenarios we should be aware of for auth key formats?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. The api_key prefix isn't coming from any of the SDKs I wouldn't think. We don't add additional prefixing, outside of the standard key formats.

Maybe that's something more aligned with the integrations team, or whoever is in charge of the auto-generated API SDKs?

Copy link
Author

Choose a reason for hiding this comment

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

lol glad we were both surprised by this behavior. It looks like iOS is doing this prefixing thing. Looks like same for android.

Copy link
Member

Choose a reason for hiding this comment

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

🤯 Well that IS surprising. I wonder if that is still necessary.

I checked the relay, and there is a fetchAuthToken method that only checks for that api_key prefix, so maybe we can be more confident that prefix is the only one we have to worry about.

Copy link
Author

Choose a reason for hiding this comment

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

Ooh. Thank you for looking at the relay implementation. If that's all it supports, I think we're all good here.

@mike-zorn mike-zorn changed the title Handle api_key prefix in auth header fix: Handle api_key prefix in auth header Feb 10, 2025

func (s *Sqlite) CreateBackup(ctx context.Context) (io.ReadCloser, int64, error) {
backupPath, err := s.backupManager.MakeBackupFile(ctx)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Oof nice catch - should we make a follow up item to add a linter rule to catch ineffectual assigns to err?

Copy link
Author

Choose a reason for hiding this comment

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

The linter is running if you use pre-commit, but it doesn't look like pre-commit checks are being run in ci.

Copy link
Author

Choose a reason for hiding this comment

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

But yeah. A follow up item to fixup the CI linter is a good idea.

@mike-zorn mike-zorn merged commit 216a951 into main Feb 10, 2025
9 of 10 checks passed
@mike-zorn mike-zorn deleted the FUN-569/mrz/dev-server-cant-handle-api-key-prefix branch February 10, 2025 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mobile Authorization Key & Project Value api_key Prefix (project not found)

4 participants