Skip to content
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

Initial checkin of feature to allow faking the Cognito user #16956

Merged
merged 1 commit into from May 6, 2021

Conversation

richiethom
Copy link
Contributor

Initial PR for adding functionality described in Issue 16647.

@patriot1burke
Copy link
Contributor

BTW, I forgot to say, thanks for submitting a PR!

@patriot1burke
Copy link
Contributor

One more comment...Why do you even need the environment variable? Can you change your test code to add the JWT? Wouldn't that be a better way to test it? Or do you really like the ease of this approach?

@patriot1burke patriot1burke marked this pull request as ready for review May 3, 2021 21:33
@patriot1burke
Copy link
Contributor

Looks good. Write some tests please?

@patriot1burke
Copy link
Contributor

patriot1burke commented May 4, 2021

@jonathanroques wrote:
"see my previous comment above, but do we even need this AWS_SAM_LOCAL env var ? If I provide the FORCE_USERNAME then it should be enough in any env (local, jvm, native)."

AWS_SAM_LOCAL is something set by SAM LOCAL correct? If so, then I like this approach as FORCE_USEERNAME is not something you want to allow at runtime. Correct?

@ghost
Copy link

ghost commented May 4, 2021

@jonathanroques wrote:
"see my previous comment above, but do we even need this AWS_SAM_LOCAL env var ? If I provide the FORCE_USERNAME then it should be enough in any env (local, jvm, native)."

AWS_SAM_LOCAL is something set by SAM LOCAL correct? If so, then I like this approach as FORCE_USEERNAME is not something you want to allow at runtime. Correct?

oh ok I guessed AWS_SAM_LOCAL was manually set. I agree with you then 👌.

@patriot1burke Btw do you know if we have the same kind of env var using gradle quarkusDev task ?

@ghost
Copy link

ghost commented May 4, 2021

Btw, is that an issue if we have this behaviour for http extension and not for the rest one ?

@patriot1burke
Copy link
Contributor

Btw, is that an issue if we have this behaviour for http extension and not for the rest one ?

Good point, i'll open a separate issue that. This PR is good, let's get some tests in and get it merged.

@patriot1burke
Copy link
Contributor

@richiethom I'll accept this PR, but I'm thinking of expanding on it.

  • Have a CognitoPrincipal that extends principal and makes JWT available from a getter method
  • Have a FORCE_JWT env var where you can put in your own JWT json. This might be useful for testing as well?

I also need to port this work to the amazon-lambda-rest extension too and document this.

@patriot1burke
Copy link
Contributor

Another critique of this, is why wouldn't you just invoke locally passing in the JWT with your event when running with SAM LOCAL?

@richiethom
Copy link
Contributor Author

richiethom commented May 6, 2021

Another critique of this, is why wouldn't you just invoke locally passing in the JWT with your event when running with SAM LOCAL?

The reason for wanting to fake the Cognito user when running locally is that it seems either difficult/overly-complicated to do (by connecting the Local instance to an Authorizer running in the cloud) or impossible, depending on who you ask. I certainly couldn't get it working locally. Knowing that this was mainly associated with the lack of an API Gateway running locally, that SAM may not even do anything with JWT tokens when running locally, and that my testing only really required to know the associated Cognito user, I thought it would make things simpler if I simply made it possible to fake the user (and perhaps the claims later on). The aim was to be able to keep things simple locally without (obviously) breaking anything when running on AWS.

I don't hide that I'm not at all an AWS expert. My experience of AWS Lambda is exclusively through the lens of Quarkus, so I've been learning a lot as I go. It is entirely possible that I'm trying to fix a problem that isn't there. You're probably much better placed to tell me if I'm wasting my time or not!

@ghost
Copy link

ghost commented May 6, 2021

Another critique of this, is why wouldn't you just invoke locally passing in the JWT with your event when running with SAM LOCAL?

I agree with @richiethom. SAM LOCAL has some issues (especially not handling authorizers...). Besides, when I'm testing my code I don't want to run sam local with an event each time and won't give me any code debug in IntelliJ.

The easiest way to do this is using quarkusDev with an env var for the user (as I also only need the user id, email in my case). Then I have a fake user + live debug capabilities and I don't depend on sam local (issues).

I'm also not an AWS expert at all, so I might have missed something :)

@patriot1burke
Copy link
Contributor

Ok, I'll accept this PR guys. Ty for your patience.

FYI @richiethom @jonathanroques I'm planning on making Quarkus dev/test mode available for lambda development so that you can mostly mimic a lambda environment locally without sam local. The idea would be when you start Quarkus in dev mode, I'd spin up a HTTP container you could post invocations to. Similar to this approach:

https://docs.aws.amazon.com/lambda/latest/dg/images-test.html

Except without the containers. I'd like your input on it when I get a prototype going.

@patriot1burke patriot1burke merged commit 8de7953 into quarkusio:main May 6, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone May 6, 2021
@richiethom
Copy link
Contributor Author

Thanks for merging @patriot1burke, but please bear in mind I hadn't yet implemented any tests!

@patriot1burke
Copy link
Contributor

@richiethom Yes, I know you didn't implement any tests. Wasn't sure you were going to or not. I'll just add them when I port this to amazon-lambda-rest.

@richiethom
Copy link
Contributor Author

@richiethom Yes, I know you didn't implement any tests. Wasn't sure you were going to or not. I'll just add them when I port this to amazon-lambda-rest.

Yes, it was going to happen...eventually...a question of finding the time!

@ghost
Copy link

ghost commented May 6, 2021

The better tests are the ones you don't write :p Hey @richiethom I tried to join you, are you on the Quarkus Zulip by any chance ?

@patriot1burke
Copy link
Contributor

@jonathanroques @richiethom FYI, i don't ZULIP. I can't stand messaging apps. Too many interrupts. email me (bburke@redhat.com) if you want something or post to quarkus-dev list.

@richiethom richiethom deleted the quarkus_16647 branch May 8, 2021 05:42
@geoand
Copy link
Contributor

geoand commented May 22, 2021

@patriot1burke is this backportable to 1.13?

@patriot1burke
Copy link
Contributor

@geoand IMO, no. I've revamping it a bunch and giving this proper integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants