-
Notifications
You must be signed in to change notification settings - Fork 249
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
feat!(core,auth): auth providers definition and CognitoIamAuthProvider registers in Auth #1851
feat!(core,auth): auth providers definition and CognitoIamAuthProvider registers in Auth #1851
Conversation
Co-authored-by: Jordan Nelson <Jordan.Ryan.Nelson@gmail.com>
|
||
/// A hardcoded key which can provide throttling for an unauthenticated API. | ||
/// | ||
/// See also: | ||
/// - [API Key Authorization](https://docs.aws.amazon.com/appsync/latest/devguide/security-authz.html#api-key-authorization) | ||
@JsonValue('API_KEY') | ||
apiKey, | ||
apiKey(AmplifyAuthProviderToken<AmplifyAuthProvider>()), |
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.
I think this will have a different auth provider type for api key, but plan to do in PR where I integrate in API plugin w api key.
packages/auth/amplify_auth_cognito_dart/test/plugin/auth_providers_test.dart
Outdated
Show resolved
Hide resolved
packages/auth/amplify_auth_cognito_dart/test/plugin/auth_providers_test.dart
Outdated
Show resolved
Hide resolved
…ders_test.dart Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>
…ders_test.dart Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>
packages/auth/amplify_auth_cognito_dart/lib/src/auth_plugin_impl.dart
Outdated
Show resolved
Hide resolved
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.
Left one nit, but otherwise LGTM
…pl.dart Co-authored-by: Dustin Noyes <dustin.noyes.dev@gmail.com>
@@ -16,6 +16,7 @@ import 'dart:convert'; | |||
|
|||
import 'package:amplify_flutter/amplify_flutter.dart'; | |||
import 'package:amplify_flutter/src/amplify_impl.dart'; | |||
import 'package:flutter/material.dart'; |
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.
Why do we need the material library here? It provides only the UI widgets correct?
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.
good catch, removed. I think it was left from a previous implementation that used describeEnum
.
abstract class AmplifyAuthProvider { | ||
Future<AWSBaseHttpRequest> authorizeRequest( | ||
AWSBaseHttpRequest request, { | ||
covariant AuthProviderOptions? options, |
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.
Good to know about covariant
, it should help us to avoid type casting required on consumer side like this?
final result = await Amplify.Auth.fetchAuthSession(
options: CognitoSessionOptions(getAWSCredentials: true),
);
String identityId = (result as CognitoAuthSession).identityId!;
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.
Maybe I'm missing this, but don't think it helps here and casting would still be needed as I think it can only be used for parameters, not return types.
Separately, I think we will be able to define an auth provider that can provide identity IDs so casting would not be needed outside auth plugin for this use case.
…tter into feat/api-iam
…r registers in Auth (#1851)
…r registers in Auth (#1851)
…r registers in Auth (aws-amplify#1851)
…r registers in Auth (aws-amplify#1851)
…r registers in Auth (#1851)
…r registers in Auth (#1851)
…r registers in Auth (#1851)
…r registers in Auth (#1851)
…r registers in Auth (#1851)
…r registers in Auth (aws-amplify#1851) commit-id:332d00d5
…r registers in Auth (#1851)
…r registers in Auth (#1851) commit-id:332d00d5
…r registers in Auth (#1851)
…r registers in Auth (aws-amplify#1851)
…r registers in Auth (#1851)
This PR has the initial implementation in core of auth providers, related changes to plugin configuration signature and auth plugin registers the first auth provider. In another PR, I plan to integrate this with API plugin to support IAM auth mode but this PR is intentionally downscoped to just upstream logic to register the auth provider.
In summary, the auth providers are a way for other plugins to call the auth plugin indirectly. By registering an auth provider from auth plugin, other plugins essentially get a callback they can use to call auth methods as long as auth is registered at runtime and other plugins don't need an auth dependency or to call
Amplify.Auth...
directly. This avoids compile-time errors.BREAKING CHANGE: configure method for plugins now requires an instance of auth provider repo. This change is only breaking for plugin authors, not plugin users.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.