Skip to content

Conversation

@moldis
Copy link
Contributor

@moldis moldis commented Aug 5, 2021

--revoke share didn't work since the last gosdk update. This fix should resolve it

@moldis moldis changed the title [BUG] Fix with revoked share [WIP] [BUG] Fix with revoked share Aug 9, 2021
@moldis moldis changed the title [WIP] [BUG] Fix with revoked share Fix with revoked share Aug 18, 2021
existingShare, err := reference.GetShareInfo(ctx, authTicket.ClientID, authTicket.FilePathHash)
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check err here? We need to know if db throw any error?

func GetShareInfo(ctx context.Context, clientID string, filePathHash string) (*ShareInfo, error) {
	db := datastore.GetStore().GetTransaction(ctx)
	shareInfo := &ShareInfo{}
	err := db.Table(TableName()).
		Where(&ShareInfo{
			ClientID:     clientID,
			FilePathHash: filePathHash,
		}).
		First(shareInfo).Error

	if err == gorm.ErrRecordNotFound {
		return nil, nil
	}
	if err != nil {
		return nil, err
	}
	return shareInfo, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it's wrong 'tricky' implementation that causes previous bugs. I removed this check from DB level

return nil, errors.New("error during share info lookup in database" + err.Error())
} else if shareInfo == nil || shareInfo.Revoked {

if err == nil && shareInfo.Revoked {
Copy link
Contributor

Choose a reason for hiding this comment

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

app might crash if shareInfo is nil? It is better to check it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly what was done in the previous comment. It' can't be nil if err is nil.

@cnlangzi
Copy link
Contributor

cnlangzi commented Aug 19, 2021

here is an example for my review. thanks


var ErrServiceUnavailable = errors.New("service unavailable")
if err != nil { //any network/db error
    if errors.Is(err, ErrHasNotShared) {
         return nil, err
    }
    traceid := logging.Error(err)
    return nil, errors.Throw(ErrServiceUnavailable,traceid)
}

if shareInfo == nil ||  shareInfo.Removed {
    return nil, ErrHasNotShared
}


var ErrHasNotShared = errors.New("client does not have permission to download the file. share revoked")


func GetShareInfo(ctx context.Context, clientID string, filePathHash string) (*ShareInfo, error) {
	db := datastore.GetStore().GetTransaction(ctx)
	shareInfo := &ShareInfo{}
	err := db.Table(TableName()).
		Where(&ShareInfo{
			ClientID:     clientID,
			FilePathHash: filePathHash,
		}).
		First(shareInfo).Error

	if err != nil {

    if errors.Is(err, gorm.ErrRecordNotFound) {
        return nil, ErrHasNotShared
    }

		return nil, err
	}
	return shareInfo, nil
}

@cnlangzi
Copy link
Contributor

cnlangzi commented Aug 19, 2021

BTW, errors.Throw is in github.com/0chain/errors package.

@moldis
Copy link
Contributor Author

moldis commented Aug 19, 2021

@cnlangzi This has to be applied globally and it's not following my current logic for sharing

@moldis moldis merged commit 461de48 into master Aug 20, 2021
@kushthedude kushthedude deleted the bug/fix-revoke-access branch August 20, 2021 19:06
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.

3 participants