Skip to content

Conversation

@andresmoschini
Copy link
Member

@andresmoschini andresmoschini commented Apr 15, 2021

Hi team,

This is a big PR! but I think that it is possible to review it easily by reading commit by commit.

It supports 3 different scenarios, not all APIs require all of them:

  • Verify token signature (whatever the account or superuser status is)
  • Verify if the token is from a superuser
  • Verify if the user has access to an URL based on his account ID or name

Maybe you do not need all of these scenarios and having a specific policy as the default is enough.

The tests are very detailed, so, it could be easy and safe adapt it for a new behavior.


Tested in INT environment with my account's token:

image

@andresmoschini andresmoschini force-pushed the DE-135-doppler-security branch from ba27eb3 to 1369489 Compare April 15, 2021 22:17
Comment on lines +45 to +49
// TODO: I would like to use ownResourceOrSuperUserPolicy as the default policy, but I
// cannot override a more restrictive policy with a less restrictive one. So,
// for the moment, we have to be carefull and chooses the right one for each
// controller.
o.DefaultPolicy = simpleAuthenticationPolicy;
Copy link
Member Author

Choose a reason for hiding this comment

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

@ms-darianbenito this what we were talking about today.

@andresmoschini andresmoschini mentioned this pull request Apr 16, 2021
17 tasks
Comment on lines +1 to +13
# Unit Testing in the local environment

To be able to run unit tests, if those require JWT to execute correctly, then you will be able to generate/update them by using the following private key located in this directory ([`dev.priv.key`](./dev.priv.key))

The steps needed to do so are the following

1. Go to [jwt.io](https://jwt.io/)
2. Select RS256 as the algorithm
3. Enter the payload
4. Paste the key mentioned above in the private key section
5. [jwt.io](https://jwt.io/) will generate a token ready to be used in the tests

**Note:** The tokens generated for unit tests will be valid for that context only
Copy link
Contributor

Choose a reason for hiding this comment

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

Lo veo bien como parte de este commit. También quedaría claro en un siguiente commint con docs:

var response = await client.GetAsync(url);

// Assert
Assert.Equal(expectedStatusCode, response.StatusCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure but we need that anonymous endpoint just not response 401 status code.
Assert.NotEqual(HttpStatusCode.Unauthorized, response.StatusCode);

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not understand.

Is not enough waiting for a 200 OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, It is, but I am thinking on another response status like InternalServerError, and that still a valid scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I understand now.

I think that an InternalServerError is not enough to confirm that anonymous authorization is working fine.

Maybe there is another problem, so, in that case, the result should not be pass, maybe inconclusive, but in that case, the test will be more complex.

@andresmoschini andresmoschini force-pushed the DE-135-doppler-security branch 2 times, most recently from d0c548d to 78e27fd Compare April 17, 2021 00:05
@andresmoschini andresmoschini merged commit 28bf9ab into main Apr 19, 2021
@andresmoschini andresmoschini deleted the DE-135-doppler-security branch April 27, 2021 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants