node-oauth2-server icon indicating copy to clipboard operation
node-oauth2-server copied to clipboard

[authorize] should not use getClient(clientId: string, clientSecret: string)

Open whatwewant opened this issue 5 years ago • 12 comments

https://github.com/oauthjs/node-oauth2-server/blob/91d2cbe70a0eddc53d72def96864e2de0fd41703/lib/handlers/authorize-handler.js#L179

when it is in authorize mode, you wonnot send clientSecret to getClient, is it a bug ?

whatwewant avatar Aug 30 '20 10:08 whatwewant

#239

whatwewant avatar Sep 03 '20 17:09 whatwewant

I get the solution, but it is implicit.

async getClient(clientId: string, clientSecret: string | null) {
    if (clientSecret !== null) {
      //  token flow
      return models.getClientById(clientId, clientSecret)
    } else {
      // authorization code flow
      return models.getClient(clientId)
    }
}

i donot whether it is the right way, this is the valid solution for me.

whatwewant avatar Sep 03 '20 17:09 whatwewant

But, sometimes I want give the client more specific error message, like client secret wrong. It seems that oauth2-server has no way to do this.

whlsxl avatar Jan 16 '21 08:01 whlsxl

But, sometimes I want give the client more specific error message, like client secret wrong. It seems that oauth2-server has no way to do this.

@whlsxl

https://github.com/oauthjs/node-oauth2-server/blob/91d2cbe70a0eddc53d72def96864e2de0fd41703/lib/handlers/authorize-handler.js#L182

like this, just to throw your error in getClient method

whatwewant avatar Jan 18 '21 02:01 whatwewant

I'm stuck on this as well. How is it possible to implement standard Authorization code flow if the client secret is always required? It can't be securely passed in the initial login/redirect process, and is only required for the code->token exchange afterwards (secure, hidden).

Unfortunately the solution offered earlier isn't secure in that it allows the secret to be omitted while it assumes this is part of the auth code flow.

perry-mitchell avatar Apr 21 '21 11:04 perry-mitchell

@perry-mitchell maybe you just see this code without context. It is a safe solution.

Please see https://github.com/oauthjs/node-oauth2-server/blob/91d2cbe70a0eddc53d72def96864e2de0fd41703/lib/handlers/token-handler.js#L123

You will find the token flow will validate before calling the getClient

whatwewant avatar Apr 22 '21 05:04 whatwewant

This bugged me also since i wondered why my client is not sending any secret. It was because of the auth_code flow

NilsBaumgartner1994 avatar Nov 11 '21 10:11 NilsBaumgartner1994

@whatwewant @NilsBaumgartner1994 @perry-mitchell

This is actually a result of a bad implementation by oauth2-server. It expects that all Clients are either public or confidential. https://datatracker.ietf.org/doc/html/rfc6749#section-2.1

If a client needs Credentials is not based on server wide allowed grants but on the Client instance. If a client has a secret, then you have to validate the secret. https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1

According to the spec, all Flows can get the Client Credentials in the Authorization Header in Basic-Auth Format. So you have to implement the getClient-Method like this:

async getClient(clientId: string, clientSecret: string | null) {
	const client = models.getClient(clientId);

	if (client.secret) {
		return ((client.secret === clientSecret)
			? client
			: undefined
		);
	}
	return client;
}

So you load the client, check if it has a secret assigned (I guessed it is the attribute "secret" in the implemented model). If it has a secret, we know it is a confidential client. We check if the secret is identical to the provided secret.

If the loaded client has no secret assigned, we can return it, as it is implicitly a public client.

Uzlopak avatar Nov 21 '21 02:11 Uzlopak

Sorry for my misleading description. I have an Client which routes to the oAuth Server with then gets back to directs which den Redirect to the client app with an access token in the url.

And now I need to setup the ask client with that token. And no, cookies are not possible in this scenario.

NilsBaumgartner1994 avatar Nov 21 '21 03:11 NilsBaumgartner1994

Sry mate, aber ich verstehe deine Ausführungen nicht.

Der Flow geht eigentlich so:

  1. Der User besucht den Resource Server und macht einen call auf eine geschützte Route.
  2. Resource Server erkennt dass die Route geschützt ist und redirected den User zum Authorization Server. Der Redirect enthält die Parameter im QueryString. https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1
  3. Ggf. loggt sich der User beim Authorization Server ein (er authentifiziert sich)
  4. Authorization Server erkennt den User und fragt ob man dem Resource Server Zugriff gibt (ja verwirrend, weil wir ja Zugriff auf die Resource wollen, aber es geht hier darum, dass wir dem AuthorizationServer sagen, welche berechtigungen wir nutzen wollen)
  5. Wenn man zustimmt, dann erstellt der Authorization Server einen Authorization Code.
  6. Der Authorization Server redirected einen zurück und der authorization code kann im query übergeben werden, https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2 bspw. http://www.resource.de/spa?authorization_code=XXX
  7. Der Client erkennt den authorization_code im query und tauscht nun den authorization_code gegen den Token aus, indem er diesen gegen den /token endpunkt vom AuthorizationServer spielt.
  8. Der Client hat nun den Token.

Da der AuthorizatioCode im Query übergeben werden kann, werden diese in den ServerLogs angezeigt und kann darum abgefangen werden. Man sollte also diese shortlived machen, also diese sollten maximal 10 Minuten gültig sein, besser 1 Minute oder noch kürzer.

Ob das jetzt genau bei oauth2-server so implementiert wurde, habe ich noch nicht testen können. Aber ich hoffe dass diese Ausführungen dir eine Idee geben, wie du es ohne Cookies hinkriegen könntest.

Uzlopak avatar Nov 21 '21 03:11 Uzlopak

@NilsBaumgartner1994

Nebenbei bemerkt, vielleicht willst du ja https://github.com/node-oauth/node-oauth2-server nutzen. Das ist ein Fork von diesem Projekt und unter aktiver Entwicklung.

Uzlopak avatar Nov 21 '21 03:11 Uzlopak

https://github.com/oauthjs/node-oauth2-server/blob/6d4c98794bc024a8cf93cf9e79f92f93766da9f4/lib/response-types/code-response-type.js#L33

Yep, nach dem Authorize hast du den Code im Query...

Uzlopak avatar Nov 21 '21 04:11 Uzlopak