Skip to content

Conversation

@ckritzinger
Copy link
Contributor

This adds quite a lot of detail to the README, I think it's basically everything that I would have needed to get Hello World up and running.

I'm not sure how you feel about about having a longer readme versus moving more detail to examples/. Personally I find it quite useful to have the main gist of things in a Readme that I can read from top to bottom.

I also fixed a small bug in getExpiryDate that I ran into. JWTs don't use padding but it seems that dart:convert requires padding even for base64Url encoding.

ckritzinger and others added 2 commits February 7, 2024 11:10
Co-authored-by: benitav <benitav@users.noreply.github.com>
Comment on lines 117 to 121
// dart:convert doesn't like missing padding
if (part.length % 4 > 0) {
part += '=' * (4 - part.length % 4);
}
final rawData = base64Decode(part);
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that JWTs use base64-url encoding instead of standard base64. Apart from the padding, there are also other minor encoding differences. Could you check if this works?

Suggested change
// dart:convert doesn't like missing padding
if (part.length % 4 > 0) {
part += '=' * (4 - part.length % 4);
}
final rawData = base64Decode(part);
final rawData = base64Url.decode(part);

Copy link
Contributor Author

@ckritzinger ckritzinger Feb 7, 2024

Choose a reason for hiding this comment

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

I'll test, but their docs suggest otherwise: https://api.flutter.dev/flutter/dart-convert/base64Url-constant.html

requires padding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested with your suggested change and still get the same FormatException about invalid length.

dart-lang/sdk#39510 seems to suggest this is deliberate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, there does seem to be a better way to do this, brb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Turns out my initial suggested change doesn't even make a difference - base64url.decode and base64.decode does the same thing.

@rkistner rkistner merged commit 8d37351 into powersync-ja:master Feb 12, 2024
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.

3 participants