fix: scope parameter ignored in refresh flow (#5)

Author: james-d-elliott

README.md

@@ -2,13 +2,13 @@
 
 This is a hard fork of [ORY Fosite](https://github.com/ory/fosite) under the
 [Apache 2.0 License](LICENSE) for the purpose of performing self-maintenance of
-this critical dependency.
+this critical Authelia dependency.
 
 We however:
 
 - Acknowledge the amazing hard work of the ORY developers in making such an
   amazing framework that we can do this with.
-- Plan to continue to contribute back to te ORY Fosite and related projects.
+- Plan to continue to contribute back to te ORY fosite and related projects.
 - Have ensured the licensing is unchanged in this fork of the library.
 - Do not have a formal affiliation with ORY and individuals utilizing this
   library should not allow their usage to be a reflection on ORY as this library
@@ -21,18 +21,33 @@ following list of differences:
 
 - [x] Module path changed from `github.com/ory/fosite` to
       `authelia.com/provider/oauth2`.
+- Documentation:
+  - [ ] Add spec support documentation
+- Overhaul testing:
+  - [ ] Ensure all tests and subtests are well named
+  - [ ] Ensure all tests are simplified where possible
+  - [ ] Restore/Implement conformance tests
 - Rename interfaces and implementations:
   - [x] `OAuth2Provider` to `Provider`.
   - [ ] `Fosite` to `TBA`.
-- [x] Minimum dependency is go version 1.21.
+- [x] Minimum dependency is go version 1.21
+- [ ] Replace string values with constants where applicable
+- [ ] Simplify the internal JWT logic to leverage `github.com/golang-jwt/jwt/v5`
+- [ ] Implement internal JWKS logic
 - Fixes:
   - [x] Basic Scheme Rejects Special Characters
-        2314625eb1f21987a9199fb1cdf6da6cee4df965
-  - [x] RFC9068 must condition ignored f4652d60c850d167da00e2d2fe9096776eff9465
-  - [ ] Refresh Flow ignores requested scope
-  - [ ] Refresh Flow does not set original request ID early enough
-  - [ ] PKCE Flow session generated needlessly
-  - [ ] OpenID Flows ignore empty redirect uri
+        <sup>[commit](https://github.com/authelia/oauth2-provider/commit/2314625eb1f21987a9199fb1cdf6da6cee4df965)</sup>
+  - [x] RFC9068 must condition ignored
+        <sup>[commit](https://github.com/authelia/oauth2-provider/commit/f4652d60c850d167da00e2d2fe9096776eff9465)</sup>
+  - Refresh Flow:
+    - [x] Requested scope ignored
+    - [x] Original request id not set early enough
+  - PKCE Flow
+    - [ ] Session generated needlessly
+    - [ ] Failure to fetch session causes an error even when not enforced
+  - OpenID Flows:
+    - [x] Absence of Redirect URI does not result in an error
+          <sup>[commit](https://github.com/authelia/oauth2-provider/commit/f4652d60c850d167da00e2d2fe9096776eff9465)</sup>
   - [ ] Decode id_token_hint with correct signer
   - [ ] Write Revocation Response does not correctly error
   - [ ] Invalid Token base 64 error not mapped to RFC
@@ -42,6 +57,7 @@ following list of differences:
 - Features:
   - [ ] Customizable Token Prefix
   - [ ] JWE support for Client Authentication and Issuance
+  - [ ] UserInfo support
   - [ ] JARM support
   - [ ] Revocation Flow per policy can decide to revoke Refresh Tokens on
         request
@@ -63,11 +79,6 @@ following list of differences:
   - [x] `github.com/form3tech-oss/jwt-go`
   - [x] `github.com/dgrijalva/jwt-go`
 - Migration of the following dependencies:
+  - [ ] `github.com/go-jose/go-jose/v3` => `github.com/golang-jwt/jwt/v5`
   - [x] `github.com/golang/mock` => `github.com/uber-go/mock`
   - [x] `github.com/cristalhq/jwt/v4` => `github.com/golang-jwt/jwt/v5`
-
-## TODO
-
-- Consolidate JWT and JOSE dependencies
-- Remove unecessary dependencies and/or abstract them
-- Apply downstream fixes

access_error_test.go

@@ -34,7 +34,7 @@ func TestWriteAccessError(t *testing.T) {
 }
 
 func TestWriteAccessError_RFC6749(t *testing.T) {
-	// https://tools.ietf.org/html/rfc6749#section-5.2
+	// https://datatracker.ietf.org/doc/html/rfc6749#section-5.2
 
 	config := new(Config)
 	provider := &Fosite{Config: config}

access_request.go

@@ -23,3 +23,22 @@ func NewAccessRequest(session Session) *AccessRequest {
 func (a *AccessRequest) GetGrantTypes() Arguments {
 	return a.GrantTypes
 }
+
+func (a *AccessRequest) SetGrantedScopes(scopes Arguments) {
+	a.GrantedScope = scopes
+}
+
+func (a *AccessRequest) SanitizeRestoreRefreshTokenOriginalRequester(requester Requester) Requester {
+	r := a.Sanitize(nil).(*Request)
+
+	ar := &AccessRequest{
+		Request: *r,
+	}
+
+	ar.SetID(requester.GetID())
+
+	ar.SetRequestedScopes(requester.GetRequestedScopes())
+	ar.SetGrantedScopes(requester.GetGrantedScopes())
+
+	return ar
+}

access_request_handler.go

@@ -15,7 +15,7 @@ import (
 )
 
 // Implements
-//   - https://tools.ietf.org/html/rfc6749#section-2.3.1
+//   - https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1
 //     Clients in possession of a client password MAY use the HTTP Basic
 //     authentication scheme as defined in [RFC2617] to authenticate with
 //     the authorization server.  The client identifier is encoded using the
@@ -31,7 +31,7 @@ import (
 //     password-based HTTP authentication schemes).  The parameters can only
 //     be transmitted in the request-body and MUST NOT be included in the
 //     request URI.
-//   - https://tools.ietf.org/html/rfc6749#section-3.2.1
+//   - https://datatracker.ietf.org/doc/html/rfc6749#section-3.2.1
 //   - Confidential clients or other clients issued client credentials MUST
 //     authenticate with the authorization server as described in
 //     Section 2.3 when making requests to the token endpoint.

access_response.go

@@ -6,6 +6,8 @@ package oauth2
 import (
 	"strings"
 	"time"
+
+	"authelia.com/provider/oauth2/internal/consts"
 )
 
 func NewAccessResponse() *AccessResponse {
@@ -21,11 +23,11 @@ type AccessResponse struct {
 }
 
 func (a *AccessResponse) SetScopes(scopes Arguments) {
-	a.SetExtra("scope", strings.Join(scopes, " "))
+	a.SetExtra(consts.AccessResponseScope, strings.Join(scopes, " "))
 }
 
 func (a *AccessResponse) SetExpiresIn(expiresIn time.Duration) {
-	a.SetExtra("expires_in", int64(expiresIn/time.Second))
+	a.SetExtra(consts.AccessResponseExpiresIn, int64(expiresIn/time.Second))
 }
 
 func (a *AccessResponse) SetExtra(key string, value any) {
@@ -53,7 +55,7 @@ func (a *AccessResponse) GetTokenType() string {
 }
 
 func (a *AccessResponse) ToMap() map[string]any {
-	a.Extra["access_token"] = a.GetAccessToken()
-	a.Extra["token_type"] = a.GetTokenType()
+	a.Extra[consts.AccessResponseAccessToken] = a.GetAccessToken()
+	a.Extra[consts.AccessResponseTokenType] = a.GetTokenType()
 	return a.Extra
 }

access_response_test.go

@@ -9,20 +9,21 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	. "authelia.com/provider/oauth2"
+	"authelia.com/provider/oauth2/internal/consts"
 )
 
 func TestAccessResponse(t *testing.T) {
 	ar := NewAccessResponse()
 	ar.SetAccessToken("access")
-	ar.SetTokenType("bearer")
-	ar.SetExtra("access_token", "invalid")
+	ar.SetTokenType(BearerAccessToken)
+	ar.SetExtra(consts.AccessResponseAccessToken, "invalid")
 	ar.SetExtra("foo", "bar")
 	assert.Equal(t, "access", ar.GetAccessToken())
-	assert.Equal(t, "bearer", ar.GetTokenType())
+	assert.Equal(t, BearerAccessToken, ar.GetTokenType())
 	assert.Equal(t, "bar", ar.GetExtra("foo"))
 	assert.Equal(t, map[string]any{
-		"access_token": "access",
-		"token_type":   "bearer",
-		"foo":          "bar",
+		consts.AccessResponseAccessToken: "access",
+		consts.AccessResponseTokenType:   BearerAccessToken,
+		"foo":                            "bar",
 	}, ar.ToMap())
 }

authorize_error_test.go

@@ -18,13 +18,13 @@ import (
 )
 
 // Test for
-//   - https://tools.ietf.org/html/rfc6749#section-4.1.2.1
+//   - https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1
 //     If the request fails due to a missing, invalid, or mismatching
 //     redirection URI, or if the client identifier is missing or invalid,
 //     the authorization server SHOULD inform the resource owner of the
 //     error and MUST NOT automatically redirect the user-agent to the
 //     invalid redirection URI.
-//   - https://tools.ietf.org/html/rfc6749#section-3.1.2
+//   - https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2
 //     The redirection endpoint URI MUST be an absolute URI as defined by
 //     [RFC3986] Section 4.3.  The endpoint URI MAY include an
 //     "application/x-www-form-urlencoded" formatted (per Appendix B) query

authorize_helper.go

@@ -36,7 +36,7 @@ var DefaultFormPostTemplate = template.Must(template.New("form_post").Parse(`<ht
 //
 // Considered specifications
 //
-//   - https://tools.ietf.org/html/rfc6749#section-3.1.2.3
+//   - https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.3
 //     If multiple redirection URIs have been registered, if only part of
 //     the redirection URI has been registered, or if no redirection URI has
 //     been registered, the client MUST include a redirection URI with the
@@ -50,7 +50,7 @@ var DefaultFormPostTemplate = template.Must(template.New("form_post").Parse(`<ht
 //     redirection URI, the authorization server MUST compare the two URIs
 //     using simple string comparison as defined in [RFC3986] Section 6.2.1.
 //
-// * https://tools.ietf.org/html/rfc6819#section-4.4.1.7
+// * https://datatracker.ietf.org/doc/html/rfc6819#section-4.4.1.7
 //   - The authorization server may also enforce the usage and validation
 //     of pre-registered redirect URIs (see Section 5.2.3.5).  This will
 //     allow for early recognition of authorization "code" disclosure to
@@ -87,7 +87,7 @@ func MatchRedirectURIWithClientRedirectURIs(rawurl string, client Client) (*url.
 // an IPv6 URI http://[::1] a client is allowed to request a dynamic port and the server MUST accept
 // it as a valid redirection uri.
 //
-// https://tools.ietf.org/html/rfc8252#section-7.3
+// https://datatracker.ietf.org/doc/html/rfc8252#section-7.3
 // Native apps that are able to open a port on the loopback network
 // interface without needing special permissions (typically, those on
 // desktop operating systems) can use the loopback interface to receive
@@ -127,7 +127,7 @@ func isMatchingAsLoopback(requested *url.URL, registeredURI string) bool {
 	// Loopback redirect URIs use the "http" scheme and are constructed with
 	// the loopback IP literal and whatever port the client is listening on.
 	//
-	// Source: https://tools.ietf.org/html/rfc8252#section-7.3
+	// Source: https://datatracker.ietf.org/doc/html/rfc8252#section-7.3
 	if requested.Scheme == "http" &&
 		isLoopbackAddress(requested.Host) &&
 		registered.Hostname() == requested.Hostname() &&
@@ -152,12 +152,12 @@ func isLoopbackAddress(address string) bool {
 
 // IsValidRedirectURI validates a redirect_uri as specified in:
 //
-// * https://tools.ietf.org/html/rfc6749#section-3.1.2
+// * https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2
 //   - The redirection endpoint URI MUST be an absolute URI as defined by [RFC3986] Section 4.3.
 //   - The endpoint URI MUST NOT include a fragment component.
-//   - https://tools.ietf.org/html/rfc3986#section-4.3
+//   - https://datatracker.ietf.org/doc/html/rfc3986#section-4.3
 //     absolute-URI  = scheme ":" hier-part [ "?" query ]
-//   - https://tools.ietf.org/html/rfc6819#section-5.1.1
+//   - https://datatracker.ietf.org/doc/html/rfc6819#section-5.1.1
 func IsValidRedirectURI(redirectURI *url.URL) bool {
 	// We need to explicitly check for a scheme
 	if !urls.IsRequestURL(redirectURI.String()) {

authorize_request_handler.go

@@ -200,7 +200,7 @@ func (f *Fosite) validateAuthorizeScope(ctx context.Context, _ *http.Request, re
 }
 
 func (f *Fosite) validateResponseTypes(r *http.Request, request *AuthorizeRequest) error {
-	// https://tools.ietf.org/html/rfc6749#section-3.1.1
+	// https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.1
 	// Extension response types MAY contain a space-delimited (%x20) list of
 	// values, where the order of values does not matter (e.g., response
 	// type "a b" is the same as "b a").  The meaning of such composite
@@ -414,7 +414,7 @@ func (f *Fosite) newAuthorizeRequest(ctx context.Context, r *http.Request, isPAR
 	// The "state" parameter should be used to link the authorization
 	// request with the redirect URI used to deliver the access token (Section 5.3.5).
 	//
-	// https://tools.ietf.org/html/rfc6819#section-4.4.1.8
+	// https://datatracker.ietf.org/doc/html/rfc6819#section-4.4.1.8
 	// The "state" parameter should not	be guessable
 	if len(request.State) < f.GetMinParameterEntropy(ctx) {
 		// We're assuming that using less then, by default, 8 characters for the state can not be considered "unguessable"

authorize_write.go

@@ -56,7 +56,7 @@ func (f *Fosite) WriteAuthorizeResponse(ctx context.Context, rw http.ResponseWri
 	}
 }
 
-// https://tools.ietf.org/html/rfc6749#section-4.1.1
+// https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1
 // When a decision is established, the authorization server directs the
 // user-agent to the provided client redirection URI using an HTTP
 // redirection response, or by other means available to it via the