vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feat: build openid redirect url dynamically #1144

Merged
dpschen merged 3 commits from feat/openid-redirect into main 2021-12-17 15:41:12 +00:00
Conversation 2 Commits 3 Files Changed 3 +12 -5
konrad commented 2021-12-05 15:40:41 +00:00
Owner
Copy Link

Maybe resolves vikunja/desktop#70 but I'm not sure if it's a good idea.

This uses the current host as a redirect url instead of the one configured in the api.

Maybe resolves https://kolaente.dev/vikunja/desktop/issues/70 but I'm not sure if it's a good idea. This uses the current host as a redirect url instead of the one configured in the api.
konrad added 1 commit 2021-12-05 15:40:42 +00:00
continuous-integration/drone/pr Build is passing Details
e0941a4d82
feat: build openid redirect url dynamically
frederick commented 2021-12-05 15:45:49 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1144-featopenid-redirect--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1144-featopenid-redirect--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
konrad requested review from dpschen 2021-12-05 19:24:32 +00:00
dpschen reviewed 2021-12-06 13:42:44 +00:00
src/views/user/Login.vue Outdated
@ -222,1 +223,3 @@
redirectToProvider(provider, this.openidConnect.redirectUrl)
const {host, protocol} = parseURL(window.location.href)
const redirectUrl = `${protocol}//${host}/auth/openid/`
redirectToProvider(provider, redirectUrl)
dpschen commented 2021-12-06 13:42:37 +00:00
Member

Just to get this right:
By doing this we would ignore the redirectUrl defined in this.openidConnect completely?

And if so:

Shouldn't redirectToProviderIfNothingElseIsEnabled be changed accordingly and the prop deleted from the store, etc.?

Just to get this right: By doing this we would ignore the `redirectUrl` defined in `this.openidConnect` completely? And if so: Shouldn't `redirectToProviderIfNothingElseIsEnabled` be changed accordingly and the prop deleted from the store, etc.?
konrad commented 2021-12-07 18:24:04 +00:00
Author
Owner

Yes and yes. I'm not quite sure about the security implications of this yet, but since the openid provider is usually responsable for allowing only some redirect urls it should be fine.

Yes and yes. I'm not quite sure about the security implications of this yet, but since the openid provider is usually responsable for allowing only some redirect urls it should be fine.
dpschen commented 2021-12-09 17:16:22 +00:00
Member

So you mean you would simply keep it like this?

So you mean you would simply keep it like this?
konrad commented 2021-12-12 15:23:53 +00:00
Author
Owner

So you mean you would simply keep it like this?

At least for now yes.

> So you mean you would simply keep it like this? At least for now yes.
dpschen commented 2021-12-12 15:29:56 +00:00
Member

I think I still don't get why we don't change it everywhere.
How about we add a comment to explain this?

I think I still don't get why we don't change it everywhere. How about we add a comment to explain this?
konrad commented 2021-12-12 15:37:51 +00:00
Author
Owner

I think that's a good idea, changed it everywhere and added a comment.

I think that's a good idea, changed it everywhere and added a comment.
👍 1
konrad marked this conversation as resolved
konrad was assigned by dpschen 2021-12-10 11:54:43 +00:00
konrad removed their assignment 2021-12-12 15:24:03 +00:00
dpschen was assigned by konrad 2021-12-12 15:24:04 +00:00
konrad added 1 commit 2021-12-12 15:37:30 +00:00
continuous-integration/drone/pr Build is failing Details
c8aa4b2da6
feat: redirect to calculated url everywhere
dpschen reviewed 2021-12-12 15:45:36 +00:00
src/views/user/Login.vue Outdated
@ -219,7 +220,7 @@ export default {
},
redirectToProvider(provider) {
dpschen commented 2021-12-12 15:45:36 +00:00
Member

replace with

redirectToProvider,

(no need to define params)

replace with ```js redirectToProvider, ``` (no need to define params)
konrad commented 2021-12-12 17:42:11 +00:00
Author
Owner

Done!

Done!
konrad marked this conversation as resolved
dpschen requested changes 2021-12-12 15:45:50 +00:00
dpschen left a comment
Member

Fine to merge after this mini fix

Fine to merge after this mini fix
konrad added 1 commit 2021-12-12 17:42:07 +00:00
continuous-integration/drone/pr Build is passing Details
2a07b8c2b9
chore: directly use redirectToProvider function
konrad commented 2021-12-12 17:42:33 +00:00
Author
Owner
Copy Link

@dpschen Should be fine now, feel free to merge once the CI passes.

@dpschen Should be fine now, feel free to merge once the CI passes.
dpschen approved these changes 2021-12-17 15:40:25 +00:00
dpschen merged commit 36fb250d1f into main 2021-12-17 15:41:12 +00:00
dpschen deleted branch feat/openid-redirect 2021-12-17 15:41:12 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
dpschen
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1144
No description provided.
Delete Branch "feat/openid-redirect"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?