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

[feature/tokenauth-uidenforce] Enforce User ID when updating token-based bookmarks #869

Merged
merged 5 commits into from
Jan 26, 2021

Conversation

felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Jan 12, 2021

Description

This PR requires the user ID to remain the same when updating token-based bookmarks. If the user logs in as a user other than the one with which the bookmark was originally created, an error will be presented.

Related Issue

Screenshots (if appropriate):

Error trying to log in as different user (edit bookmark) Regular login error Error trying to log in as different user (inline editing in "opened" account)
Bildschirmfoto 2021-01-12 um 23 52 44 Bildschirmfoto 2021-01-12 um 23 53 10 Bildschirmfoto 2021-01-12 um 23 53 23

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

- Authentication: require username to stay the same when updating OAuth2 / OIDC bookmarks
- SDK update to support this
@CLAassistant
Copy link

CLAassistant commented Jan 12, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ hosy
❌ felix-schwarz
You have signed the CLA already but the status is still pending? Let us recheck it.

@felix-schwarz felix-schwarz added this to the 11.5.0-Current milestone Jan 12, 2021
@hosy
Copy link
Collaborator

hosy commented Jan 13, 2021

@felix-schwarz For a while we talked about adding the username as parameter for the authentication web view, so that the username value is automatically set.
Does it make sense to implement it inside this PR?

@felix-schwarz
Copy link
Contributor Author

@hosy Yeah, that could make sense as it is closely related. Last time we tried adding it for OC OAuth2, however, it crashed the web view (and curiously also Safari, when opening the URL there), hinting at a WebKit bug. So I'd skip that for OC OAuth2.

For OIDC, however, it's certainly worth looking into. I'll check the RFC if there is a parameter we can send along.

@felix-schwarz
Copy link
Contributor Author

felix-schwarz commented Jan 13, 2021

@hosy Ok, checked and for OIDC the SDK is already passing along the username as login_hint (spec). However, it is not used by the IDP of the ocis.owncloud.works test server.

Also got feedback that this is not supported by the Kopano IDP yet. But as soon as it is, this should work.

@felix-schwarz
Copy link
Contributor Author

felix-schwarz commented Jan 13, 2021

Re OAuth2: I just re-checked if Apple fixed the WebKit crash in iOS 14.3, but unfortunately, they didn't:
Bildschirmfoto 2021-01-13 um 10 49 45

alertController.addAction(UIAlertAction(title: "Sign in".localized, style: .default, handler: { [weak self] (_) in
completionHandler()

var notifyAuthDelegate = true
Copy link
Contributor

Choose a reason for hiding this comment

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

@felix-schwarz reduce nesting with guard let statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I didn't find any opportunity to simplify the logic there with guard let statements. Still could reduce nesting by unifying ifs that were nested unnecessarily.

@jesmrec
Copy link
Contributor

jesmrec commented Jan 26, 2021

QA checks

  • Edit OAuth2 bookmark, logging in with different user -> correct error

  • Revoke OAuth2 token, logging in with different user -> correct error

  • Edit OIDC bookmark, logging in with different user -> correct error

  • Edit OAuth2 bookmark, logging in with same user -> correct login

  • Revoke OAuth2 token, logging in with same user -> correct login

  • Edit OIDC bookmark, logging in with same user -> correct login

@jesmrec
Copy link
Contributor

jesmrec commented Jan 26, 2021

Approved on my side

@jesmrec jesmrec added the Approved by QA Approved by QA label Jan 26, 2021
@hosy hosy merged commit 1c54190 into milestone/11.5 Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved by QA Approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants