Client authentication filter #64

tomvandenberge · 2020-04-23T09:22:01Z

This adds a rather simple filter for client authentication (#5) for the Token Endpoint. It can currently only be used for the Client Credentials Grant.

This PR contains some files that shouldn't be there. This is probably a result of my earlier failed attempts to squash/rebase for another PR. I hope it doesn't mess it up too badly. This PR should only contain the filter, the tests for the filter, and three exception classes.

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

Captain-P-Goldfish · 2020-04-26T17:31:57Z

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

+                if (!clientId.equals("myclientid") || !clientSecret.equals("myclientsecret")) {
+                    throw new BadCredentialsException("Invalid client credentials");
+                }


still pretty much hardcoded

Captain-P-Goldfish · 2020-04-26T17:43:49Z

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

+        }
+        return null;
+    }
+


the pitfals of mockito.
you will surely never get a request in which you will be able to get the client_secret as a parameter from the request-object. The client_secret is always encoded in the Authorization header.
Besides you wrote that you only implemented the client credentials grant.
https://tools.ietf.org/html/rfc6749#section-4.4.2
RFC6749 does neither define the client_id nor the client_secret in the "client credentials grant" "access token request".
The only required parameter is "grant_type"

@Captain-P-Goldfish Please see 4.4.2. Access Token Request

The client MUST authenticate with the authorization server as described in Section 3.2.1.

right but this does not nullify my issue does it?
The client_id and client_secret must be extracted from the Authorization-header not from the requests parameters

EDIT:
sorry forgot the other authentication methods from chapter 2.3

Captain-P-Goldfish · 2020-04-26T17:45:53Z

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

+    }
+
+    // Taken from BasicAuthenticationFilter (spring-security-web)
+    private String[] extractAndDecodeHeader(String header, HttpServletRequest request) {


why give the header name as parameter? At least I do only know one header ("Authorization") that may contain the content that is necessary for this method to operate successfully. If I am wrong please correct me.

Captain-P-Goldfish · 2020-04-26T17:47:37Z

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

+
+        String token = new String(decoded, getCredentialsCharset(request));
+
+        int delim = token.indexOf(":");


why not "split" and spare yourself the substring methods below?

Captain-P-Goldfish · 2020-04-26T17:48:46Z

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

+            throw new BadCredentialsException("Invalid basic authentication token");
+        }
+        return new String[] { token.substring(0, delim), token.substring(delim + 1) };
+    }


You got the same problem here as I mentioned in this pull request:
#63

jgrandja

Thanks for the PR @tomvandenberge. Please see my review comments.

Also, can you please squash commits and force push on next update.

build.gradle

gradle/wrapper/gradle-wrapper.properties

gradlew

gradlew.bat

settings.gradle

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

jgrandja · 2020-04-27T11:29:19Z

src/main/java/sample/ClientCredentialsAuthenticationFilter.java

+        }
+        return null;
+    }
+


@Captain-P-Goldfish Please see 4.4.2. Access Token Request

The client MUST authenticate with the authorization server as described in Section 3.2.1.

src/main/java/sample/InvalidRequestException.java

src/test/java/sample/ApplicationTest.java

tomvandenberge · 2020-04-29T13:19:44Z

@jgrandja I'm struggling with how to correctly respond to your review comments. I'm not very familiar with working with pull requests and reviews in github. I think I've resolved the things you've mentioned, and committed and pushed them. But I'm really not sure if you're able to see them now. I have been looking for some more details on the reviewing process, but couldn't find anything useful. Please let met know.

jgrandja · 2020-05-01T17:15:52Z

@tomvandenberge I'm still seeing some files that should have been removed as per my last review comments.

Can you be more specific on the struggles you are having?

Have you reviewed contributing, as it provides links to working with pull requests, squashing commits using interactive rebasing, etc.

Please let me know exactly what you need help with and I'll point you in the right direction.

tomvandenberge · 2020-05-01T18:57:52Z

@jgrandja I don't understand why you want me to remove e.g. build.gradle, and the other gradle files. Without these, gradle doesn't work, or am I missing something?

tomvandenberge · 2020-05-01T19:08:32Z

@jgrandja Thanks for pointing me to these docs. I'm having a better understanding of rebasing and squashing now, and I'll try it with my next changes. My previous attempt to squash commits in Intellij was a disaster ;-)

Is it recommended to rebase (instead of merge) when pulling updates from the upstream repository?

jgrandja · 2020-05-01T21:34:32Z

@tomvandenberge

I don't understand why you want me to remove e.g. build.gradle, and the other gradle files

The project has been setup with gradle multi modules via 5ed7c8f. And the sample project already has a gradle file

Is it recommended to rebase (instead of merge)

Yes, please rebase on top of master

tomvandenberge · 2020-05-02T10:49:56Z

@jgrandja it must be my lack of experience with gradle. When I remove gradlew, gradle/wrapper/gradle-wrapper.properties and settings.gradle as per your comments, I can't run gradle anymore.
The build instructions also explicitly state that there should be a gradlew in the root of the source.

Toerktumlare · 2020-05-02T16:05:34Z

@tomvandenberge what they mean is that you should remove the files from the commit, not from your harddrive itself. You still need them localy to run your build, but they should not be commited in your PR.

tomvandenberge · 2020-05-02T17:04:06Z

@Tandolf Thanks for your clarification. I'm afraid my next question is how do I remove specific files from a commit?

Toerktumlare · 2020-05-03T11:36:14Z

@tomvandenberge you use the command git rm. Git rm on stack overflow And i believe for you to understand how you ended up in this situation. Before you commit you should always check with the git status command what git wants to commit. If git wants to commit files you havn't edited, you should look into why this is so.

You should never commit files you havn't touched. My belive is that you are facing the "CRLF" issue. If you have no idea what im talking about i recommend you reading up on what "CR" and "LF" is, the problems with it and how git can handle it, because it is too long to explain here.

tomvandenberge · 2020-05-03T17:35:20Z

@Tandolf These files were not added or modified accidentally. I've initially added them for another PR, and while I was working on this one, somebody changed them. I merged these upstream changes into my branch, which I maybe shouldn't have done (?), or maybe I should have used rebasing instead of merging when I did this, I don't know if that would have made a difference.

I've tried to remove the files with git rm as you said (isn't this exactly the same as removing the files locally, and then committing that?). Trying to push this to my origin branch didn't work. Apparently I had to pull first. After a lot of hassling, the files were no longer in my repository. Then the builds in the upstream Spring repo failed, because the gradle files were no longer there (which makes sense in my view, because git rm removed them). To fix this, I added them again, which brings me back to square one again.

No doubt that the git workflow I'm using is causing these problems. That's what probably got me into this mess. Any help to get me out of it is greatly appreciated!

jgrandja · 2020-05-04T18:12:25Z

@tomvandenberge

When I remove gradlew, gradle/wrapper/gradle-wrapper.properties and settings.gradle as per your comments, I can't run gradle anymore.

The gradle script already exists in the project root. You can run the build (from project root) using ./gradlew build, which will build ALL the modules. Or you can run a specific module, eg. ./gradlew :spring-authorization-server-samples-boot-minimal:build.

I'm seeing 20 commits in this PR, which most of them are in master. I think the best approach to get back on track with this PR, is to start from master and then manually add your changes into 1 commit. Then you can --force push the branch to bring this PR back to 1 commit on top of master. Any subsequent changes should be rebased (not merged) on top of master.

tomvandenberge · 2020-05-04T19:56:19Z

@jgrandja

The gradle script already exists in the project root.

I'm aware of that (assuming you mean gradlew). My PR also has this file in this location (in this comment I try to explain how it ended up there). In your comments you ask me to remove it. It's still not clear to me what exactly how you want me to this (or why). The same applies to the other gradle-related files.

I think the best approach to get back on track with this PR, is to start from master and then manually add your changes into 1 commit.

I'm afraid I don't understand this. What do you mean exactly by "to start from master"? My changes are already in my master branch, and therefore in the PR, so I can't add them again. Or is there some way to "reset" the PR?

(I start to realize that some of the problems I'm facing might be caused by the fact that I'm working from my master branch, and not a feature branch)

Thanks for you patience ;-)

jgrandja · 2020-05-05T20:31:41Z

@tomvandenberge

What do you mean exactly by "to start from master"?

I mean start from origin master - the master branch in this repository.

I'm working from my master branch, and not a feature branch

It's recommended to work in a feature branch for each PR you plan on submitting.

tomvandenberge · 2020-05-05T20:50:03Z

@jgrandja

I mean start from origin master - the master branch in this repository.

What should I do in this branch? Could you please be more specific? As I said, it already contains my changes.

jgrandja · 2020-05-05T21:24:55Z

@tomvandenberge I'm suggesting that you create a feature branch and submit that branch for the PR. You could close the existing PR after you submit the new one. I just feel it's easier with this approach as your master branch is way off with origin master. You will need to git reset --hard your master to origin master after you submit the new PR (with new branch) to sync up your master.

Create a new feature branch:

git checkout -b gh-5-client-auth origin/master

** Assuming origin is the name of the remote in your git setup.

Now, manually apply the changes from your master to gh-5-client-auth branch. Please ensure you don't copy any of the gradle related files. The only changes should be related to the Filter.

After you manually apply the changes then commit.

At this point you will have 1 commit that sits on top of latest commit from origin/master.

Now submit a new PR based on this branch. Close the other PR.

All changes going forward should be rebased on (current) origin/master.

dfcoffin · 2020-05-05T21:34:16Z

@tomvandenberge @jgrandja

I mean start from origin master - the master branch in this repository.

I suspect Joe is asking you to start over after reading from the master branch. An approach you might consider doing the following steps:

git remote add upstream https://github.com/spring-projects-experimental/spring-authorization-server
git checkout repair
git rebase upstream master

Then update the new branch with your code changes. Once the changes use:

git add <file_name> Note: wildcard "*" values can be used git commit -m "msg"

That way you will retain the files being asked to remove from the submission in your local repository as "untracked" files

Once all of the above is done issue a PR:

git pull upstream master

The above will get conflicting issues corrected.

dfcoffin · 2020-05-05T21:39:05Z

@tomvandenberge

If it helps we can conference and pair to help resolve the issue

tomvandenberge · 2020-05-05T21:43:29Z

@dfcoffin

If it helps we can conference and pair to help resolve the issue

That would be very helpful! Can I send you a message on [email protected] to get this started?

tomvandenberge · 2020-05-05T22:30:11Z

@jgrandja

I'm suggesting that you create a feature branch and submit that branch for the PR. You could
close the existing PR after you submit the new one. I just feel it's easier with this approach as
your master branch is way off with origin master

A new branch sounds like the most practical way forward, I agree. What I don't understand is why you think that my master branch is off with origin master. They are identical. origin/master is the remote master branch of my fork. Did you mean upstream/master?

Because my master and origin/master are in sync, creating a new feature branch as you suggest will result in a branch containing all the mess that's currently in my origin/master. Therefore I think that the approach you suggest will not work.

jgrandja · 2020-05-05T23:06:50Z

@tomvandenberge

What I don't understand is why you think that my master branch is off with origin master. They are identical....Did you mean upstream/master?

Yes, I mean upstream/master. Your master branch has 40 commits, whereas upstream/master has 29 commits. Your master branch has a few "merge" commits, which is causing part of this issue.

dfcoffin · 2020-05-06T01:59:14Z

@tomvandenberge

That would be very helpful! Can I send you a message on [email protected] to get this started?

Yes. I'm in Georgia to help set up a time we are both available

tomvandenberge marked this pull request as ready for review April 24, 2020 06:04

Toerktumlare reviewed Apr 24, 2020

View reviewed changes

src/main/java/sample/ClientCredentialsAuthenticationFilter.java Outdated Show resolved Hide resolved

Captain-P-Goldfish suggested changes Apr 26, 2020

View reviewed changes

jgrandja requested changes Apr 27, 2020

View reviewed changes

jgrandja self-assigned this Apr 27, 2020

pkostrzewa mentioned this pull request Apr 27, 2020

Implement Client Authentication #39

Closed

tomvandenberge requested a review from jgrandja April 28, 2020 20:48

tomvandenberge closed this May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client authentication filter #64

Client authentication filter #64

tomvandenberge commented Apr 23, 2020

Captain-P-Goldfish Apr 26, 2020

Captain-P-Goldfish Apr 26, 2020

jgrandja Apr 27, 2020

Captain-P-Goldfish Apr 27, 2020 •

edited

Loading

Captain-P-Goldfish Apr 26, 2020

Captain-P-Goldfish Apr 26, 2020

Captain-P-Goldfish Apr 26, 2020

jgrandja left a comment •

edited

Loading

jgrandja Apr 27, 2020

tomvandenberge commented Apr 29, 2020

jgrandja commented May 1, 2020

tomvandenberge commented May 1, 2020

tomvandenberge commented May 1, 2020

jgrandja commented May 1, 2020

tomvandenberge commented May 2, 2020

Toerktumlare commented May 2, 2020 •

edited

Loading

tomvandenberge commented May 2, 2020

Toerktumlare commented May 3, 2020 •

edited

Loading

tomvandenberge commented May 3, 2020

jgrandja commented May 4, 2020

tomvandenberge commented May 4, 2020 •

edited

Loading

jgrandja commented May 5, 2020 •

edited

Loading

tomvandenberge commented May 5, 2020 •

edited

Loading

jgrandja commented May 5, 2020

dfcoffin commented May 5, 2020

dfcoffin commented May 5, 2020

tomvandenberge commented May 5, 2020

tomvandenberge commented May 5, 2020

jgrandja commented May 5, 2020

dfcoffin commented May 6, 2020


		String token = new String(decoded, getCredentialsCharset(request));

		int delim = token.indexOf(":");

Client authentication filter #64

Client authentication filter #64

Conversation

tomvandenberge commented Apr 23, 2020

Captain-P-Goldfish Apr 26, 2020

Choose a reason for hiding this comment

Captain-P-Goldfish Apr 26, 2020

Choose a reason for hiding this comment

jgrandja Apr 27, 2020

Choose a reason for hiding this comment

Captain-P-Goldfish Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Captain-P-Goldfish Apr 26, 2020

Choose a reason for hiding this comment

Captain-P-Goldfish Apr 26, 2020

Choose a reason for hiding this comment

Captain-P-Goldfish Apr 26, 2020

Choose a reason for hiding this comment

jgrandja left a comment • edited Loading

Choose a reason for hiding this comment

jgrandja Apr 27, 2020

Choose a reason for hiding this comment

tomvandenberge commented Apr 29, 2020

jgrandja commented May 1, 2020

tomvandenberge commented May 1, 2020

tomvandenberge commented May 1, 2020

jgrandja commented May 1, 2020

tomvandenberge commented May 2, 2020

Toerktumlare commented May 2, 2020 • edited Loading

tomvandenberge commented May 2, 2020

Toerktumlare commented May 3, 2020 • edited Loading

tomvandenberge commented May 3, 2020

jgrandja commented May 4, 2020

tomvandenberge commented May 4, 2020 • edited Loading

jgrandja commented May 5, 2020 • edited Loading

tomvandenberge commented May 5, 2020 • edited Loading

jgrandja commented May 5, 2020

dfcoffin commented May 5, 2020

dfcoffin commented May 5, 2020

tomvandenberge commented May 5, 2020

tomvandenberge commented May 5, 2020

jgrandja commented May 5, 2020

dfcoffin commented May 6, 2020

Captain-P-Goldfish Apr 27, 2020 •

edited

Loading

jgrandja left a comment •

edited

Loading

Toerktumlare commented May 2, 2020 •

edited

Loading

Toerktumlare commented May 3, 2020 •

edited

Loading

tomvandenberge commented May 4, 2020 •

edited

Loading

jgrandja commented May 5, 2020 •

edited

Loading

tomvandenberge commented May 5, 2020 •

edited

Loading