[GH-235] Refresh token on user level oauth (#235) #242

jupriano · 2021-11-17T02:16:22Z

Summary

i think is only move the refresh token outside "if isAccountLevel" but if the refresh token function only for account level, how we get user level token ?

Ticket Link

Fixes #235

mattermod · 2021-11-17T02:16:25Z

Hello @vicky-demansol,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

codecov · 2021-11-17T02:20:59Z

Codecov Report

Merging #242 (9e85b76) into master (c4cda31) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master    #242   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           8       8           
  Lines         882     900   +18     
======================================
- Misses        882     900   +18

Impacted Files	Coverage Δ
server/plugin.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4cda31...9e85b76. Read the comment docs.

larkox · 2021-11-17T08:05:04Z

Thanks, @vicky-demansol but this won't work. If I understand the code correctly, you are just not making any distinction between AccountLevel and UserLevel by removing the if. Both tokens come from different places (take a look to the getActiveClient function), and have to be handled slightly different.

This may work if you instead of getting the token from the getSuperUserToken you get it directly from the client (since it is already fetch from the database), but then you also have to distinguish where you store it. You must store the account level one as the superuser token, or the user level one on the user information.

larkox

I like the approach, but I fear it may not be handling all the cases correctly. Take a look at the comments.

larkox · 2021-11-25T13:11:26Z

server/http.go

@@ -126,6 +126,12 @@ func (p *Plugin) completeUserOAuthToZoom(w http.ResponseWriter, r *http.Request)
 			http.Error(w, err.Error(), http.StatusInternalServerError)
 			return
 		}
+	} else {
+		err = p.setUserToken(userID, token)


This seems cleaner so far, but a few lines after (line 161) we are storing the token along the OAuthUserInfo. If we want to follow this pattern, we would need to refactor the token out of the OAuthUserInfo and fix whatever breaks from that.

Another solution would be to keep the current pattern, and just update the token when needed in the OAuthUserInfo structure when needed.

I leave up to you which path to take.

larkox · 2021-11-25T13:12:02Z

server/plugin.go

@@ -175,7 +175,10 @@ func (p *Plugin) getActiveClient(user *model.User) (Client, string, error) {
 	if err != nil {
 		return nil, message, errors.New("could not decrypt OAuth access token")
 	}
-
+	oldToken, err := p.getUserToken(user.Id)
+	if err == nil {


We should handle the error, and remove this (I guess) debug lines.

sibasankarnayak · 2021-11-29T14:46:37Z

server/plugin.go

+	oldToken, err := p.getUserToken(user.Id)
+	if err == nil {
+		p.API.LogInfo(fmt.Sprintf("oldTokenIs===========================%s", *oldToken))
+	}


Suggested change

}

if err != nil {

p.API.LogError("error while fetching token" , "error" , err.Error())

return

}

server/plugin.go

sibasankarnayak · 2021-11-29T14:48:03Z

server/plugin.go

+	if token == nil {
+		return nil, errors.New("zoom app not connected")
+	}
+	return token, nil


Suggested change

return token, nil

return token, nil

server/plugin.go

sibasankarnayak · 2021-11-29T14:48:49Z

server/plugin.go

+func (p *Plugin) SetZoomUserToken(userID string, token *oauth2.Token) error {
+	err := p.setUserToken(userID, token)
+	if err != nil {
+		return errors.Wrap(err, "could not set token")


error should be logged here or in the function getting called

sibasankarnayak · 2021-11-29T14:49:27Z

server/zoom/oauth.go

@@ -97,6 +97,7 @@ func (c *OAuthClient) GetMeeting(meetingID int) (*Meeting, error) {

 func (c *OAuthClient) getUserViaOAuth(user *model.User) (*User, error) {
 	url := fmt.Sprintf("%s/users/me", c.apiURL)
+
 	if c.isAccountLevel {


remove unwanted change here

mattermod · 2021-12-11T01:01:33Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @BenLloydPearson @aspleenic

sibasankarnayak

Some non blocking changes are pending , code looks good so far.

larkox

LGTM, Thanks!

dipak-demansol · 2022-01-25T16:08:25Z

@hanzei can you pls tell me what i have to Test in this PR? what is the expected result for this?

hanzei · 2022-01-26T09:05:18Z

@larkox Can you please help with tests steps here? I'm not sure how to reproduce crated a token that timed out.

dipak-demansol · 2022-01-26T09:08:20Z

@larkox Can you please help with tests steps here? I'm not sure how to reproduce crated a token that timed out.

@larkox @hanzei is this related to this? #239

hanzei · 2022-01-26T09:32:06Z

@dipak-demansol Yes

larkox · 2022-01-26T13:53:46Z

I don't remember the timeline 100%, but in the old version, after you connect, if you wait for, I think, one hour, the token will be outdated and will force you to disconnect and connect again.

In order to test it, have two environments: one with the new version, and one with the old, and connect to zoom (on user level oauth) roughly at the same time. Then let that simmer for a few hours (or overnight), and try to create a new meeting. The old version should fail, saying that the token is expired, or something like that, and the new version should work without issues.

It would be useful also to have a third environment with account level app, to see if that is still working fine.

dipak-demansol

LGTM, Tested With Two server, 1st server had a market zoom plugin on a Chrome browser and 2nd server had a PR_code zoom app on the Brave browser, after 6 hour issue generated in the 1st server but the same issue not generated in the 2nd server.
Additional Info :- with the chrome browser and 2nd server when i create zoom meeting and join the meeting then i'm successfully join the meeting but in the brew browser with the 2nd server when i perform the same step then zoom don't identify me as a host and its wait for start the meeting. i will discuss with dylan about that.

* [GH-235] Refresh token on user level oauth (#235) * [GH-235] Add Oauth User Level Refresh Token (#235) * [GH-235] Remove Unwanted Change * [GH-235] Add Error Log * [GH-235] Add Oauth User Level Refresh Token (#235) Co-authored-by: root <[email protected]> Co-authored-by: jupriano <[email protected]> Co-authored-by: Ben Schumacher <[email protected]>

This reverts commit 8a5a54d.

[mattermostGH-235] Refresh token on user level oauth (mattermost#235)

55291a8

jupriano requested a review from larkox as a code owner November 17, 2021 02:16

hanzei added the 2: Dev Review Requires review by a core committer label Nov 22, 2021

[mattermostGH-235] Add Oauth User Level Refresh Token (mattermost#235)

5414f37

larkox requested changes Nov 25, 2021

View reviewed changes

sibasankarnayak reviewed Nov 29, 2021

View reviewed changes

server/plugin.go Show resolved Hide resolved

sibasankarnayak reviewed Nov 29, 2021

View reviewed changes

server/plugin.go Show resolved Hide resolved

sibasankarnayak reviewed Nov 29, 2021

View reviewed changes

hanzei added Awaiting Submitter Action Blocked on the author 3: QA Review Requires review by a QA tester labels Nov 30, 2021

jupriano added 2 commits December 6, 2021 12:44

[mattermostGH-235] Remove Unwanted Change

aeb9d4b

[mattermostGH-235] Add Error Log

502b78b

mickmister mentioned this pull request Dec 9, 2021

[MM-40214] Register zoom app bar component #246

Merged

mattermod added the Lifecycle/1:stale label Dec 11, 2021

jupriano and others added 2 commits December 14, 2021 14:16

[mattermostGH-235] Add Oauth User Level Refresh Token (mattermost#235)

34a68c0

Merge branch 'master' into mattermostGH-235

b4147bd

hanzei requested review from sibasankarnayak and larkox December 20, 2021 12:11

sibasankarnayak approved these changes Dec 20, 2021

View reviewed changes

Merge branch 'master' into mattermostGH-235

c93327e

larkox approved these changes Jan 11, 2022

View reviewed changes

hanzei requested a review from dipak-demansol January 25, 2022 13:49

hanzei removed the 2: Dev Review Requires review by a core committer label Jan 25, 2022

hanzei removed Awaiting Submitter Action Blocked on the author Lifecycle/1:stale labels Jan 25, 2022

Merge branch 'master' into mattermostGH-235

9e85b76

hanzei added Awaiting Submitter Action Blocked on the author and removed Awaiting Submitter Action Blocked on the author labels Jan 26, 2022

hanzei added this to the v2.0.0 milestone Jan 26, 2022

dipak-demansol approved these changes Feb 1, 2022

View reviewed changes

dipak-demansol added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Feb 1, 2022

hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed QA Review Done PR has been approved by QA labels Feb 7, 2022

hanzei assigned larkox Feb 7, 2022

larkox merged commit 8a5a54d into mattermost:master Feb 7, 2022

mickmister mentioned this pull request Feb 16, 2022

Fix OAuth token refresh #253

Merged

mickmister added a commit that referenced this pull request Jul 14, 2022

Revert "[GH-235] Refresh token on user level oauth (#235) (#242)"

c5d440e

This reverts commit 8a5a54d.

mickmister mentioned this pull request Jul 14, 2022

Revert "[GH-235] Refresh token on user level oauth (#235) (#242)" #268

Merged

mickmister added a commit that referenced this pull request Jul 21, 2022

Revert "[GH-235] Refresh token on user level oauth (#235) (#242)" (#268)

8434d19

This reverts commit 8a5a54d.

This was referenced Nov 14, 2023

Bump version v2.0.0 (closed, renamed to v1.7.0) #321

Closed

Bump version v1.7.0. #322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-235] Refresh token on user level oauth (#235) #242

[GH-235] Refresh token on user level oauth (#235) #242

jupriano commented Nov 17, 2021 •

edited by hanzei

Loading

mattermod commented Nov 17, 2021

codecov bot commented Nov 17, 2021 •

edited

Loading

larkox commented Nov 17, 2021

larkox left a comment

larkox Nov 25, 2021

larkox Nov 25, 2021

sibasankarnayak Nov 29, 2021 •

edited

Loading

sibasankarnayak Nov 29, 2021

sibasankarnayak Nov 29, 2021

sibasankarnayak Nov 29, 2021

mattermod commented Dec 11, 2021

sibasankarnayak left a comment

larkox left a comment

dipak-demansol commented Jan 25, 2022

hanzei commented Jan 26, 2022

dipak-demansol commented Jan 26, 2022

hanzei commented Jan 26, 2022

larkox commented Jan 26, 2022

dipak-demansol left a comment

[GH-235] Refresh token on user level oauth (#235) #242

[GH-235] Refresh token on user level oauth (#235) #242

Conversation

jupriano commented Nov 17, 2021 • edited by hanzei Loading

Summary

Ticket Link

mattermod commented Nov 17, 2021

codecov bot commented Nov 17, 2021 • edited Loading

Codecov Report

larkox commented Nov 17, 2021

larkox left a comment

Choose a reason for hiding this comment

larkox Nov 25, 2021

Choose a reason for hiding this comment

larkox Nov 25, 2021

Choose a reason for hiding this comment

sibasankarnayak Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

sibasankarnayak Nov 29, 2021

Choose a reason for hiding this comment

sibasankarnayak Nov 29, 2021

Choose a reason for hiding this comment

sibasankarnayak Nov 29, 2021

Choose a reason for hiding this comment

mattermod commented Dec 11, 2021

sibasankarnayak left a comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

dipak-demansol commented Jan 25, 2022

hanzei commented Jan 26, 2022

dipak-demansol commented Jan 26, 2022

hanzei commented Jan 26, 2022

larkox commented Jan 26, 2022

dipak-demansol left a comment

Choose a reason for hiding this comment

jupriano commented Nov 17, 2021 •

edited by hanzei

Loading

codecov bot commented Nov 17, 2021 •

edited

Loading

sibasankarnayak Nov 29, 2021 •

edited

Loading