Add SuperOffice provider #417

AnthonyYates · 2020-05-04T11:59:36Z

Add SuperOffice provider and unit tests.
Update README.md

No longer use JetBrains.Annotations attributes.

README.md

src/AspNet.Security.OAuth.SuperOffice/AspNet.Security.OAuth.SuperOffice.csproj

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationConfiguration.cs

src/AspNet.Security.OAuth.SuperOffice/Implementation/DefaultSuperOfficeConfigurationManager.cs

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationHandler.cs

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationInvalidEnvironmentException.cs

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationOptions.cs

test/AspNet.Security.OAuth.Providers.Tests/AspNet.Security.OAuth.Providers.Tests.csproj

…perOfficeConfigurationManager.cs Co-authored-by: Martin Costello <martin@martincostello.com>

…nConfiguration.cs Co-authored-by: Martin Costello <martin@martincostello.com>

…nConstants.cs Spelling correction. Co-authored-by: Martin Costello <martin@martincostello.com>

Co-authored-by: Martin Costello <martin@martincostello.com>

….OAuth.Providers into dev

This reverts commit 8ccf405.

Updates project content based on PR feedback.

Updated comment based on PR feedback.

AnthonyYates

Hi Martin,
Thanks for all the comments and suggestions. I believe I have either corrected everything or commented accordingly and am awaiting any further comments.

Best regards!

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationConfigurationManager.cs

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationEnvironment.cs

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationHandler.cs

src/AspNet.Security.OAuth.SuperOffice/Implementation/DefaultSuperOfficeConfigurationManager.cs

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationConfiguration.cs

src/AspNet.Security.OAuth.SuperOffice/AspNet.Security.OAuth.SuperOffice.csproj

src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticationConfiguration.cs

Prevents duplicate claims with signin/signout/signin scenario.

Co-authored-by: Martin Costello <martin@martincostello.com>

kevinchalet · 2020-05-06T13:27:43Z

Hey @AnthonyYates!

Thanks for your contribution 👏

I took a brief look and wouah, it will be a lot of code to maintain (like the Apple provider) 😭
Assuming SuperOffice implements standard-compliant OAuth 2.0/OpenID Connect provider discovery, we should instead consider using IdentityModel's ConfigurationManager (Microsoft.IdentityModel.Protocols.OpenIdConnect).

https://github.com/dotnet/aspnetcore/blob/master/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectPostConfigureOptions.cs#L71-L102

Hard-coded JWKS URI instead of utilizing open-id configuration to determine jwks URL.

AnthonyYates · 2020-07-02T11:04:42Z

The sample application, using my provider, works just fine with System.IdentityModel.Tokens.Jwt from version 5.4.0, it's just the tests fail when calling Options.ConfigurationManager.GetConfigurationAsync(Context.RequestAborted); . Bumping it to 5.6.0 makes my local tests pass, but appveyor and travis-ci seem to have issues with it. It causes the Apple test to fail.

Going to put the props to 5.4.0... maybe it's just my local environment that has issues...

martincostello · 2020-07-02T11:19:16Z

Are you correctly rebased onto dev? It might be that your branch doesn't have the fix integrated from #439.

As I get this in the GitHub UI, this would suggest to me that it's not been rebased appropriately:

As we squash merge big PRs anyway, there's nothing to lose by squashing this PR down into one commit locally based on the current dev branch, and then force pushing.

AnthonyYates · 2020-07-02T11:47:48Z

Maybe not? When I visited this page earlier today I discovered there were two conflicts. So I fixed them here on Github and committed them.

That's what I started to convert the rest of my solution to the new additions such as Directory.Packages.props.

I see I have both commits from #439

Tempted to save what I have and start this whole PR from scratch - clean slate.

AnthonyYates · 2020-07-05T15:44:36Z

@martincostello , I think I got the conflict fixed. Can you verify that from your side? Also, it seems you have made some build/testing changes I'm not familiar with, and do not know how that is meant to impact my PR.

I updated the IdentityModel packages to 5.6.0, and all tests are passing. I did have to alter one Apple test, but only specify the complete namespace for an expected exception. Unsure why the type wasn't resolving correctly since the namespace was declared at the top.

Any advice the automated build error message "The project file could not be loaded. Data at the root level is invalid."?

AspNet.Security.OAuth.Providers.sln

Directory.Packages.props

martincostello · 2020-07-05T16:03:49Z

If you run build.cmd or build.sh locally, you should be able to find compilation errors and/or test failures before commiting.

AnthonyYates · 2020-07-05T16:24:03Z

@martincostello , thanks for the tip(s)! Unfortunately running local build.cmd doesn't shed any light on compilation errors in the PR.

martincostello · 2020-07-05T16:25:39Z

Ah yes, sorry, you need to add the -test argument to run the tests too.

kevinchalet · 2020-07-05T16:29:01Z

@martincostello should we tweak the build workflow to always upload the xUnit results even if the tests didn't pass?

martincostello · 2020-07-05T16:30:38Z

Can do. The old one used to, but if the new one doesn’t it must’ve got accidentally lost in the migration to GH Actions.

AnthonyYates · 2020-07-05T16:44:44Z

OK, Test errors are AppleTests.

When run in VS, they all but one pass:

Interestingly, when run in debug mode, it green lights.

However, running tests with build.cmd -test produces more failed tests:

Can_Sign_In_Using_Apple_With_Client_Secret
Can_Sign_In_Using_Apple_With_Private_Key
Cannot_Sign_In_Using_Apple_With_Expired_Token
Cannot_Sign_In_Using_Apple_With_Invalid_Token_Audience
Cannot_Sign_In_Using_Apple_With_Invalid_Token_Issuer

Mostly related to SecurityTokenInvalidSignatureException

martincostello · 2020-07-06T19:19:34Z

Based on this announcement, maybe we should just use 5.5.0 as the minimum, which is the built-in version for 3.1?

aspnet/Announcements#428

AnthonyYates · 2020-07-07T08:37:24Z

@martincostello If you can make the AppleTests succeed with 5.5.0, so be it!

martincostello · 2020-07-07T11:35:20Z

Looks like something is different in 5.5.0 that disrupts the timing/sequencing in some way (caching?), so that configuration options associated with previous tests are being leftover or something.

I'll look into it when I have the time to do so. Otherwise, either you can look into it and fix it if you have the time, or otherwise downgrade to 5.4.0 if you can get the SuperOffice package to work with that version.

martincostello · 2020-07-07T13:15:37Z

I've found the problem. I'll push up a fix some time later today. TL;DR - a fix similar to #439 needs to be done elsewhere in the code.

martincostello · 2020-07-07T16:43:10Z

Fixes are now in dev.

AnthonyYates · 2020-07-07T17:35:25Z

OK, great! Didn't have time today to get into it... Will update things now.

AnthonyYates · 2020-07-07T18:18:10Z

Excellent @martincostello! Seems everything is good to go now. Appreciate your efforts and work on this PR, can't thank you enough.

kevinchalet · 2020-07-07T18:31:52Z

Merged, thanks for your contribution.

martincostello · 2020-07-07T18:35:10Z

I’ll look at prepping the 3.1.2 release to ship this to NuGet.org when I get some time tomorrow, probably in the evening.

In the meantime you can consume the prerelease version from our MyGet feed 🙂

SuperOfficeDevNet added 4 commits May 4, 2020 12:40

Initial checkin SuperOffice provider

6b75310

Removed dependency on JetBrains.Annotations

f19e703

No longer use JetBrains.Annotations attributes.

Updated README.md with SuperOffice info

d563257

Updated README.md with SuperOffice details.

4d1ac8a

AnthonyYates mentioned this pull request May 4, 2020

Add SuperOffice provider #418

Closed

martincostello linked an issue May 4, 2020 that may be closed by this pull request

Add SuperOffice provider #418

Closed

martincostello reviewed May 4, 2020

View reviewed changes

martincostello added the new provider label May 4, 2020

AnthonyYates and others added 16 commits May 4, 2020 15:09

Removed duplicate SuperOffice entry.

31ac0d6

Update src/AspNet.Security.OAuth.SuperOffice/Implementation/DefaultSu…

e984e45

…perOfficeConfigurationManager.cs Co-authored-by: Martin Costello <martin@martincostello.com>

Update src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticatio…

05088a2

…nConfiguration.cs Co-authored-by: Martin Costello <martin@martincostello.com>

Update src/AspNet.Security.OAuth.SuperOffice/SuperOfficeAuthenticatio…

3c61c1a

…nConstants.cs Spelling correction. Co-authored-by: Martin Costello <martin@martincostello.com>

Spelling correction.

f7e0587

Co-authored-by: Martin Costello <martin@martincostello.com>

Spelling correction.

8a7d99b

Co-authored-by: Martin Costello <martin@martincostello.com>

Spelling correction.

b7814d5

Co-authored-by: Martin Costello <martin@martincostello.com>

Spelling correction.

8bdb2b9

Co-authored-by: Martin Costello <martin@martincostello.com>

Apply Spelling corrections.

c12eb98

Co-authored-by: Martin Costello <martin@martincostello.com>

Spelling corrections

85d50f8

Co-authored-by: Martin Costello <martin@martincostello.com>

Minor updates based on PR comments.

8ccf405

Merge branch 'dev' of https://github.com/AnthonyYates/AspNet.Security…

c34753c

….OAuth.Providers into dev

Revert "Minor updates based on PR comments."

8c2a6d4

This reverts commit 8ccf405.

Undid test cs proj file changes

fab6b66

Minor updates.

9e26f1d

Updates project content based on PR feedback.

Updated comment.

1c3b602

Updated comment based on PR feedback.

AnthonyYates commented May 4, 2020

View reviewed changes

SuperOfficeDevNet and others added 2 commits May 4, 2020 18:39

Updated Options.ClaimActions

5a9bb67

Prevents duplicate claims with signin/signout/signin scenario.

Changed string comparison option.

accd181

Co-authored-by: Martin Costello <martin@martincostello.com>

kevinchalet mentioned this pull request May 6, 2020

Consider using IdentityModel's ConfigurationManager in the Apple provider #421

Closed

Simplified Configuration - Jwks Handling

0810018

Hard-coded JWKS URI instead of utilizing open-id configuration to determine jwks URL.

Update Directory.Packages.props, reset Tokens package to 5.4.0

1c548e3

SuperOfficeDevNet added 2 commits July 5, 2020 15:37

Merge remote-tracking branch 'upstream/dev' into dev

46e6085

Updated IdentityModel packages to 5.6.0, and one AppleTest

2999a98

martincostello reviewed Jul 5, 2020

View reviewed changes

AspNet.Security.OAuth.Providers.sln Show resolved Hide resolved

Directory.Packages.props Outdated Show resolved Hide resolved

Fix Sln file, and add space in Packages.props

cbc75ef

Changed System.IdentityModel libs to 5.5.0

ece145e

This was referenced Jul 7, 2020

Update System.IdentityModel.Tokens.Jwt to v5.5.0 #451

Merged

Always publish logs and test results #452

Merged

Merge remote-tracking branch 'upstream/dev' into dev

e89a24f

kevinchalet merged commit 75ebd13 into aspnet-contrib:dev Jul 7, 2020

kevinchalet added this to the 3.1.2 milestone Jul 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SuperOffice provider #417

Add SuperOffice provider #417

AnthonyYates commented May 4, 2020

AnthonyYates left a comment

kevinchalet commented May 6, 2020

AnthonyYates commented Jul 2, 2020 •

edited

Loading

martincostello commented Jul 2, 2020 •

edited

Loading

AnthonyYates commented Jul 2, 2020

AnthonyYates commented Jul 5, 2020

martincostello commented Jul 5, 2020

AnthonyYates commented Jul 5, 2020

martincostello commented Jul 5, 2020

kevinchalet commented Jul 5, 2020

martincostello commented Jul 5, 2020

AnthonyYates commented Jul 5, 2020

martincostello commented Jul 6, 2020

AnthonyYates commented Jul 7, 2020

martincostello commented Jul 7, 2020

martincostello commented Jul 7, 2020

martincostello commented Jul 7, 2020

AnthonyYates commented Jul 7, 2020

AnthonyYates commented Jul 7, 2020

kevinchalet commented Jul 7, 2020

martincostello commented Jul 7, 2020

Add SuperOffice provider #417

Add SuperOffice provider #417

Conversation

AnthonyYates commented May 4, 2020

AnthonyYates left a comment

Choose a reason for hiding this comment

kevinchalet commented May 6, 2020

AnthonyYates commented Jul 2, 2020 • edited Loading

martincostello commented Jul 2, 2020 • edited Loading

AnthonyYates commented Jul 2, 2020

AnthonyYates commented Jul 5, 2020

martincostello commented Jul 5, 2020

AnthonyYates commented Jul 5, 2020

martincostello commented Jul 5, 2020

kevinchalet commented Jul 5, 2020

martincostello commented Jul 5, 2020

AnthonyYates commented Jul 5, 2020

martincostello commented Jul 6, 2020

AnthonyYates commented Jul 7, 2020

martincostello commented Jul 7, 2020

martincostello commented Jul 7, 2020

martincostello commented Jul 7, 2020

AnthonyYates commented Jul 7, 2020

AnthonyYates commented Jul 7, 2020

kevinchalet commented Jul 7, 2020

martincostello commented Jul 7, 2020

AnthonyYates commented Jul 2, 2020 •

edited

Loading

martincostello commented Jul 2, 2020 •

edited

Loading