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

Remove authParams from getAuthorizeUrl #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gillyhl
Copy link

@gillyhl gillyhl commented Aug 4, 2022

This PR removes the need for authParams to be specified in the authorisation URL generated, considering the request object holds all that information already.

Copy link
Collaborator

@tomstrat tomstrat left a comment

Choose a reason for hiding this comment

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

Copy link

@ozamarripa ozamarripa left a comment

Choose a reason for hiding this comment

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

Has this been tested with the latest version of Identity service? just to make sure that it is not expecting anything in the auth params when the request object is present

Copy link

@ozamarripa ozamarripa left a comment

Choose a reason for hiding this comment

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

I tried generating an auth url and I can see that is adding scope only as openid in the auth url but the request object has all of the right scopes, will this not cause issues?
I can also see that is still adding client_id, response_type and redirect_url but those might be required according to the spec

https://identity.moneyhub.co.uk/oidc/auth?client_id=1e1b2556-6e29-426c-9a52-8cd5b9019c72&scope=openid&response_type=code&redirect_uri=https%3A%2F%2Fmoneyhub.com%2Fcallback&request=eyJhbGciOiJSUzUxMiIsInR5cCI6Im9hdXRoLWF1dGh6LXJlcStqd3QiLCJraWQiOiJzTUppX3JmQWt4THZNOWZkYUI2STFZOTBZeU4yRmFsVmpTaFJYa2R2OU9nIn0.eyJjbGllbnRfaWQiOiIxZTFiMjU1Ni02ZTI5LTQyNmMtOWE1Mi04Y2Q1YjkwMTljNzIiLCJzY29wZSI6Im9wZW5pZCBvZmZsaW5lX2FjY2VzcyBpZDoxZmZlNzA0ZDM5NjI5YTkyOWM4ZTI5Mzg4MGZiNDQ5YSBhY2NvdW50czpyZWFkIHRyYW5zYWN0aW9uczpyZWFkOmFsbCBjYXRlZ29yaWVzOnJlYWQgY2F0ZWdvcmllczp3cml0ZSBzcGVuZGluZ19nb2FsczpyZWFkIHNhdmluZ3NfZ29hbHM6cmVhZCIsInN0YXRlIjoiZm9vIiwibm9uY2UiOiJiYXIiLCJyZWRpcmVjdF91cmkiOiJodHRwczovL21vbmV5aHViLmNvbS9jYWxsYmFjayIsInJlc3BvbnNlX3R5cGUiOiJjb2RlIiwicHJvbXB0IjoiY29uc2VudCIsImNsYWltcyI6eyJpZF90b2tlbiI6eyJzdWIiOnsiZXNzZW50aWFsIjp0cnVlfSwibWg6Y29uX2lkIjp7ImVzc2VudGlhbCI6dHJ1ZX19fSwibWF4X2FnZSI6ODY0MDAsImlzcyI6IjFlMWIyNTU2LTZlMjktNDI2Yy05YTUyLThjZDViOTAxOWM3MiIsImF1ZCI6Imh0dHBzOi8vaWRlbnRpdHkubW9uZXlodWIuY28udWsvb2lkYyIsImp0aSI6ImRLcHRSZkktY3ZTTUxIbzdoNEN4UzQxNkl0TTY4Rk9JUmF0SjBoS0gzd1kiLCJpYXQiOjE2NjA3OTUzNzQsImV4cCI6MTY2MDc5NTY3NH0.EzKp9igDei4DB4T3oIPDjplfuei9n6iuV9PFWoPL2elv7WGnulMIMSnFZj9HhmIdqmZeDVhZJ3ImMhL4SwE-KsqULzkESc8gE-sn_ZnbcG5arh_xbUYuHk0cDWRnmBm_YEtsX3vU5aYvn4F4keLxrXvGsod9B3myNZo5d0CZQ8cFvwicQ_oPHntDso3UV5yVeldGC-2BqB_FM98LogYvoErgDauCZJ2qz0fIjQJGxI1tSgv65f4cqQaXAO5GTOpB2YUluYMZ0QD5exRUPXOciSYD0sW9qp8cyOmG6pUvriRhBE9LfbzsIX_REE4iZVWyQO94CKCl3NGGv-ePTqW8tg

Request object decoded
{
"client_id": "1e1b2556-6e29-426c-9a52-8cd5b9019c72",
"scope": "openid offline_access id:1ffe704d39629a929c8e293880fb449a accounts:read transactions:read:all categories:read categories:write spending_goals:read savings_goals:read",
"state": "foo",
"nonce": "bar",
"redirect_uri": "https://moneyhub.com/callback",
"response_type": "code",
"prompt": "consent",
"claims": {
"id_token": {
"sub": {
"essential": true
},
"mh:con_id": {
"essential": true
}
}
},
"max_age": 86400,
"iss": "1e1b2556-6e29-426c-9a52-8cd5b9019c72",
"aud": "https://identity.moneyhub.co.uk/oidc",
"jti": "dKptRfI-cvSMLHo7h4CxS416ItM68FOIRatJ0hKH3wY",
"iat": 1660795374,
"exp": 1660795674
}

@ozamarripamoneyhub
Copy link
Contributor

@gilberthl-mh @tomstrat do we know if this is still relevant or can we close this PR?

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

Successfully merging this pull request may close these issues.

5 participants