MaikuB / flutter_appauth

A Flutter wrapper for AppAuth iOS and Android SDKs
270 stars 243 forks source link

[flutter_appauth]feat: add state parameter #321

Closed Sembauke closed 2 years ago

Sembauke commented 2 years ago

As this repository hosts two packages, please ensure the PR title starts with the name of the package that it relates to using square brackets (e.g. [flutter_appauth])

Sembauke commented 2 years ago

Hi @MaikuB,

I have been working on adding the state parameter. More info on it here: https://auth0.com/docs/secure/attack-protection/state-parameters

I am not sure how to test it.

if you have any suggestions or changes you would like me to make. Let me know.

MaikuB commented 2 years ago

Thanks for the PR. Do you mean to say you made the changes without verifying that they work? One thing I should call out is the native SDKs will generate a random state value already so this PR is really more on if apps want to do so on there own. With this in mind, I suggest that the API docs for the property gets updated to indicate a null value means the native SDKs will deal with state

Sembauke commented 2 years ago

Thanks for the review Maiku. I am having trouble testing it in the example app as this newly added state parameter is not showing up.

Our app needs a custom state set, so it will retrieve more personal data.

MaikuB commented 2 years ago

This is a monorepo that can be used with melos so once you've done the setup covered in their docs, melos will do some setup behind the scenes so that local dependencies are used. This would then solve the issue. Without this, the code for the plugin just references the dependencies that have been published that wouldn't have your changes

Note: the purpose of the state parameter from my understanding was preventing cross site forgery attacks and this is mentioned in the OIDC specs. With this in mind, perhaps the dependency on this needs to be re-evaluated though I'm sure there will be others that would find PR useful

MaikuB commented 2 years ago

Note I've tried testing your changes and it doesn't work on iOS with a state mismatch error being returned. The state coming back from the server looks like one generated by the AppAuth iOS SDK so it looks like you are missing some changes to make sure the state value specified is actually be used

Sembauke commented 2 years ago

Alright thanks for the review. I will look into it.

Sembauke commented 2 years ago

Hey @MaikuB,

I am trying out Melos - It works great. Only I am unable to do reach the demo.identityserver.io for some reason. Also the build seems to pass on Android. But not on IOS.

I have read your comment, and I am trying to come up with a good solution.

Only I can't find a good way to debug. The only thing that is coming up is:

melos run build:example_ios
   └> melos exec -c 1 -- \  "flutter build ios --no-codesign"
       └> FAILED
ScriptException: The script build:example_ios failed to execute
MaikuB commented 2 years ago

You'll need to update your branch with the latest changes from master. I recently pushed changes as the details of the demo IdentityServer instance changed.

As for your iOS build issue, that would be something you'd likely need to look further into as it's likely an issue with your environment. CI is setup on the repo using the same commands and you should that find the iOS build task works fine on your PR.

For debugging, you don't really need to run the build commands to begin with. As mentioned earlier, once you bootstrap melos it'll set all projects in your clone to use local dependencies. After this you debug an app as you normally would i.e. don't need to use the melos CLI anymore after bootstrapping. If you happen to still struggle with this then you could manually update all the pubspec.yaml files to point to your local versions of each package. See https://docs.flutter.dev/development/packages-and-plugins/using-packages#dependencies-on-unpublished-packages

MaikuB commented 2 years ago

@Sembauke wanted to see if you had an updates on this and plan to continue on the PR or if this is being abandoned

Sembauke commented 2 years ago

I will not be continuing this. We found a better way of doing thing using our own API.