corsac-dart / jwt

Lightweight implementation of JSON Web Tokens (JWT)
BSD 2-Clause "Simplified" License
45 stars 28 forks source link

Migrate to null safety #21

Closed Jjagg closed 3 years ago

Jjagg commented 3 years ago

Migrates the library to NNBD.

Null-safety is not sound because rsa_pkcs and pointycastle have not yet been migrated to NNBD. However, because corsac_jwt does not export anything from those libraries and there's very little code using them, and that code looks to be null-safe, there's no harm in using them like this.

I migrated this library because I depend on it in a project and had a conflicting dependency on crypto (because another library I depend on depended on the null-safe preview version of crypto).

The issue can be worked around by adding a dependency_override for crypto, so if you prefer you could hold off merging this until both rsa_pkcs and pointycastle have migrated as well.

EDIT: All (non-dev) dependencies have been migrated, so this branch is now sound null-safe.

pulyaevskiy commented 3 years ago

Thanks for submitting! I had another PR waiting for review, after merge this one has conflicts now, sorry. Let me know if you'd be ok with resolving the conflicts and I'll take a look when it's ready.

Jjagg commented 3 years ago

Yeah, no problem :)

augustozanellato commented 3 years ago

Any plans on publishing a prerelease with nnbd support using this branch?

Jjagg commented 3 years ago

If you reference this branch right now it'll cause a dependency constraint issue on crypto because of rsa_pkcs. So either we need to relax the constraint here or (the better option) migrate rsa_pkcs.

You can directly reference this branch if you want in your pubspec with a git reference. But because of the crypto issue you'll need to add it to dependency overrides.

augustozanellato commented 3 years ago

I just opened a PR on rsa_pkcs, after that's merged this pr should be unblocked amaurel/rsa_pkcs#15

augustozanellato commented 3 years ago

Hey @Jjagg can you update this pr to use pointcastle: ^3.0.0-nullsafety.2?

EDIT: that was fast :P

Jjagg commented 3 years ago

I already updated locally, but forgot to push :)

augustozanellato commented 3 years ago

Looks like you removed the pubspec dependency for logging by accident

Error: Could not resolve the package 'logging' in 'package:logging/logging.dart'.
../../flutter/.pub-cache/git/jwt-a9d28e5c42cbf87cf5ba42ddd410dd74aa4410ed/lib/corsac_jwt.dart:36:8: Error: Not found: 'package:logging/logging.dart'
import 'package:logging/logging.dart';
       ^
../../flutter/.pub-cache/git/jwt-a9d28e5c42cbf87cf5ba42ddd410dd74aa4410ed/lib/corsac_jwt.dart:43:17: Error: Method not found: 'Logger'.
final _logger = Logger('JWTRsaSha256Signer');
                ^^^^^^
Unhandled exception:
FileSystemException(uri=org-dartlang-untranslatable-uri:package%3Alogging%2Flogging.dart; message=StandardFileSystem only supports file:* and data:* URIs)
#0      StandardFileSystem.entityForUri (package:front_end/src/api_prototype/standard_file_system.dart:36:7)
#1      asFileUri (package:vm/kernel_front_end.dart:599:37)
#2      writeDepfile (package:vm/kernel_front_end.dart:738:21)
<asynchronous suspension>
#3      FrontendCompiler.compile (package:frontend_server/frontend_server.dart:558:9)
<asynchronous suspension>
#4      starter (package:flutter_frontend_server/server.dart:180:12)
<asynchronous suspension>
#5      main (file:///opt/s/w/ir/cache/builder/src/flutter/flutter_frontend_server/bin/starter.dart:13:24)
<asynchronous suspension>

Command PhaseScriptExecution failed with a nonzero exit code
note: Using new build system
note: Building targets in parallel
note: Planning build
note: Constructing build description
** BUILD FAILED **
augustozanellato commented 3 years ago

Had to make another PR, see amaurel/rsa_pkcs#16, while we wait for the PR to be merged and for a new release on pub.dev you can override the dependecy like this and it should work fine

dependency_overrides:
  rsa_pkcs:
    git:
      url: https://github.com/memolight/rsa_pkcs.git
      ref: 071afd90adefa9834da72bb8865710c4adf479ad
augustozanellato commented 3 years ago

rsa_pkcs has been finally updated to 2.0.0 with full support for nnbd. PR should be ready to merge after bumping rsa_pkcs @pulyaevskiy @Jjagg

jagmit commented 3 years ago

@Jjagg thank you for your work! Is anything missing to get this merged? All tests succeed in my local environment

0biWanKenobi commented 3 years ago

Sorry for bumping this PR, but it's the last package missing before I can migrate my app to null safety.. πŸ˜„

fbatschi commented 3 years ago

I would also like to see this PR merged. Especially because of the updated dependency of crypto ^3.0.0

MilesAdamson commented 3 years ago

Sorry for bumping this PR, but it's the last package missing before I can migrate my app to null safety.. πŸ˜„

same

pulyaevskiy commented 3 years ago

Thanks everyone and sorry for silence. I'll take a look tonight.

It looks like all dependencies (including crypto?) has been ported so there should be no blockers to merge this.

codecov-commenter commented 3 years ago

Codecov Report

Merging #21 (32ff5b7) into master (58d8196) will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   95.48%   95.51%   +0.02%     
==========================================
  Files           2        2              
  Lines         155      156       +1     
==========================================
+ Hits          148      149       +1     
  Misses          7        7              
Impacted Files Coverage Ξ”
lib/corsac_jwt.dart 95.45% <100.00%> (+0.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 58d8196...32ff5b7. Read the comment docs.

pulyaevskiy commented 3 years ago

This is now live as https://pub.dev/packages/corsac_jwt/versions/1.0.0-nullsafety.1