Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #43—Callback url registered incorrectly if using route groups #102

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bgisme
Copy link

@bgisme bgisme commented Jul 16, 2024

Attempt to fix issue #43—Callback url registered incorrectly if using route groups.

It adds an optional parameter to ServiceRegister called grouped: [PathComponent]

oAuth<OAuthProvider>(
        grouped: [PathComponent] = [],
        ...

Can be used with group routes like this...

let abComponents: [PathComponent] = ["a", "b"]
let abRoutes = routes.grouped(abComponents)
try abRoutes.oAuth(
    grouped: abComponents,
    from: Google.self,
    authenticate: "start",
    callback: "http://localhost:8080/a/b/oauth/google",
    scope: ["profile", "email"],
    redirect: "/done")
    
 // call to trigger... http://localhost:8080/a/b/start

Ungroued routes can omit the parameter. So existing usage should not change.

The parameter is passed down to FederatedServiceRouter configureRoutes(grouped:withAuthURL:authenticateCallback:on:) where the callback URL gets registered.

When breaking the callbackURL into path components, duplicate components at the head of the path are removed. So the grouped route is registered properly and matches the callback url.

Most of the work happens inside ImperialCore > Helpers > String + Tools.swift > pathComponents(grouped:)

Also added a few tests for different kinds of groups and callback urls.

Happy to add readme stuff if you like the fix.

@@ -4,7 +4,20 @@ import XCTest
class ImperialTests: XCTestCase {
func testExists() {}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests for different kinds of grouped scenarios

public func configureRoutes(withAuthURL authURL: String, authenticateCallback: ((Request) throws -> (EventLoopFuture<Void>))?, on router: RoutesBuilder) throws {
router.get(callbackURL.pathComponents, use: callback)
router.get(authURL.pathComponents) { req -> EventLoopFuture<Response> in
public func configureRoutes(grouped: [PathComponent], withAuthURL authURL: String, authenticateCallback: ((Request) throws -> (EventLoopFuture<Void>))?, on router: RoutesBuilder) throws {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the new func pathComponents(grouped:) on both callbackURL and authURL

@@ -2,16 +2,31 @@ import Foundation
import RoutingKit

extension String {
Copy link
Author

@bgisme bgisme Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change pathComponents var to a func with grouped: [PathComponent] parameter.
Break url path into array of components like before.
Remove any duplicates with grouped at beginning of array.
Stop immediately when a component at the same index does not match.

@@ -6,6 +6,7 @@ public class DeviantArt: FederatedService {

@discardableResult
public required init(
grouped: [PathComponent],
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add parameter grouped: [PathComponent]
It gets passed down in to where the callback url is parsed into path components and registered.
Any overlap between callback url path components and grouped parameter is removed before registration.
So the grouped route matches the callback url.

@0xTim
Copy link
Member

0xTim commented Jul 31, 2024

Just an FYI I'm going to be doing an Imperial code sprint next week to go through all the issues and PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants