Make client secret optional while parsing basic authentication secret

[AliBazzi]

This PR add feature request in #2267 ticket

It doesn't introduce breaking change.

[brockallen]

It doesn't introduce breaking change.

Except the tests?

[AliBazzi]

Sorry didn't get it, can you elaborate more ?

[brockallen]

Sorry didn't get it, can you elaborate more ?

Well, look at the status:

Some checks were not successful
1 failing and 1 successful checks
continuous-integration/appveyor/pr — AppVeyor build failed

[AliBazzi]

Oh yes, I noticed that, but it looks like it's related to Ubuntu where it failed to compile the integration test project, the windows image with visual studio passed all tests

I don't think the modified unit test broke that project

[brockallen]

Ah yes, you're right -- I hadn't followed the link to know it was the unbutu build. I guess something environmental.

[AliBazzi]

This is my second PR to Identity server, and both of them were very minimal changes, but my bad luck jumped in the verification process in both PR, the earlier one failed mysteriously as well

[brockallen]

No worries. We'll have a look. Thanks

[AliBazzi]

Welcome :)

[leastprivilege]

Thanks!

Could you also add an integration test - basically a client with empty secret calling into the token endpoint - I want to make sure, nothing breaks later in the pipeline because of a null secret.

[AliBazzi]

Sure, working on it :)

[AliBazzi]

@leastprivilege please take a look at the integration test when you have time.

[leastprivilege]

You also need a test for an empty secret I guess - not just RequireClientSecret = false.

[AliBazzi]

So to verify the assertion of the test with you:
define a client with a secret as empty string, and this client configuration have RequireClientSecret set to false
The test will verify that it pass with the expected payload by passing the client Id and the empty client secret as basic authentication style.

Correct me if I'm wrong, and thanks for the help in advance.

[leastprivilege]

Add a client with require secret = true but an empty secret - see if that works as expected.

[AliBazzi]

Hi @leastprivilege

After investigating the code, I think the semantic of empty secret is not supported currently, what is supported is a client that has RequireClientSecret set to false.
When that property is set to false it was breaking client id and empty secrets passed in the Basic Authorization header, this PR aimed to fix this bug in BasicAuthenticationSecretParser, so adding a test to verify that a client secret empty + RequireClientSecret set to True will not pass due to current implementation.

Please advice on the next step and correct me please if I'm wrong.

[leastprivilege]

You are probably right - will have a look and merge later...thanks!

[leastprivilege]

thanks!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.