Feature/support dynamic registration management rfc7591 rfc7592 #1

[mqf20]

Resolves #763

Definition of Ready

• I am happy with the code
• Short description of the feature/issue is added in the pr description
• PR is linked to the corresponding user story
• Acceptance criteria are met
• All open todos and follow ups are defined in a new ticket and justified
• Deviations from the acceptance criteria and design are agreed with the PO and documented.
• No debug or dead code
• My code has no repetitions
• Critical parts are tested automatically
• Where possible E2E tests are implemented
• Documentation/examples are up-to-date
• All non-functional requirements are met
• Functionality of the acceptance criteria is checked manually on the dev system.

[zekth]

this would accept Authorization: Bearerfoobar 1111
suggestion:

func getBearerToken(r *http.Request) (string, error) {
	authHeader := r.Header.Get("authorization")
	if len(authHeader) == 0 {
		return "", errMissingAuthorizationHeader
	}
	tokenSlice := strings.Split(authHeader[0], " ")
	if tokenSlice[0] != "Bearer" {
		return "", errors.New("unsupported auth method")
	} else if len(tokenSlice) != 2 {
		return "", errInvalidHeader
	}
	return tokenSlice[1], nil
}

[mqf20]

this would accept Authorization: Bearerfoobar 1111

getBearerToken checks against oidc.PrefixBearer:

	if !strings.HasPrefix(auth, oidc.PrefixBearer) {
		http.Error(w, "invalid header", http.StatusUnauthorized)
		return false, ""
	}

In other words,

# not accepted

Authorization: Bearerfoobar 1111

# accepted (returns `foobar 1111`)

Authorization: Bearer foobar 1111

getBearerToken is modelled after checkToken:

func checkToken(w http.ResponseWriter, r *http.Request) (bool, string) {
	auth := r.Header.Get("authorization")
	if auth == "" {
		http.Error(w, "auth header missing", http.StatusUnauthorized)
		return false, ""
	}
	if !strings.HasPrefix(auth, oidc.PrefixBearer) {
		http.Error(w, "invalid header", http.StatusUnauthorized)
		return false, ""
	}
	return true, strings.TrimPrefix(auth, oidc.PrefixBearer)
}

[zekth]

My bad wasn't explicit enough; i was more meaning about error checking like differenciating between protocols. Bearer Basic and so on.

[mqf20]

Thanks for clarifying, I didn't read about Dynamic Client Registration supporting basic auth, is this something we have to support?

[zekth]

No but for example okta uses SSWS, depends if we want to make it generic or not.

[mqf20]

@zekth I'm fine if you would like to add to this PR, or submit a follow-up PR

[mqf20]

Would a reviewer like to weigh in on this? @elinashoko

[elinashoko]

Heya @mqf20 thanks for your contribution - we'll review this as soon as we can, currently working through a lot of contributions.

[lukaslihotzki-f]

This approach looks good. I especially like the modeling of the JSON schema as Go types (for example, InternationalizedField).

[lukaslihotzki-f]

To support open registration and facilitate wider interoperability,
the client registration endpoint SHOULD allow registration requests
with no authorization (which is to say, with no initial access token
in the request). These requests MAY be rate-limited or otherwise
limited to prevent a denial-of-service attack on the client
registration endpoint.

This comment contradicts RFC 7591 (section 3.) by implying Protected Dynamic Client Registration. Implementing Open Dynamic Client Registration should be possible by always returning nil.

[mqf20]

@lukaslihotzki-f I'm not sure I follow your point - would you like to suggest a fix?

[lukaslihotzki-f]

This PR only adds the routes in op.go, but not in http_server.go. I don't know why there are two routers with separate handler functions, but Zitadel seems to use http_server.go. Otherwise, when configuring a registration endpoint, Zitadel only returns 404 on the advertised registration endpoint. I used this commit to add the route to http_server.go, which works: famedly@a38fab7 This commit implements the new interface in Zitadel: famedly/zitadel@c747723

Is there a way to use the existing interface (ClientsStorage) in Zitadel? Otherwise, all routes should also be added to http_server.go.

[muhlemmer]

Please take note of #785

[mqf20]

Please take note of #785

@muhlemmer any chance this PR can be looked at in an upcoming sprint?

[mqf20]

This PR only adds the routes in op.go, but not in http_server.go. I don't know why there are two routers with separate handler functions, but Zitadel seems to use http_server.go. Otherwise, when configuring a registration endpoint, Zitadel only returns 404 on the advertised registration endpoint. I used this commit to add the route to http_server.go, which works: famedly@a38fab7 This commit implements the new interface in Zitadel: famedly/zitadel@c747723

Is there a way to use the existing interface (ClientsStorage) in Zitadel? Otherwise, all routes should also be added to http_server.go.

@lukaslihotzki-f do you mean all routes should be added to server_http.go (instead of http_server.go)?

Also, will you be submitting a PR for your fix?